Re: [PATCH-patch v6] cgroup: Show # of subsystem CSSes in cgroup.stat

[Waiman Long]

On 7/13/24 11:50, Kamalesh Babulal wrote:
> On 7/13/24 4:59 AM, Waiman Long wrote:
> > Cgroup subsystem state (CSS) is an abstraction in the cgroup layer to
> > help manage different structures in various cgroup subsystems by being
> > an embedded element inside a larger structure like cpuset or mem_cgroup.
> >
> > The /proc/cgroups file shows the number of cgroups for each of the
> > subsystems.  With cgroup v1, the number of CSSes is the same as the
> > number of cgroups.  That is not the case anymore with cgroup v2. The
> > /proc/cgroups file cannot show the actual number of CSSes for the
> > subsystems that are bound to cgroup v2.
> >
> > So if a v2 cgroup subsystem is leaking cgroups (usually memory cgroup),
> > we can't tell by looking at /proc/cgroups which cgroup subsystems may
> > be responsible.
> >
> > As cgroup v2 had deprecated the use of /proc/cgroups, the hierarchical
> > cgroup.stat file is now being extended to show the number of live and
> > dying CSSes associated with all the non-inhibited cgroup subsystems
> > that have been bound to cgroup v2 as long as they are not both zero.
> > The number includes CSSes in the current cgroup as well as in all the
> > descendants underneath it.  This will help us pinpoint which subsystems
> > are responsible for the increasing number of dying (nr_dying_descendants)
> > cgroups.
> >
> > The CSSes dying counts are stored in the cgroup structure itself
> > instead of inside the CSS as suggested by Johannes. This will allow
> > us to accurately track dying counts of cgroup subsystems that have
> > recently been disabled in a cgroup. It is now possible that a zero
> > subsystem number is coupled with a non-zero dying subsystem number.
> >
> > The cgroup-v2.rst file is updated to discuss this new behavior.
> >
> > With this patch applied, a sample output from root cgroup.stat file
> > was shown below.
> >
> > 	nr_descendants 56
> > 	nr_subsys_cpuset 1
> > 	nr_subsys_cpu 43
> > 	nr_subsys_io 43
> > 	nr_subsys_memory 56
> > 	nr_subsys_perf_event 57
> > 	nr_subsys_hugetlb 1
> > 	nr_subsys_pids 56
> > 	nr_subsys_rdma 1
> > 	nr_subsys_misc 1
> > 	nr_dying_descendants 30
> > 	nr_dying_subsys_cpuset 0
> > 	nr_dying_subsys_cpu 0
> > 	nr_dying_subsys_io 0
> > 	nr_dying_subsys_memory 30
> > 	nr_dying_subsys_perf_event 0
> > 	nr_dying_subsys_hugetlb 0
> > 	nr_dying_subsys_pids 0
> > 	nr_dying_subsys_rdma 0
> > 	nr_dying_subsys_misc 0
> >
> > Another sample output from system.slice/cgroup.stat was:
> >
> > 	nr_descendants 32
> > 	nr_subsys_cpu 30
> > 	nr_subsys_io 30
> > 	nr_subsys_memory 32
> > 	nr_subsys_perf_event 33
> > 	nr_subsys_pids 32
> > 	nr_dying_descendants 32
> > 	nr_dying_subsys_cpu 0
> > 	nr_dying_subsys_io 0
> > 	nr_dying_subsys_memory 32
> > 	nr_dying_subsys_perf_event 0
> > 	nr_dying_subsys_pids 0
> [...]
>
> > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> > index c8e4b62b436a..73774c841100 100644
> > --- a/kernel/cgroup/cgroup.c
> > +++ b/kernel/cgroup/cgroup.c
> > @@ -3669,12 +3669,43 @@ static int cgroup_events_show(struct seq_file *seq, void *v)
> >   static int cgroup_stat_show(struct seq_file *seq, void *v)
> >   {
> >   	struct cgroup *cgroup = seq_css(seq)->cgroup;
> > +	struct cgroup_subsys_state *css;
> > +	int dying_cnt[CGROUP_SUBSYS_COUNT];
> > +	int ssid;
> >   
> >   	seq_printf(seq, "nr_descendants %d\n",
> >   		   cgroup->nr_descendants);
> > +
> > +	/*
> > +	 * Show the number of live and dying csses associated with each of
> > +	 * non-inhibited cgroup subsystems that is either enabled in current
> > +	 * cgroup or has non-zero dying count.
> > +	 *
> > +	 * Without proper lock protection, racing is possible. So the
> > +	 * numbers may not be consistent when that happens.
> > +	 */
> > +	rcu_read_lock();
> > +	for (ssid = 0; ssid < CGROUP_SUBSYS_COUNT; ssid++) {
> > +		dying_cnt[ssid] = -1;
> > +		if (BIT(ssid) & cgrp_dfl_inhibit_ss_mask)
> > +			continue;
> > +		css = rcu_dereference_raw(cgroup->subsys[ssid]);
> > +		if (!css && !cgroup->nr_dying_subsys[ssid])
> > +			continue;
> Sorry, If I have misread the discussion from the other thread about displaying
> nr_descendants and nr_dying_subsys_<subsys>. I believe the idea was to print
> them for enabled and disabled cgroup controllers, so the output stays consistent
> and does not vary depending on the enabled controllers or previously enabled
> controller with nr_dying_subsys > 0.
>
> For example, on a rebooted vm:
>
> # cd /sys/fs/cgroup/
> # cat cgroup.subtree_control
> cpu memory pids
>
> # mkdir foo
> # cat foo/cgroup.stat
> nr_descendants 0
> nr_subsys_cpu 1
> nr_subsys_memory 1
> nr_subsys_perf_event 1
> nr_subsys_pids 1
> nr_dying_descendants 0
> nr_dying_subsys_cpu 0
> nr_dying_subsys_memory 0
> nr_dying_subsys_perf_event 0
> nr_dying_subsys_pids 0
>
> # echo '+cpuset' > cgroup.subtree_control
>
> # cat foo/cgroup.stat
> nr_descendants 0
> nr_subsys_cpuset 1
> nr_subsys_cpu 1
> nr_subsys_memory 1
> nr_subsys_perf_event 1
> nr_subsys_pids 1
> nr_dying_descendants 0
> nr_dying_subsys_cpuset 0
> nr_dying_subsys_cpu 0
> nr_dying_subsys_memory 0
> nr_dying_subsys_perf_event 0
> nr_dying_subsys_pids 0

I am fine with fine with that. I will update the patch as suggested.

Thanks,
Longman

> > +
> > +		dying_cnt[ssid] = cgroup->nr_dying_subsys[ssid];
> > +		seq_printf(seq, "nr_subsys_%s %d\n", cgroup_subsys[ssid]->name,
> > +			   css ? (css->nr_descendants + 1) : 0);
> > +	}
> > +
> >   	seq_printf(seq, "nr_dying_descendants %d\n",
> >   		   cgroup->nr_dying_descendants);
> > -
> > +	for (ssid = 0; ssid < CGROUP_SUBSYS_COUNT; ssid++) {
> > +		if (dying_cnt[ssid] >= 0)
> > +			seq_printf(seq, "nr_dying_subsys_%s %d\n",
> > +				   cgroup_subsys[ssid]->name, dying_cnt[ssid]);
> > +	}
> > +	rcu_read_unlock();
> >   	return 0;
> >   }
> >