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 972F3C021A4 for ; Mon, 24 Feb 2025 18:36:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 056B728000F; Mon, 24 Feb 2025 13:36:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F20E328000E; Mon, 24 Feb 2025 13:36:31 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DC1CC28000F; Mon, 24 Feb 2025 13:36:31 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id B0CE028000E for ; Mon, 24 Feb 2025 13:36:31 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 49FC2A3F24 for ; Mon, 24 Feb 2025 18:36:31 +0000 (UTC) X-FDA: 83155693782.06.C8F089F Received: from out-181.mta0.migadu.com (out-181.mta0.migadu.com [91.218.175.181]) by imf27.hostedemail.com (Postfix) with ESMTP id 26A724001D for ; Mon, 24 Feb 2025 18:36:28 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=GAxR+QNe; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf27.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.181 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740422189; a=rsa-sha256; cv=none; b=W6CQ47jKSQo5Btiji53TWD8KIh9vC0P55Zwa7hSwz3gVeUJ3izYh1Qa5k8O9opGThEDZiC +B4lxozbZDZGHHyIQPTHiLhNYOu9xEy8WnUn5klZ4VNzPkc1wIJHbW85IS7GL7yi3jY/ns Q6v56Ib9vjm5ueSNhAYPHgvrmiQ4HgY= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=GAxR+QNe; dmarc=pass (policy=none) header.from=linux.dev; spf=pass (imf27.hostedemail.com: domain of yosry.ahmed@linux.dev designates 91.218.175.181 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=1740422189; 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=NiXNNmTrJUgmWa7uPBQD4a8uvZYwoSh0GRtnyVAIVxI=; b=6p9eixLdv83mzz8KTt+9aJdue8m5iy8fd3HwDIXCiDZ5+lyVT/O8Atbj/gLE60D8Gxv5e0 hptZWT/E//tmqMZli5DzHUL1qxCiTNDiYfFeGW2RSgj4tS2BxKs2LhwE2k/SztbXlffju6 CnfeTxE1H3NvV41dPd7RBlucd+YLNAU= Date: Mon, 24 Feb 2025 18:36:22 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740422187; 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=NiXNNmTrJUgmWa7uPBQD4a8uvZYwoSh0GRtnyVAIVxI=; b=GAxR+QNeLEWMlqPUu4kvAHKEPPlOijX9LBUGJn6CwF5ne0gFrdvieWJ0TWfCbG1IYx/CL6 ldzYiqVB0pYh7HrqKaTjx7q8jpREPaFofYBQOoXsFYy/UZhvw1lBE5+MPZfISloY/rKimn f0Ew3/Kl9QhAmpmsKeIBBBz0gIIZdzM= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: JP Kobryn Cc: shakeel.butt@linux.dev, tj@kernel.org, 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 01/11] cgroup: move rstat pointers into struct of their own Message-ID: References: <20250218031448.46951-1-inwardvessel@gmail.com> <20250218031448.46951-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-Rspam-User: X-Rspamd-Queue-Id: 26A724001D X-Rspamd-Server: rspam12 X-Stat-Signature: xy4hsfryd6hnt83jep6mefowjf8siayd X-HE-Tag: 1740422188-67221 X-HE-Meta: U2FsdGVkX1/KjCCf2xvRZoOuFWYEN6gUh6buHMrBmu9EKlgmnFOvIFfPpXktfKEUma4TygWl7OabDNmvrT+PU9KsZSxXuazRhwLDrW2owLr7m7NQ6ThuW1I3e4PTRrBjt4w/iyskAO6v2tsD1LHc3o6SR9S6UcUqUnKdIQwormKRA0QPBMt0HD9Q/2HNlGSOUd8FseXMnClH0iPdIRTGZkf929Q3hQfKa1cjr010dIpgGBv227hc6zGM5poEzE+yxDHgUtdjBhqJ7BGPoxj9+jw8N2MbZr36FUOR8ZIUWQ+/l1sYCCj4IQDegZM94/Lb78n5oI4RjfphEquIFkdr+qNR+HonQvSMeKaucpS0t87wJQu2IZdyg6nD+A9KCaiqA/khW9LEswxfYevL/7hGImidtPQOprTDi3+RS2MsMxXL6aQi10+SwCinBCNcOTe9u2SGYe3c4fZ8XbOTU4agmwVdFN/urMwoUH2H+BrrU0tj6FgWn/EDROiAj0Ky+GIuRfhoHblCYs4W/pKoE7dT7zCwZR+C6Qr8KTcmkWZXMwDmYVwVlqSrg5RzkC2UAgp9Nn044q/Axy/1nNE5GxStoK0p8Ynr+FpNY9ykNvEozQr25e7zqmcztEAkad6SQaUe6L5fdjexCds7KegXU/33KNnoqCze/m+tVt7UhgVtJSL3G29pLoKa+IUeYzKbVwftXdLb9jtqFZmOUSy+v8GKfmWz5EmKmKfhGKZ+KOhCon3KcPNayHlEG+Lcu4Jas1vuFhdOxYdsOUoSVfsZk66cIpFEe2jAcW8kq4i5Mv9Sw7Bmx+qQRQh3WYLcRc+xPrXx+vDeoxYb6yop4rjgFQiieYz5xnuLCSD+xUdjNlFNuQGU1ngDLsSM5/Wk08QJ9BFInlOrwrO3jjoBss8+HW0ZsQPsS30EDj034J4xlkaoRJx2aytjOZFYzzhDUXxOx4tG5rR9RrFNkN3fE2dpBOx JJrTKDiA +rP7avdbnHTAIWUifUIKwrd88nC0PT3yyFdQOKalHWNXTrURECy/6kz0HhXq1dmIG1OWZNpT0/6n0EOXizU56j8cKReCfexKctzXhYQwgshEQGUOjgeFn2v2HcNDg8K/IGWsNG9We2/TwupXCjORRgtu8KVvFfkSU98i0Nv+heqlL84m8M+WgG1vDe6QEZoxxGRkPxNWc2QTyk5pQ4yxZztVyAxNW1ORrNRjTnFTOyY4bIqE= 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 Mon, Feb 24, 2025 at 09:06:21AM -0800, JP Kobryn wrote: > On 2/20/25 8:53 AM, Yosry Ahmed wrote: > > On Mon, Feb 17, 2025 at 07:14:38PM -0800, JP Kobryn wrote: > > > The rstat infrastructure makes use of pointers for list management. > > > These pointers only exist as fields in the cgroup struct, so moving them > > > into their own struct will allow them to be used elsewhere. The base > > > stat entities are included with them for now. > > > > > > Signed-off-by: JP Kobryn > > > --- > > > include/linux/cgroup-defs.h | 90 +----------------- > > > include/linux/cgroup_rstat.h | 92 +++++++++++++++++++ > > > kernel/cgroup/cgroup.c | 3 +- > > > kernel/cgroup/rstat.c | 27 +++--- > > > .../selftests/bpf/progs/btf_type_tag_percpu.c | 4 +- > > > 5 files changed, 112 insertions(+), 104 deletions(-) > > > create mode 100644 include/linux/cgroup_rstat.h > > > > > > diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h > > > index 1b20d2d8ef7c..6b6cc027fe70 100644 > > > --- a/include/linux/cgroup-defs.h > > > +++ b/include/linux/cgroup-defs.h > > > @@ -17,7 +17,7 @@ > > > #include > > > #include > > > #include > > > -#include > > > +#include > > > #include > > > #include > > > #include > > > @@ -321,78 +321,6 @@ struct css_set { > > > struct rcu_head rcu_head; > > > }; > > > -struct cgroup_base_stat { > > > - struct task_cputime cputime; > > > - > > > -#ifdef CONFIG_SCHED_CORE > > > - u64 forceidle_sum; > > > -#endif > > > - u64 ntime; > > > -}; > > > - > > > -/* > > > - * rstat - cgroup scalable recursive statistics. Accounting is done > > > - * per-cpu in cgroup_rstat_cpu which is then lazily propagated up the > > > - * hierarchy on reads. > > > - * > > > - * When a stat gets updated, the cgroup_rstat_cpu and its ancestors are > > > - * linked into the updated tree. On the following read, propagation only > > > - * considers and consumes the updated tree. This makes reading O(the > > > - * number of descendants which have been active since last read) instead of > > > - * O(the total number of descendants). > > > - * > > > - * This is important because there can be a lot of (draining) cgroups which > > > - * aren't active and stat may be read frequently. The combination can > > > - * become very expensive. By propagating selectively, increasing reading > > > - * frequency decreases the cost of each read. > > > - * > > > - * This struct hosts both the fields which implement the above - > > > - * updated_children and updated_next - and the fields which track basic > > > - * resource statistics on top of it - bsync, bstat and last_bstat. > > > - */ > > > -struct cgroup_rstat_cpu { > > > - /* > > > - * ->bsync protects ->bstat. These are the only fields which get > > > - * updated in the hot path. > > > - */ > > > - struct u64_stats_sync bsync; > > > - struct cgroup_base_stat bstat; > > > - > > > - /* > > > - * Snapshots at the last reading. These are used to calculate the > > > - * deltas to propagate to the global counters. > > > - */ > > > - struct cgroup_base_stat last_bstat; > > > - > > > - /* > > > - * This field is used to record the cumulative per-cpu time of > > > - * the cgroup and its descendants. Currently it can be read via > > > - * eBPF/drgn etc, and we are still trying to determine how to > > > - * expose it in the cgroupfs interface. > > > - */ > > > - struct cgroup_base_stat subtree_bstat; > > > - > > > - /* > > > - * Snapshots at the last reading. These are used to calculate the > > > - * deltas to propagate to the per-cpu subtree_bstat. > > > - */ > > > - struct cgroup_base_stat last_subtree_bstat; > > > - > > > - /* > > > - * Child cgroups with stat updates on this cpu since the last read > > > - * are linked on the parent's ->updated_children through > > > - * ->updated_next. > > > - * > > > - * In addition to being more compact, singly-linked list pointing > > > - * to the cgroup makes it unnecessary for each per-cpu struct to > > > - * point back to the associated cgroup. > > > - * > > > - * Protected by per-cpu cgroup_rstat_cpu_lock. > > > - */ > > > - struct cgroup *updated_children; /* terminated by self cgroup */ > > > - struct cgroup *updated_next; /* NULL iff not on the list */ > > > -}; > > > - > > > struct cgroup_freezer_state { > > > /* Should the cgroup and its descendants be frozen. */ > > > bool freeze; > > > @@ -517,23 +445,9 @@ struct cgroup { > > > struct cgroup *old_dom_cgrp; /* used while enabling threaded */ > > > /* per-cpu recursive resource statistics */ > > > - struct cgroup_rstat_cpu __percpu *rstat_cpu; > > > + struct cgroup_rstat rstat; > > > struct list_head rstat_css_list; > > > - /* > > > - * Add padding to separate the read mostly rstat_cpu and > > > - * rstat_css_list into a different cacheline from the following > > > - * rstat_flush_next and *bstat fields which can have frequent updates. > > > - */ > > > - CACHELINE_PADDING(_pad_); > > > - > > > - /* > > > - * A singly-linked list of cgroup structures to be rstat flushed. > > > - * This is a scratch field to be used exclusively by > > > - * cgroup_rstat_flush_locked() and protected by cgroup_rstat_lock. > > > - */ > > > - struct cgroup *rstat_flush_next; > > > - > > > /* cgroup basic resource statistics */ > > > struct cgroup_base_stat last_bstat; > > > struct cgroup_base_stat bstat; > > > diff --git a/include/linux/cgroup_rstat.h b/include/linux/cgroup_rstat.h > > > new file mode 100644 > > > index 000000000000..f95474d6f8ab > > > --- /dev/null > > > +++ b/include/linux/cgroup_rstat.h > > > @@ -0,0 +1,92 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +#ifndef _LINUX_RSTAT_H > > > +#define _LINUX_RSTAT_H > > > + > > > +#include > > > + > > > +struct cgroup_rstat_cpu; > > > > Why do we need the forward declaration instead of just defining struct > > cgroup_rstat_cpu first? Also, why do we need a new header for these > > definitions rather than just adding struct cgroup_rstat to > > cgroup-defs.h? > > The new header was added so the cgroup_rstat type can be used in bpf > cgroup-defs.h. As for the forward declaration, this was done so that > updated_next and updated children fields of the cgroup_rstat_cpu can > change type from from cgroup to cgroup_rstat. > > Regardless, based on the direction we are moving with bpf sharing the > "self" tree, this new header will NOT be needed in v2. Nice. Seems like the series will be simplified quite a bit by doing this.