linux-kernel - Re: [PATCH] cgroup: add cgroup.stat interface with basic hierarchy stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170728130155.GA13728@castle.dhcp.TheFacebook.com>
Date:   Fri, 28 Jul 2017 14:01:55 +0100
From:   Roman Gushchin <guro@...com>
To:     Tejun Heo <tj@...nel.org>
CC:     <cgroups@...r.kernel.org>, Zefan Li <lizefan@...wei.com>,
        Waiman Long <longman@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>, <kernel-team@...com>,
        <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cgroup: add cgroup.stat interface with basic hierarchy
 stats

On Thu, Jul 27, 2017 at 12:22:43PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Thu, Jul 27, 2017 at 05:14:20PM +0100, Roman Gushchin wrote:
> > Add a cgroup.stat interface to the base cgroup control files
> > with the following metrics:
> > 
> > nr_descendants		total number of descendant cgroups
> > nr_dying_descendants	total number of dying descendant cgroups
> > max_descendant_depth	maximum descent depth below the current cgroup
> 
> Yeah, this'd be great to have.  Some comments below.
> 
> > +  cgroup.stat
> > +	A read-only flat-keyed file with the following entries:
> > +
> > +	  nr_descendants
> > +		Total number of descendant cgroups.
> > +
> > +	  nr_dying_descendants
> > +		Total number of dying descendant cgroups.
> 
> Can you please go into more detail on what's going on with dying
> descendants here?

Sure.
Don't we plan do describe cgroup/css lifecycle in details
in a separate section?

> > +static int cgroup_stats_show(struct seq_file *seq, void *v)
> > +{
> > +	struct cgroup_subsys_state *css;
> > +	unsigned long total = 0;
> > +	unsigned long offline = 0;
> > +	int max_level = 0;
> > +
> > +	rcu_read_lock();
> > +	css_for_each_descendant_pre(css, seq_css(seq)) {
> > +		if (css == seq_css(seq))
> > +			continue;
> > +		++total;
> 
> Let's do post increment for consistency.

Ok.

> 
> > +		if (!(css->flags & CSS_ONLINE))
> > +			++offline;
> > +		if (css->cgroup->level > max_level)
> > +			max_level = css->cgroup->level;
> > +	}
> > +	rcu_read_unlock();
> 
> I wonder whether we want to keep these counters in sync instead of
> trying to gather the number on read.  Walking all descendants can get
> expensive pretty quickly and things like nr_descendants will be useful
> for other purposes too.

Ok, assuming that I'm working on adding an ability to set limits
on cgroup hierarchy, it seems very reasonable. I'll implement this
and post the updated path as a part of a patchset.

Thanks!