Re: tools/cgroup/memcg_slabinfo.py is broken

[Matthew Wilcox <willy@infradead.org>]

On Wed, Feb 16, 2022 at 02:26:18PM -0800, Roman Gushchin wrote:
> On Wed, Feb 16, 2022 at 09:55:29PM +0000, Matthew Wilcox wrote:
> > On Wed, Feb 16, 2022 at 01:44:10PM -0800, Roman Gushchin wrote:
> > > On Wed, Feb 16, 2022 at 09:12:01PM +0000, Matthew Wilcox wrote:
> > > > On Wed, Feb 16, 2022 at 12:52:36PM -0800, Roman Gushchin wrote:
> > > > > The problem can be fixed by explicitly casting struct page * to struct
> > > > > slab * for slab pages. The tools works as expected with this fix, e.g.:
> > > > 
> > > > This feels like a quick fix, and not really correct.
> > > 
> > > Do you mind to provide a correct version?
> > 
> > I know nothing about python, nor how to even run this script.  But this
> > is the kind of thing I was thinking about.
> > 
> > I didn't do the 'skip over tail pages' because I have no idea how
> > you'd tell for_each_page() to do that.  I don't know where to get
> > drgn.helpers.linux from, so I can't look at the implementation.
> > Maybe it already does that.
> 
> Is there an actual plan to set the slab flag for tail pages?
> If not, why to check both? It's fairly expensive already, so I'd
> not add any additional checks if there is no strict necessity.

The plan to set the slab flag on tail pages is probably about five
years away.  There are a lot of moving parts before we get there.
I think you could reduce the cost if we had a for_each_folio()
loop.  Most code doesn't really want to look at every page, so
having a shorter list (and iterating over each page within a folio
for the few places that really do want to see every page) would
be a good tradeoff.

> > @@ -145,14 +145,14 @@ def detect_kernel_config():
> >      return cfg
> >  
> >  
> > -def for_each_slab_page(prog):
> > +def for_each_slab(prog):
> 
> Here I'd keep _page (or _folio, if you want), because
> it makes it clear that the function goes over all pages
> (and is expensive therefore).

But there's no intrinsic reason that it should.  If that's a
performance problem, you could keep slabs on a separate list
from, eg, file or anon memory.