Re: [PATCH v3 1/2] cgroup: Show # of subsystem CSSes in cgroup.stat

[Waiman Long]

On 7/11/24 09:49, Johannes Weiner wrote:
> On Wed, Jul 10, 2024 at 02:23:52PM -0400, Waiman Long wrote:
> > @@ -3669,12 +3669,34 @@ 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 ssid;
> >   
> > +	/* cgroup_mutex required for for_each_css() */
> > +	cgroup_lock();
> >   	seq_printf(seq, "nr_descendants %d\n",
> >   		   cgroup->nr_descendants);
> >   	seq_printf(seq, "nr_dying_descendants %d\n",
> >   		   cgroup->nr_dying_descendants);
> >   
> > +	/*
> > +	 * Show the number of live and dying csses associated with each of
> > +	 * non-inhibited cgroup subsystems bound to cgroup v2 if non-zero.
> > +	 */
> > +	for_each_css(css, ssid, cgroup) {
> > +		if ((BIT(ssid) & cgrp_dfl_inhibit_ss_mask) ||
> > +		    (cgroup_subsys[ssid]->root !=  &cgrp_dfl_root))
> > +			continue;
> > +
> > +		seq_printf(seq, "nr_%s %d\n", cgroup_subsys[ssid]->name,
> > +			   css->nr_descendants + 1);
> > +		/* Current css is online */
> > +		if (css->nr_dying_descendants)
> > +			seq_printf(seq, "nr_dying_%s %d\n",
> > +				   cgroup_subsys[ssid]->name,
> > +				   css->nr_dying_descendants);
> > +	}
> I think it'd be better to print the dying count unconditionally. It
> makes the output more predictable for parsers, and also it's clearer
> to users which data points are being tracked and reported.
>
> With that, and TJ's "subsys" suggestion for the name, it looks good to
> me. Thanks!

Given the fact that for_each_css() iteration is filtering out csses that 
are absent, the dying counts follow the same logic of skipping it if 
there is no dying css. That also makes it easier to identify cgroups 
with dying descendant csses as we don't need filter out entries with a 0 
dying count. It also makes the output less verbose and let user focus 
more on what are significant.

I do understand that it makes it inconsistent with the ways 
nr_descendants and nr_dying_descendants are being handled as entries 
with 0 count are also displayed. I can update the patch to display those 
entries with 0 dying subsys count if other people also think that is the 
better way forward.

Cheers,
Longman

>