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

From: Waiman Long
Date: Thu Jul 11 2024 - 10:05:41 EST



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