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 B3A93C54EE9 for ; Sun, 18 Sep 2022 11:08:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E9346940008; Sun, 18 Sep 2022 07:08:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E1BB3940007; Sun, 18 Sep 2022 07:08:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C6EC8940008; Sun, 18 Sep 2022 07:08:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id B2CDB940007 for ; Sun, 18 Sep 2022 07:08:21 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 7480DC053C for ; Sun, 18 Sep 2022 11:08:21 +0000 (UTC) X-FDA: 79924932402.17.4A502A9 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) by imf24.hostedemail.com (Postfix) with ESMTP id 11AAD180003 for ; Sun, 18 Sep 2022 11:08:20 +0000 (UTC) Received: by mail-ed1-f44.google.com with SMTP id z13so23392677edb.13 for ; Sun, 18 Sep 2022 04:08:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=colorfullife-com.20210112.gappssmtp.com; s=20210112; h=in-reply-to:from:content-language:references:cc:to:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date; bh=Oea3lZdjTibNOlHVzcym0NcZxwPywJ9NIc7pdYdsFKA=; b=uhm3hv5hEl6jxLpaGezD9+GEo9z87injzAGd2aaMHERYOK0bk7t2Z7gx1xsrGQaakg ytmW/BHICQANTPPpsJqtzhMbL5VVNvLe0ruhBOCUhkQhMC9lNke9ND84y+Fyv/tIfwHA GhVIilJw2yxmmDGgwkYNqLMmtZMGU4xE8jZ5OHicaScoSpNDrTx9u4hiC4NVsyk7P9Rq nG+EdpjOLYxF60Vvp2I1zY++82+A+ywE8Y7gEFwtRLnrGDHHOST48l2pdLuXmLs16Tos A8kxgSYnuwZqkeo+JjXgh2/MB7K0H0EqrRWH6igK9RfnIs0zvSRBJIPASO4VAWPl/C79 h4/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:from:content-language:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date; bh=Oea3lZdjTibNOlHVzcym0NcZxwPywJ9NIc7pdYdsFKA=; b=f6d/OmFgiClASdpOgZb/b3XXAKIn/wPceb1bUux79PGbVK0RSOQZhlsJYXNamkQAJG 0fZgMCLQKLX/Qdcl7yoES628g5RTBiRWCdgdm8FFfCCSVmaJ4ukF+TzktDsen+7P6hRc tLPXDGWw6aFltjuUhxnkllFGj3oBezReLDlxN5OGTkCUez72kl02de5g+bwPpJQMdjPl eHrOw5ccZnb4rCEkiKDwShsDlEYs1X1idAmOR2blJUe9GyME8ys9NAK2FZaGCee1ePYm +ihytnByMXSqxuUHtXIMiBMRywn53glsOOKvMhhnNjnyl50jC2h15U5SSn2v8r7m7zd8 Oj1w== X-Gm-Message-State: ACrzQf1UCBYcBRG9cogviZm3w0BciuZNdeu3eu8RqA6slk2L6s+xhtXq kul6pBM2aCyM/eXub90Tr0Rujg== X-Google-Smtp-Source: AMsMyM5VwUQ+Uh69CR2BN4i+hFB41Guy/pxpdhPp2IcmNvq5/LkPkvXxAz7W+BKyERlqapCNl9GQWQ== X-Received: by 2002:aa7:d5d0:0:b0:44e:f6cc:7107 with SMTP id d16-20020aa7d5d0000000b0044ef6cc7107mr11000522eds.371.1663499299479; Sun, 18 Sep 2022 04:08:19 -0700 (PDT) Received: from ?IPV6:2003:d9:9726:8300:b088:c09:537d:ce99? (p200300d997268300b0880c09537dce99.dip0.t-ipconnect.de. [2003:d9:9726:8300:b088:c09:537d:ce99]) by smtp.googlemail.com with ESMTPSA id cz19-20020a0564021cb300b0044657ecfbb5sm17714983edb.13.2022.09.18.04.08.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 18 Sep 2022 04:08:18 -0700 (PDT) Content-Type: multipart/mixed; boundary="------------EF1201diUEP5cbqrvHB1wETx" Message-ID: Date: Sun, 18 Sep 2022 13:08:16 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH v6 1/2] percpu: Add percpu_counter_add_local and percpu_counter_sub_local To: Jiebin Sun , 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> Content-Language: en-US From: Manfred Spraul In-Reply-To: <20220913192538.3023708-2-jiebin.sun@intel.com> ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1663499301; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Oea3lZdjTibNOlHVzcym0NcZxwPywJ9NIc7pdYdsFKA=; b=DEHrxIfcBr9tN+1q3sXrnaNn8MlSb14PYrRmSoyI8TvkG9AFzCVLCY3saT/SKtNy3YymzS 9b5BxTVDQ8XNNhtq+eoc7/QI/LbbB2/EUiHrm5VQ5eXdKhBmLeRHcwgfXMmp+JazTJDwcU Wj5yIrH4QMWMecY6b4cZgFwGk/88JvE= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=colorfullife-com.20210112.gappssmtp.com header.s=20210112 header.b=uhm3hv5h; spf=pass (imf24.hostedemail.com: domain of manfred@colorfullife.com designates 209.85.208.44 as permitted sender) smtp.mailfrom=manfred@colorfullife.com; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1663499301; a=rsa-sha256; cv=none; b=OgV8ZDo2bQkhHrq0SCMuij6CrCKHpxnM0gSrB5kUrVIuNvNbPro9Pojs0nItmjBoEWF7Gq M7xYsP/SbBsuT8MZ0l51129fm23D2rVAED3lVyGfuIt5prwmwZiJr09isERNWPE7HYd/x7 dj+NulRJ3PoMNFUYid1YZScZ60B+MZE= X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 11AAD180003 X-Rspam-User: Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=colorfullife-com.20210112.gappssmtp.com header.s=20210112 header.b=uhm3hv5h; spf=pass (imf24.hostedemail.com: domain of manfred@colorfullife.com designates 209.85.208.44 as permitted sender) smtp.mailfrom=manfred@colorfullife.com; dmarc=none X-Stat-Signature: heqcejuhiqq46o6so1db7gcycqhrqyxc X-HE-Tag: 1663499300-79064 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: This is a multi-part message in MIME format. --------------EF1201diUEP5cbqrvHB1wETx Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 --------------EF1201diUEP5cbqrvHB1wETx Content-Type: text/x-patch; charset=UTF-8; name="0001-lib-percpu_counter-RFC-potential-overflow-underflow.patch" Content-Disposition: attachment; filename*0="0001-lib-percpu_counter-RFC-potential-overflow-underflow.pat"; filename*1="ch" Content-Transfer-Encoding: base64 RnJvbSA2YTFkMmE0YmViMTgwMjQxYjYzZjliZjU3NDU0YmJlMDMxOTE1ZGQxIE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBNYW5mcmVkIFNwcmF1bCA8bWFuZnJlZEBjb2xvcmZ1 bGxpZmUuY29tPgpEYXRlOiBTdW4sIDE4IFNlcCAyMDIyIDEyOjE3OjI3ICswMjAwClN1Ympl Y3Q6IFtQQVRDSF0gbGliL3BlcmNwdV9jb3VudGVyOiBbUkZDXSBwb3RlbnRpYWwgb3ZlcmZs b3cvdW5kZXJmbG93CgpJZiBhbiBpbnRlcnJ1cHQgaGFwcGVucyBiZXR3ZWVuIF9fdGhpc19j cHVfcmVhZCgqZmJjLT5jb3VudGVycykgYW5kCnRoaXNfY3B1X2FkZCgqZmJjLT5jb3VudGVy cywgYW1vdW50KSwgYW5kIHRoYXQgaW50ZXJydXB0IG1vZGlmaWVzCnRoZSBwZXJfY3B1X2Nv dW50ZXIsIHRoZW4gdGhlIHRoaXNfY3B1X2FkZCgpIGFmdGVyIHRoZSBpbnRlcnJ1cHQKcmV0 dXJucyBtYXkgdW5kZXIvb3ZlcmZsb3cuCgpUaHVzOiBEaXNhYmxlIGludGVycnVwdHMuCgpO b3RlOiBUaGUgcGF0Y2ggaXMgaW5jb21wbGV0ZSwgaWYgdGhlIHJhY2UgaXMgcmVhbCwgdGhl bgptb3JlIGZ1bmN0aW9ucyB0aGFuIGp1c3QgcGVyY3B1X2NvdW50ZXJfYWRkX2JhdGNoKCkg bmVlZHMgdG8gYmUKdXBkYXRlZC4KCkVzcGVjaWFsbHksIHRoZSAhQ09ORklHX1NNUCBjb2Rl IGxvb2tzIHdyb25nIHRvIG1lIGFzIHdlbGw6Cj4gc3RhdGljIGlubGluZSB2b2lkCj4gcGVy Y3B1X2NvdW50ZXJfYWRkKHN0cnVjdCBwZXJjcHVfY291bnRlciAqZmJjLCBzNjQgYW1vdW50 KQo+IHsKPglwcmVlbXB0X2Rpc2FibGUoKTsKPglmYmMtPmNvdW50ICs9IGFtb3VudDsKPglw cmVlbXB0X2VuYWJsZSgpOwo+IH0KVGhlIHVwZGF0ZSBvZiBmYmMtPmNvdW50IGlzIG5vdCBJ UlEgc2FmZS4KClNpZ25lZC1vZmYtYnk6IE1hbmZyZWQgU3ByYXVsIDxtYW5mcmVkQGNvbG9y ZnVsbGlmZS5jb20+Ci0tLQogbGliL3BlcmNwdV9jb3VudGVyLmMgfCA4ICsrKysrLS0tCiAx IGZpbGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQoKZGlmZiAt LWdpdCBhL2xpYi9wZXJjcHVfY291bnRlci5jIGIvbGliL3BlcmNwdV9jb3VudGVyLmMKaW5k ZXggZWQ2MTBiNzVkYzMyLi4zOWRlOTRkNTliNGYgMTAwNjQ0Ci0tLSBhL2xpYi9wZXJjcHVf Y291bnRlci5jCisrKyBiL2xpYi9wZXJjcHVfY291bnRlci5jCkBAIC04MiwxOCArODIsMjAg QEAgRVhQT1JUX1NZTUJPTChwZXJjcHVfY291bnRlcl9zZXQpOwogdm9pZCBwZXJjcHVfY291 bnRlcl9hZGRfYmF0Y2goc3RydWN0IHBlcmNwdV9jb3VudGVyICpmYmMsIHM2NCBhbW91bnQs IHMzMiBiYXRjaCkKIHsKIAlzNjQgY291bnQ7CisJdW5zaWduZWQgbG9uZyBmbGFnczsKIAog CXByZWVtcHRfZGlzYWJsZSgpOworCWxvY2FsX2lycV9zYXZlKGZsYWdzKTsKIAljb3VudCA9 IF9fdGhpc19jcHVfcmVhZCgqZmJjLT5jb3VudGVycykgKyBhbW91bnQ7CiAJaWYgKGFicyhj b3VudCkgPj0gYmF0Y2gpIHsKLQkJdW5zaWduZWQgbG9uZyBmbGFnczsKLQkJcmF3X3NwaW5f bG9ja19pcnFzYXZlKCZmYmMtPmxvY2ssIGZsYWdzKTsKKwkJcmF3X3NwaW5fbG9jaygmZmJj LT5sb2NrKTsKIAkJZmJjLT5jb3VudCArPSBjb3VudDsKIAkJX190aGlzX2NwdV9zdWIoKmZi Yy0+Y291bnRlcnMsIGNvdW50IC0gYW1vdW50KTsKLQkJcmF3X3NwaW5fdW5sb2NrX2lycXJl c3RvcmUoJmZiYy0+bG9jaywgZmxhZ3MpOworCQlyYXdfc3Bpbl91bmxvY2soJmZiYy0+bG9j ayk7CiAJfSBlbHNlIHsKIAkJdGhpc19jcHVfYWRkKCpmYmMtPmNvdW50ZXJzLCBhbW91bnQp OwogCX0KKwlsb2NhbF9pcnFfcmVzdG9yZShmbGFncyk7CiAJcHJlZW1wdF9lbmFibGUoKTsK IH0KIEVYUE9SVF9TWU1CT0wocGVyY3B1X2NvdW50ZXJfYWRkX2JhdGNoKTsKLS0gCjIuMzcu MgoK --------------EF1201diUEP5cbqrvHB1wETx--