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 7E7FAC6FD1D for ; Tue, 4 Apr 2023 06:01:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CCFB26B0071; Tue, 4 Apr 2023 02:01:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C7F566B0074; Tue, 4 Apr 2023 02:01:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B47006B0078; Tue, 4 Apr 2023 02:01:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id A555C6B0071 for ; Tue, 4 Apr 2023 02:01:34 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 6534BC0C7A for ; Tue, 4 Apr 2023 06:01:34 +0000 (UTC) X-FDA: 80642661708.12.7AB81EC Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf28.hostedemail.com (Postfix) with ESMTP id 91B18C0014 for ; Tue, 4 Apr 2023 06:01:31 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=WrzCtmwg; spf=pass (imf28.hostedemail.com: domain of dchinner@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=dchinner@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680588091; 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=cEjburEshfupS59FzIzY20KcgCn+Z0OofnnG6gTk1R4=; b=8Q5mDqK1PltWLSJfO40tY1CTkigY+ByCR++kH/qWXpCOcL+HsccJLctxRPdDn5sd/FfjAO nHUpJXsUBGGERvl6TX/P9v+6OR9VGXMY8mNyPuVl/HNxXawQezl4fG2xKwQWnIh5flPv+5 2k64A5seYaCulgAaFmsCLZ5LLXNZdPM= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=WrzCtmwg; spf=pass (imf28.hostedemail.com: domain of dchinner@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=dchinner@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680588091; a=rsa-sha256; cv=none; b=rmBr6EPmrvlqF2f3uqpqUSp7u3bL5y04UKdQ2IzXxPt8hnjWErjianFjVgU8tQKu6mYDig Exb/rrk0TtiCXWAMTh82Sf7ofcwhEFqPRjQj1ZYuUu08EfHMbmCyU4G0xXOThCMDrI3lPI 4B+GKgkqDNfsEtNvSa8wjOie/VuxxF0= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1680588090; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=cEjburEshfupS59FzIzY20KcgCn+Z0OofnnG6gTk1R4=; b=WrzCtmwgfOJIVP2ed9BpQtbT3pqZA/frdo4sXQ7OnVFg7j1uoUBGE0PT+bNW8YcGGdEgOx 9ZcNOClcUSpFnPDqAMJknzBy7ybhbt9poLfQipH1iGtl/ERHD6gojKqDzb9nqV2soA6Wen de3/8ARjIkcHIH2jUJt0EASDQTxhnM8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-73-RAYnqle7O1KjYBt4-em6lQ-1; Tue, 04 Apr 2023 02:01:26 -0400 X-MC-Unique: RAYnqle7O1KjYBt4-em6lQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4C9D9811E7C; Tue, 4 Apr 2023 06:01:25 +0000 (UTC) Received: from rh (vpn2-52-18.bne.redhat.com [10.64.52.18]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 257DE2027061; Tue, 4 Apr 2023 06:01:23 +0000 (UTC) Received: from localhost ([::1] helo=rh) by rh with esmtps (TLS1.3) tls TLS_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1pjZjM-002uhf-0s; Tue, 04 Apr 2023 16:01:20 +1000 Date: Tue, 4 Apr 2023 16:01:18 +1000 From: Dave Chinner To: Ye Bin Cc: dennis@kernel.org, tj@kernel.org, cl@linux.com, linux-mm@kvack.org, yury.norov@gmail.com, andriy.shevchenko@linux.intel.com, linux@rasmusvillemoes.dk, linux-kernel@vger.kernel.org, yebin10@huawei.com Subject: Re: [PATCH 2/2] lib/percpu_counter: fix dying cpu compare race Message-ID: References: <20230404014206.3752945-1-yebin@huaweicloud.com> <20230404014206.3752945-3-yebin@huaweicloud.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230404014206.3752945-3-yebin@huaweicloud.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.4 X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 91B18C0014 X-Rspam-User: X-Stat-Signature: gzzxarage3ozgxujsb4a7ye8sd1d8zzy X-HE-Tag: 1680588091-273754 X-HE-Meta: U2FsdGVkX1+34XRV684P4qUjOKnOwJN0efZ+vvbWpTVCapGOBLCiSf1ftWe1/JdeiprFNZF4L8+YqFAGz/DNC8vwTjJC6+HR75mDYgcI1PaKHP4vvRa+4D4BBf+7rO810jAWVJSHHuvgJAPT3V8rNJZXXHtHZkVrozQJi8FHNJ6Mkhh5LkJAqt9irqeUkol2QCI2QM7FzTYpxQNQ1n0uQJ4M45UJSiD0jSoX2Wv3lyFeu0ghLrIgpI8tQTrqFxE6O/12b9itv9aK1F/O4PutBQx8mDC6QfaY5GViENpUP+sQqU7gS877EFELCbRpK+GJyNqZtWjgJfLXY2zGgfUAh+gB71NPk/x5byzILjkULqLHamsXW4JUFXb70PZ759gWVYMYqBCYuEReaHIZvxpGz9zqe53b+w37sR5CWkJZ/fliT6nnWDf/8wdyJHDQ86h7eraLm6LZSxthwSiJtjDKB42TQKLh/UTSIw6qOqZGjl5bRuNx/wjmO2ZurAm6HgR3IcWltTioG1ENFMmRAF78Fj0ymP4S4P5k5toBUZuUDpXdCY8CS4ySLMfmwKKLYPFitMVpQIj3tB21CJo6XqKvEKgkc5RRcyK2HUxEDZ/EanWJIf/+va3Qtqh/FVX0Op5QYs7QfN7h6+18CuIMSUHP1QpEen9G6f48cXc4mRduG0rj2QJGYFm7EE1Ka1yHPslpzOvLLKAIdy98ply4Idw7/XNGH4inoaxRm2SZHpYD+zl8fQGHgC0I2Sbylm/+xnfJhyU8HhTQp+Zq4WGHYi0YQxlc5ENE0myCOr9M4jXQLuO0ZLGJV2GwrkOq0aTYInsQDUgyee3wPUPQFRBZUZd99TU/782MnijicY9pIRbNe4f4TZATADQJe9d6CMYCPQSpIEM9lUa5ZoAx38iLrUdlHriKy9/Uhf284thsk+4vSvIEMA/tjXpn5DtGuKSKciqdhAAPBwj5v9jPKa7NnYu sQHChMwu YaniemYCqvIqZ0MOce3FO7Hv3cl2DyUsb6Hhpgx1UB2mBVqVtXpSXSlH4pHttmwuTC6W8OlDLNLjHDnbv6U8IK02DhzqOAzHZTFkVr0gLTCTn+RcvBZKmnUIzZZLUqUs7CMZXBlFbpIpTQ+OeGkh+ZnqfhRtI3TF65S9XCC9Sk+01tVnZrYSqidqdL0Ror7D9d5kIBgoIfQKAgKSsijudP21rNpnWRh3eJQ2oeYpuOvogK6jy8KNvsEVXFCrP0Rz4/rZYaX47apzEYbxNKqmnigW8aKz3MDAxCXLhFHzqIzExn9OqRVPDjxgClSBfnZV4Hh6TxAuivl0OA/Cje5JEAISs9ObWYfgGDdVpYNdUrfbB3/A= 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 Tue, Apr 04, 2023 at 09:42:06AM +0800, Ye Bin wrote: > From: Ye Bin > > In commit 8b57b11cca88 ("pcpcntrs: fix dying cpu summation race") a race > condition between a cpu dying and percpu_counter_sum() iterating online CPUs > was identified. > Acctually, there's the same race condition between a cpu dying and > __percpu_counter_compare(). Here, use 'num_online_cpus()' for quick judgment. > But 'num_online_cpus()' will be decreased before call 'percpu_counter_cpu_dead()', > then maybe return incorrect result. > To solve above issue, also need to add dying CPUs count when do quick judgment > in __percpu_counter_compare(). > > Signed-off-by: Ye Bin > --- > lib/percpu_counter.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c > index 5004463c4f9f..399840cb0012 100644 > --- a/lib/percpu_counter.c > +++ b/lib/percpu_counter.c > @@ -227,6 +227,15 @@ static int percpu_counter_cpu_dead(unsigned int cpu) > return 0; > } > > +static __always_inline unsigned int num_count_cpus(void) > +{ > +#ifdef CONFIG_HOTPLUG_CPU > + return (num_online_cpus() + num_dying_cpus()); > +#else > + return num_online_cpus(); > +#endif > +} > + > /* > * Compare counter against given value. > * Return 1 if greater, 0 if equal and -1 if less > @@ -237,7 +246,7 @@ int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch) > > count = percpu_counter_read(fbc); > /* Check to see if rough count will be sufficient for comparison */ > - if (abs(count - rhs) > (batch * num_online_cpus())) { > + if (abs(count - rhs) > (batch * num_count_cpus())) { What problem is this actually fixing? You haven't explained how the problem you are fixing manifests in the commit message or the cover letter. We generally don't care about the accuracy of the comparison here because we've used percpu_counter_read() which is completely racy against on-going updates. e.g. we can get preempted between percpu_counter_read() and the check and so the value can be completely wrong by the time we actually check it. Hence checking online vs online+dying really doesn't fix any of the common race conditions that occur here. Even if we fall through to using percpu_counter_sum() for the comparison value, that is still not accurate in the face of racing updates to the counter because percpu_counter_sum only prevents the percpu counter from being folded back into the global sum while it is running. The comparison is still not precise or accurate. IOWs, the result of this whole function is not guaranteed to be precise or accurate; percpu counters cannot ever be relied on for exact threshold detection unless there is some form of external global counter synchronisation being used for those comparisons (e.g. a global spinlock held around all the percpu_counter_add() modifications as well as the __percpu_counter_compare() call). That's always been the issue with unsynchronised percpu counters - cpus dying just don't matter here because there are many other more common race conditions that prevent accurate, race free comparison of per-cpu counters. Cheers, Dave. -- Dave Chinner dchinner@redhat.com