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

Waiman Long
2024-07-11T14:05:34Z

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

>