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 B30C0C83F03 for ; Thu, 3 Jul 2025 22:46:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 558946B02B5; Thu, 3 Jul 2025 18:46:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 52E596B02B6; Thu, 3 Jul 2025 18:46:24 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 46B3A6B02B7; Thu, 3 Jul 2025 18:46: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 2DD5E6B02B5 for ; Thu, 3 Jul 2025 18:46:24 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id E2B6480146 for ; Thu, 3 Jul 2025 22:46:23 +0000 (UTC) X-FDA: 83624438646.12.A67AEF3 Received: from out-183.mta1.migadu.com (out-183.mta1.migadu.com [95.215.58.183]) by imf21.hostedemail.com (Postfix) with ESMTP id 01B691C000A for ; Thu, 3 Jul 2025 22:46:21 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=kghDbcnD; spf=pass (imf21.hostedemail.com: domain of shakeel.butt@linux.dev designates 95.215.58.183 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1751582782; 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=iHwIqAp/B8daR4LVuH57/rc6G148Q4tQ/FTZcGDdVkk=; b=WpA/MwPGCPSwcPJGnLDwH767sHHrjWviVhQFcag74jYY6uxW3wxqxKsCH8HfspLsk6v6fZ C4vswJWtB/KaRSA3QjDWkDmqR3xI8fRFcgPrrBtWhZKwBAPGCnOJMaaIygEIXIiVyyQGLb fZzQ9udjsWgib7x5uV3OF6jG/6UJECs= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=kghDbcnD; spf=pass (imf21.hostedemail.com: domain of shakeel.butt@linux.dev designates 95.215.58.183 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1751582782; a=rsa-sha256; cv=none; b=2Zm1TyiHVHDFQqAcNmc3Fh5mSSDG38UdNVm5B7PbIl80RDjir7/8NgBuY0KnV4scQqzhNw GI1vSxSD1Oxj4s1JBno7LVrl2Z/Ddfcg1pgy387eE5xbdAJS7falKujAvoLup3ssyfWG02 uCbakp1W7krzgk19Zxcc825uFnMQrPg= Date: Thu, 3 Jul 2025 15:46:07 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1751582778; 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=iHwIqAp/B8daR4LVuH57/rc6G148Q4tQ/FTZcGDdVkk=; b=kghDbcnDvBVgHJ0dowMXteUg0gVzJ5n0x45H2J3+4Mb1Yx4kndrQkcqehVgjpJX8YzDOcl FdM4zive8jcp6WYd30Oc/kik2OkBoXPdyLHmlNPvu+l/OPYYZFpxVPmJVh3jqP1989jheL a4VUcbKYLjXSs9vkL5MHNS12tsBtcAw= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Shakeel Butt To: "Paul E. McKenney" Cc: Tejun Heo , Andrew Morton , JP Kobryn , Johannes Weiner , Ying Huang , Vlastimil Babka , Alexei Starovoitov , Sebastian Andrzej Siewior , Michal =?utf-8?Q?Koutn=C3=BD?= , bpf@vger.kernel.org, linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, Meta kernel team Subject: Re: [PATCH 2/2] cgroup: explain the race between updater and flusher Message-ID: References: <20250703200012.3734798-1-shakeel.butt@linux.dev> <20250703200012.3734798-2-shakeel.butt@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Stat-Signature: 6okk4sdkkkk46t4w1y1fw45ckasik3np X-Rspamd-Queue-Id: 01B691C000A X-Rspam-User: X-Rspamd-Server: rspam07 X-HE-Tag: 1751582781-912719 X-HE-Meta: U2FsdGVkX18dYf14ah/eMZVdSOtDsu3+sQOO32cj+DxiNPuPfa6RiUoZppVeDahDhTZHzF79ntaKcCpdax81osCsTP75n9aL4aI1jKQd++4bUhqZ5rJTzom3cT50teUA/UPnEWzEX4nAJ+Mbvc96u1YvjHj1AaWuO3IxbPjkwayKZkv6wy9Xiv0CpMnINfTLMHa2uGxAVwBE/pGovGun6EcpcpkwNU9lqq6pTIfKJE42MEvumSNpbLCxLQQK7X0X0IKSq1lXph3HHuQmzaaxBmJvC/aNCRvKS/DUi2+FxXscpvDvXbpaly3iSH1Wa+ic54F/jyNFMymmjZ6lN0CY6vOiXRRvgCd7Ut/xY73DFOl4YU1TpQJ8bzc8YuBUCEB8kzyYhvaw0ZGMtiHZYpvs7g1rbfSUQ1lc5VjTXttgnXqbuEWuCMOJns+mlpfc735AZ1x9B6tPUAAv+0kA7JuBX8zi/KqS6ymkKsZWOXxgSyjfJxje0pZPljDJTNNK4tORMfMEwGsd1VTTXfTs2NfXF9B4uLRYD6r0+VqvFX+fcxe6QyIgGJMRiyht1LtMJEtLwzTcGNs4XX8M/N/wPAY4dLCYMHM7jM/XNHR3ExgtM/nOxuw1k32gKxk4M7Wb6LfeGYKcqQKdbmBjaJbseiHcnjtZ0oMRcARQeCEzpzBHJS1FmWUdPpRceVnPRoXF/veheCXDlNJVZlFCqMMV+IvBNAWBjXP/nlxczlIP3FkSQSnUOsi0ChgEeHL1cPCYuHwNmfTo8iExpFURzbeXUeVlO8rXWnrgq7yxirjGeLbuMGsoohfkgCYdUGMOOPsvFsetn/ahGx9i94uwpFtQ/lx7/z6bihLo2mOJvBPkqjVxtjT4GQEz81210Ris6+lsULTrvxENc1s4O4aaDeHQOMf/0hm+fMkBS1IdQNGORd//jOtKdMelZVxTegthunVY5uSu8uQgOV18QofP7KVIP9P /pSwisOO gEkrYnSywbNvVKHT5duwTWhVkNZ90VR2DyDylOlhI1EoB9ChnQZQJv4eWtCrwksSDmGfx8SDm3SztU59Dqp1tTs749V/F1ijQCYhFWkJGpQ5fz6e+wcKabpfWH2LMbnvf4Tzs/kBEsRorD5kZLSA4enzF7q6RvmSRjTRhqyuRo0ZAx0LWzxZRnZmdpwN1JZDnjn0zpuKo+bDiEAP+eO2FP38QJ/ZDlth3rp6M5BzgsnRy8YnK21lHl/SI8g== 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: List-Subscribe: List-Unsubscribe: On Thu, Jul 03, 2025 at 03:29:16PM -0700, Paul E. McKenney wrote: > On Thu, Jul 03, 2025 at 01:00:12PM -0700, Shakeel Butt wrote: > > Currently the rstat updater and the flusher can race and cause a > > scenario where the stats updater skips adding the css to the lockless > > list but the flusher might not see those updates done by the skipped > > updater. This is benign race and the subsequent flusher will flush those > > stats and at the moment there aren't any rstat users which are not fine > > with this kind of race. However some future user might want more > > stricter guarantee, so let's add appropriate comments and data_race() > > tags to ease the job of future users. > > > > Signed-off-by: Shakeel Butt > > --- > > kernel/cgroup/rstat.c | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > > index c8a48cf83878..b98c03b1af25 100644 > > --- a/kernel/cgroup/rstat.c > > +++ b/kernel/cgroup/rstat.c > > @@ -60,6 +60,12 @@ static inline struct llist_head *ss_lhead_cpu(struct cgroup_subsys *ss, int cpu) > > * Atomically inserts the css in the ss's llist for the given cpu. This is > > * reentrant safe i.e. safe against softirq, hardirq and nmi. The ss's llist > > * will be processed at the flush time to create the update tree. > > + * > > + * NOTE: if the user needs the guarantee that the updater either add itself in > > + * the lockless list or the concurrent flusher flushes its updated stats, a > > + * memory barrier is needed before the call to css_rstat_updated() i.e. a > > + * barrier after updating the per-cpu stats and before calling > > + * css_rstat_updated(). > > */ > > __bpf_kfunc void css_rstat_updated(struct cgroup_subsys_state *css, int cpu) > > { > > @@ -86,8 +92,13 @@ __bpf_kfunc void css_rstat_updated(struct cgroup_subsys_state *css, int cpu) > > return; > > > > rstatc = css_rstat_cpu(css, cpu); > > - /* If already on list return. */ > > - if (llist_on_list(&rstatc->lnode)) > > + /* > > + * If already on list return. This check is racy and smp_mb() is needed > > + * to pair it with the smp_mb() in css_process_update_tree() if the > > + * guarantee that the updated stats are visible to concurrent flusher is > > + * needed. > > + */ > > + if (data_race(llist_on_list(&rstatc->lnode))) > > OK, I will bite... > > Why is this needed given the READ_ONCE() that the earlier patch added to > llist_on_list()? > > > return; > > > > /* > > @@ -145,9 +156,24 @@ static void css_process_update_tree(struct cgroup_subsys *ss, int cpu) > > struct llist_head *lhead = ss_lhead_cpu(ss, cpu); > > struct llist_node *lnode; > > > > - while ((lnode = llist_del_first_init(lhead))) { > > + while ((lnode = data_race(llist_del_first_init(lhead)))) { > > And for this one, why not make init_llist_node(), which is invoked from > llist_del_first_init(), do a WRITE_ONCE()? > Let me answer this one first. The previous patch actually made init_llist_node() do WRITE_ONCE(). So the actual question is why do we need data_race([READ|WRITE]_ONCE()) instead of just [READ|WRITE]_ONCE()? Actually I had the similar question myself and found the following comment in include/linux/compiler.h: /** * data_race - mark an expression as containing intentional data races * * This data_race() macro is useful for situations in which data races * should be forgiven. One example is diagnostic code that accesses * shared variables but is not a part of the core synchronization design. * For example, if accesses to a given variable are protected by a lock, * except for diagnostic code, then the accesses under the lock should * be plain C-language accesses and those in the diagnostic code should * use data_race(). This way, KCSAN will complain if buggy lockless * accesses to that variable are introduced, even if the buggy accesses * are protected by READ_ONCE() or WRITE_ONCE(). * * This macro *does not* affect normal code generation, but is a hint * to tooling that data races here are to be ignored. If the access must * be atomic *and* KCSAN should ignore the access, use both data_race() * and READ_ONCE(), for example, data_race(READ_ONCE(x)). */ IIUC correctly, I need to protect llist_node against tearing and as well as tell KCSAN to ignore the access for race then I should use both. Though I think KCSAN treat [READ|WRITE]_ONCE similar to data_race(), so it kind of seem redundant but I think at least I want to convey that we need protection against tearing and ignore KCSAN and using both conveys that. Let me know if you think otherwise. thanks a lot for taking a look.