From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5D407C6FA82 for ; Tue, 20 Sep 2022 06:01:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D272F940009; Tue, 20 Sep 2022 02:01:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CD67A940007; Tue, 20 Sep 2022 02:01:24 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B9DCD940009; Tue, 20 Sep 2022 02:01:24 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id AC1AF940007 for ; Tue, 20 Sep 2022 02:01:24 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 74B15A0AB1 for ; Tue, 20 Sep 2022 06:01:24 +0000 (UTC) X-FDA: 79931416488.29.B9C9FC5 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by imf26.hostedemail.com (Postfix) with ESMTP id 5BEFF14001D for ; Tue, 20 Sep 2022 06:01:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1663653683; x=1695189683; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=mNy52+6WxoYe/YHruALlMJMB2aDWgFWZufTGAbS/0S0=; b=EoyGBJjpIn+KbemBQeW9jWPolGgS69YS2NZ60rUfDJc7+GUiXaMPIv2t FBBfzpaSbilrJVCbMHO2vD/uQKuIiAlhcsd3qo0rgaZ34v8eof/mc3Zkv ZTlZl7iJ0d4y2BRV2wlSfjEYbDxTplEorerFK4ensDcD9iCBiElIIDKmY g0rDL+o/c4jZ410Hu8UwLj5+NTffEt7a+hd7kvF7nVKwjJ5Cyz0cWXIe2 EAh4CZpznmFW453NGplr1jQkvGeM7lBBaA5LuNExb+MbZNuimO3JCJRhx erq41YRr78x1szLcAaxgneuX/JLU3jZ/cWNLSnLBFSDHGO9V7ZyfhUllc g==; X-IronPort-AV: E=McAfee;i="6500,9779,10475"; a="286653224" X-IronPort-AV: E=Sophos;i="5.93,329,1654585200"; d="scan'208";a="286653224" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Sep 2022 23:01:21 -0700 X-IronPort-AV: E=Sophos;i="5.93,329,1654585200"; d="scan'208";a="649448596" Received: from jiebinsu-mobl.ccr.corp.intel.com (HELO [10.238.4.108]) ([10.238.4.108]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Sep 2022 23:01:16 -0700 Message-ID: Date: Tue, 20 Sep 2022 14:01:14 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.2.1 Subject: Re: [PATCH v6 1/2] percpu: Add percpu_counter_add_local and percpu_counter_sub_local Content-Language: en-US To: Manfred Spraul , akpm@linux-foundation.org, vasily.averin@linux.dev, shakeelb@google.com, dennis@kernel.org, tj@kernel.org, cl@linux.com, ebiederm@xmission.com, legion@kernel.org, alexander.mikhalitsyn@virtuozzo.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org Cc: tim.c.chen@intel.com, feng.tang@intel.com, ying.huang@intel.com, tianyou.li@intel.com, wangyang.guo@intel.com, Tim Chen , kernel test robot , 1vier1@web.de References: <20220902152243.479592-1-jiebin.sun@intel.com> <20220913192538.3023708-1-jiebin.sun@intel.com> <20220913192538.3023708-2-jiebin.sun@intel.com> From: "Sun, Jiebin" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=EoyGBJjp; spf=pass (imf26.hostedemail.com: domain of jiebin.sun@intel.com designates 134.134.136.20 as permitted sender) smtp.mailfrom=jiebin.sun@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1663653684; a=rsa-sha256; cv=none; b=GVhU8nhS5QQZrhkErn9KNx3rzQT2kRCU4H3wCQ6Y+F+tJUY1+VXf8g9IOuye1eQ7QeX/AV bPo8PLHzjIWNTyfKkU6uEwb0RKCyyGuGVJnd+861DNpCg0SW5UdtGDVmNUkVL5YM4U38om psumB01YIrOqtPGiU073mC2yOpBjw18= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1663653684; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=UD/ceIjOYzxvN2qYP/U6gQBqwab89n/BiXdJRm5Qb2c=; b=S9vvHLD19sY6uuN5bsOW1u/wEcMZq6bwpEo8uN/8tPk/jyiFRFbp0fh97fXrnKsbKMNLy0 FyiDe4tT6tv4TV7KkNiAS8fvqpwoKtUn2xp/CqrRkgTqhFgRDlXMS4Qb0On19LIAgRPXWG f7wQMdHiPrQi3aKPVFDFQuJyz4FoJqU= X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 5BEFF14001D X-Rspam-User: Authentication-Results: imf26.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=EoyGBJjp; spf=pass (imf26.hostedemail.com: domain of jiebin.sun@intel.com designates 134.134.136.20 as permitted sender) smtp.mailfrom=jiebin.sun@intel.com; dmarc=pass (policy=none) header.from=intel.com X-Stat-Signature: zkpskdng55s9fkefx13md1yttrwfehp8 X-HE-Tag: 1663653683-780826 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 9/18/2022 7:08 PM, Manfred Spraul wrote: > Hi Jiebin, > > On 9/13/22 21:25, Jiebin Sun wrote: >>   +/* >> + * With percpu_counter_add_local() and percpu_counter_sub_local(), >> counts >> + * are accumulated in local per cpu counter and not in fbc->count until >> + * local count overflows PERCPU_COUNTER_LOCAL_BATCH. This makes counter >> + * write efficient. >> + * But percpu_counter_sum(), instead of percpu_counter_read(), needs >> to be >> + * used to add up the counts from each CPU to account for all the local >> + * counts. So percpu_counter_add_local() and percpu_counter_sub_local() >> + * should be used when a counter is updated frequently and read rarely. >> + */ >> +static inline void >> +percpu_counter_add_local(struct percpu_counter *fbc, s64 amount) >> +{ >> +    percpu_counter_add_batch(fbc, amount, PERCPU_COUNTER_LOCAL_BATCH); >> +} >> + > > Unrelated to your patch, and not relevant for ipc/msg as the functions > are not called from interrupts, but: > Aren't there races with interrupts? > >> * >> * This function is both preempt and irq safe. The former is due to >> explicit >> * preemption disable. The latter is guaranteed by the fact that the >> slow path >> * is explicitly protected by an irq-safe spinlock whereas the fast >> patch uses >> * this_cpu_add which is irq-safe by definition. Hence there is no >> need muck >> * with irq state before calling this one >> */ >> void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount, >> s32 batch) >> { >>        s64 count; >> >>        preempt_disable(); >>        count = __this_cpu_read(*fbc->counters) + amount; >>        if (abs(count) >= batch) { >>                unsigned long flags; >>                raw_spin_lock_irqsave(&fbc->lock, flags); >>                fbc->count += count; >>                __this_cpu_sub(*fbc->counters, count - amount); >>                raw_spin_unlock_irqrestore(&fbc->lock, flags); >>        } else { >>                this_cpu_add(*fbc->counters, amount); >>        } >>        preempt_enable(); >> } >> EXPORT_SYMBOL(percpu_counter_add_batch); >> >> > Race 1: > > start: __this_cpu_read(*fbc->counters) = INT_MAX-1. > > Call: per_cpu_counter_add_batch(fbc, 1, INT_MAX); > > Result: > > count=INT_MAX; > > if (abs(count) >= batch) { // branch taken > > before the raw_spin_lock_irqsave(): > > Interrupt > > Within interrupt: > >    per_cpu_counter_add_batch(fbc, -2*(INT_MAX-1), INT_MAX) > >    count=-(INT_MAX-1); > >    branch not taken > >    this_cpu_add() updates fbc->counters, new value is -(INT_MAX-1) > >    exit interrupt > > raw_spin_lock_irqsave() > > __this_cpu_sub(*fbc->counters, count - amount) > > will substract INT_MAX-1 from *fbc->counters. But the value is already > -(INT_MAX-1) -> underflow. > > > Race 2: (much simpler) > > start: __this_cpu_read(*fbc->counters) = 0. > > Call: per_cpu_counter_add_batch(fbc, INT_MAX-1, INT_MAX); > > amont=INT_MAX-1; > > - branch not taken. > > before this_cpu_add(): interrupt > > within the interrupt: call per_cpu_counter_add_batch(fbc, INT_MAX-1, > INT_MAX) > >    new value of *fbc->counters: INT_MAX-1. > >    exit interrupt > > outside interrupt: > > this_cpu_add(*fbc->counters, amount); > > <<< overflow. > > Attached is an incomplete patch (untested). > If needed, I could check the whole file and add/move the required > local_irq_save() calls. > > > -- > >     Manfred The interrupt protect patch in the real case looks good to me. Thanks.