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 A6C45C282C5 for ; Sat, 1 Mar 2025 01:25:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2D9786B0085; Fri, 28 Feb 2025 20:25:14 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 2623D6B0088; Fri, 28 Feb 2025 20:25:14 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 10352280001; Fri, 28 Feb 2025 20:25:14 -0500 (EST) 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 E60186B0085 for ; Fri, 28 Feb 2025 20:25:13 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 6D4D5141583 for ; Sat, 1 Mar 2025 01:25:13 +0000 (UTC) X-FDA: 83171238906.12.A7F4D7D Received: from out-172.mta0.migadu.com (out-172.mta0.migadu.com [91.218.175.172]) by imf08.hostedemail.com (Postfix) with ESMTP id 92986160008 for ; Sat, 1 Mar 2025 01:25:11 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=CcLZInc9; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf08.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.172 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740792311; a=rsa-sha256; cv=none; b=SUyTF4OgUDichv3RxGmz/8gEmluQDbcQehRjtuyJ4bwaJ0Gh1ZXmOC5lwVTj8CKcBjAc4E FyZBhfcClyeBtQm5Ti9K/zhN7G+toi2n2bU4p+ZzSxfQbmquDPQc+PUVJ26scJZKwF7Bwt wPRHH4mILF4BeMjko4G0TAEn1ruzjmQ= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=CcLZInc9; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf08.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.172 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740792311; 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=xgzekGSfpi4MCbgi0ZXny+52I2B/jcmyprnXdtn4RtA=; b=Mg0RaD0R+EH6CbWckH4mJBCsN2bb4GgCippLYfDitPagV/+XZ4j5/PS3O1N30jG4tZABZY 1whi6tl5BEVVu11N/ZVxxgMyPbbb6zQ32uqdR3O9QB6dWAOl5B4Z8ObMkCHz2R8qCYhqJJ HxWD5xHi35c9a6UtdH0ln5vhEkL0v6I= Date: Sat, 1 Mar 2025 01:25:03 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740792309; 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=xgzekGSfpi4MCbgi0ZXny+52I2B/jcmyprnXdtn4RtA=; b=CcLZInc915gBfEHIJBfK+nXL/0F/GPJoqB6X9aF6P4VFp7zLZJSmQB3I2pMbH2kMXhOiIv BDY6rEPDvB1xwl9g1/MBH3mFlhAzUkc9n0dcO1/cXZ07b3tTY5V4RIUbxDzf8TpJ+3GNXu kSY6YihpLkGAW5gFPO8mwNXzcKEi3kw= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: JP Kobryn Cc: tj@kernel.org, shakeel.butt@linux.dev, mhocko@kernel.org, hannes@cmpxchg.org, akpm@linux-foundation.org, linux-mm@kvack.org, cgroups@vger.kernel.org, kernel-team@meta.com Subject: Re: [PATCH 1/4 v2] cgroup: move cgroup_rstat from cgroup to cgroup_subsys_state Message-ID: References: <20250227215543.49928-1-inwardvessel@gmail.com> <20250227215543.49928-2-inwardvessel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT X-Rspamd-Queue-Id: 92986160008 X-Stat-Signature: 43musjzpk4z1w3yg8d5fbokccjz8ar4n X-Rspam-User: X-Rspamd-Server: rspam05 X-HE-Tag: 1740792311-997069 X-HE-Meta: U2FsdGVkX19WLOZzCGVVW02Fstr6i09nkQWk/36WOCJLvABec5xHoNbUf0LjQsW4PVS1amtiu6zlXXAJHiTA1Z2HVCpL7dVc6TA4+a6KABt63+CxVhtad84p5ca87EKwKBN265MOFbYxOqzZMJsq/14X6f+pBFf7YUDN/gvnNVvptxDsWaD++EtxZLxelkUdYml9NR50oTS1RPoHS4fGxPEtsts5AKVVsfI7zDN5nlZFz9HZr0Cmr2EPV65heh0lxJHL2G4/uID0Bf5+I7v0efK96fF/TIHCw9BN0nsQAFcR8UNVHwwovuxMZ7iZlda8OjB9j7uzpbtVP2y8oihXovJuEs8a9+Kr1mdDBIscsKemhWFCYwEalQuibd4QyN8lwN9jZvpucse2IKIMZQ8VyYxSOd8xvDP5Ae3QlsO3Bl82Dv8d7WZHkzYzy4xT7uExJKQvwZ1EPUI6cmOJHj8qBaBJrcbNIrvE3E+kQn2uIpcnZoPA9xQUaM6U8FSjQ7piiIJRhRk6sk15K9jUQBi3sdWRnQfqLQ3ZhdRXG/fQ+GAuyeucYZtS/ELjSeVqthMHAQ40JV8Url0fz/YNPPX9dpkW7L4Do1L0uzmyJD+jm+wHMIOug+yIUPmhUVqE53U502cgINwwtT6YbCcMQmEwKFvb45h+9uQBQ2Qfrt7WNeKdckf+m+9HgdxxPi7ww5yb9D75a6lkjLH9pJ8rKAWub9bDvytp0M3hiBmf0fdvcTq28o12Nqsj5V1yeAT90XZVNRb7t5hdWuZC56DQ+Bk9/R5ELSE3/GCn5gUmIpVNnDf7lxAyiIgbivh/nlApk/KmhMgkNjfeDcR0ixERzPPXx67mUSoX0OyBrcnW/6IlXMH+FD1zX1MF1P3o+X3nNWYJexUSQNkWy3LNow8jCOlpQyiwYYU4fC47/jid1cCbxQM2GJ5ENIGaiub8Fv1/wXHZBO9mNJ5lU06ZBdCX05c mzjT6RlU CzrNmPjneH7QFJlD1JgqdXezNoFXgX0X54wMcDK4JAh1mtRSrR40CZfw2KMjnmjCIDj//OzTs3rVBga3PLgO6gTu8Aqweogv+25INid95ON95tQyanwbDV5kqx3PYzRvrLZ2lZ4CYVQ4WpK5SiSALTkYi3fryZOL6OiS1gOLhd8HZQMzluK2AKzdENcO/LQauf4kXxSnJ0wqwd8OBFjE0HWFs7nmPqySSw1Nv6H5pae0BZ8/j5QcutUoOwhM9qgmtFB6VYPyNYSwMv5c= 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 Fri, Feb 28, 2025 at 05:06:23PM -0800, JP Kobryn wrote: [..] > > > > > cgroup_idr_replace(&ss->css_idr, NULL, css->id); > > > if (ss->css_released) > > [..] > > > @@ -6188,6 +6186,9 @@ int __init cgroup_init(void) > > > css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2, > > > GFP_KERNEL); > > > BUG_ON(css->id < 0); > > > + > > > + if (css->ss && css->ss->css_rstat_flush) > > > + BUG_ON(cgroup_rstat_init(css)); > > > > Why do we need this call here? We already call cgroup_rstat_init() in > > cgroup_init_subsys(). IIUC for subsystems with ss->early_init, we will > > have already called cgroup_init_subsys() in cgroup_init_early(). > > > > Did I miss something? > > Hmmm it's a good question. cgroup_rstat_init() is deferred in the same > way that cgroup_idr_alloc() is. So for ss with early_init == true, > cgroup_rstat_init() is not called during cgroup_early_init(). Oh I didn't realize that the call here is only when early_init == false. I think we need a comment to clarify that cgroup_idr_alloc() and cgroup_rstat_init() are not called in cgroup_init_subsys() when early_init == true, and hence need to be called in cgroup_init(). Or maybe organize the code in a way to make this more obvious (put them in a helper with a descriptive name? idk). > > Is it safe to call alloc_percpu() during early boot? If so, the > deferral can be removed and cgroup_rstat_init() can be called in one > place. I don't think so. cgroup_init_early() is called before setup_per_cpu_areas(). > > > > > > } else { > > > cgroup_init_subsys(ss, false); > > > } > > [..] > > > @@ -300,27 +306,25 @@ static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop) > > > } > > > /* see cgroup_rstat_flush() */ > > > -static void cgroup_rstat_flush_locked(struct cgroup *cgrp) > > > +static void cgroup_rstat_flush_locked(struct cgroup_subsys_state *css) > > > __releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock) > > > { > > > + struct cgroup *cgrp = css->cgroup; > > > int cpu; > > > lockdep_assert_held(&cgroup_rstat_lock); > > > for_each_possible_cpu(cpu) { > > > - struct cgroup *pos = cgroup_rstat_updated_list(cgrp, cpu); > > > + struct cgroup_subsys_state *pos; > > > + pos = cgroup_rstat_updated_list(css, cpu); > > > for (; pos; pos = pos->rstat_flush_next) { > > > - struct cgroup_subsys_state *css; > > > + if (!pos->ss) > > > + cgroup_base_stat_flush(pos->cgroup, cpu); > > > + else > > > + pos->ss->css_rstat_flush(pos, cpu); > > > - cgroup_base_stat_flush(pos, cpu); > > > - bpf_rstat_flush(pos, cgroup_parent(pos), cpu); > > > - > > > - rcu_read_lock(); > > > - list_for_each_entry_rcu(css, &pos->rstat_css_list, > > > - rstat_css_node) > > > - css->ss->css_rstat_flush(css, cpu); > > > - rcu_read_unlock(); > > > + bpf_rstat_flush(pos->cgroup, cgroup_parent(pos->cgroup), cpu); > > > > We should call bpf_rstat_flush() only if (!pos->ss) as well, right? > > Otherwise we will call BPF rstat flush whenever any subsystem is > > flushed. > > > > I guess it's because BPF can now pass any subsystem to > > cgroup_rstat_flush(), and we don't keep track. I think it would be > > better if we do not allow BPF programs to select a css and always make > > them flush the self css. > > > > We can perhaps introduce a bpf_cgroup_rstat_flush() wrapper that takes > > in a cgroup and passes cgroup->self internally to cgroup_rstat_flush(). > > I'm fine with this if others are in agreement. A similar concept was > done in v1. Let's wait for Shakeel to chime in here since he suggested removing this hook, but I am not sure if he intended to actually do it or not. Better not to waste effort if this will be gone soon anyway. > > > > > But if the plan is to remove the bpf_rstat_flush() call here soon then > > it's probably not worth the hassle. > > > > Shakeel (and others), WDYT? >