Message-ID: <20200428170645.GA158422@carbon.dhcp.thefacebook.com>
Date:   Tue, 28 Apr 2020 10:06:45 -0700
From:   Roman Gushchin <guro@...com>
To:     Christopher Lameter <cl@...ux.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>, <linux-mm@...ck.org>,
        <kernel-team@...com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 04/19] mm: slub: implement SLUB version of
 obj_to_index()

On Mon, Apr 27, 2020 at 09:46:38AM -0700, Roman Gushchin wrote:
> On Mon, Apr 27, 2020 at 04:21:01PM +0000, Christoph Lameter wrote:
> > On Fri, 24 Apr 2020, Roman Gushchin wrote:
> > 
> > > > The patch seems to only use it for setup and debugging? It is used for
> > > > every "accounted" allocation???? Where? And what is an "accounted"
> > > > allocation?
> > > >
> > > >
> > >
> > > Please, take a look at the whole series:
> > > https://lore.kernel.org/linux-mm/20200422204708.2176080-1-guro@fb.com/T/#t
> > >
> > > I'm sorry, I had to cc you directly for the whole thing. Your feedback
> > > will be highly appreciated.
> > >
> > > It's used to calculate the offset of the memcg pointer for every slab
> > > object which is charged to a memory cgroup. So it must be quite hot.
> > 
> > 
> > Ahh... Thanks. I just looked at it.
> > 
> > You need this because you have a separate structure attached to a page
> > that tracks membership of the slab object to the cgroup. This is used to
> > calculate the offset into that array....
> > 
> > Why do you need this? Just slap a pointer to the cgroup as additional
> > metadata onto the slab object. Is that not much simpler, safer and faster?
> > 
> 
> So, the problem is that not all slab objects are accounted, and sometimes
> we don't know if advance if they are accounted or not (with the current semantics
> of __GFP_ACCOUNT and SLAB_ACCOUNT flags). So we either have to increase
> the size of ALL slab objects, either create a pair of slab caches for each size.
> 
> The first option is not that cheap in terms of the memory overhead. Especially
> for those who disable cgroups using a boot-time option.
> The second should be fine, but it will be less simple in terms of the code complexity
> (in comparison to the final result of the current proposal).
> 
> I'm not strictly against of either approach, but I'd look for a broader consensus
> on what's the best approach here.

To be more clear here: in my original version (prior to v3) I had two sets
of kmem_caches: one for root- and other non accounted allocations, and the
other was shared by all non-root memory cgroups. With this approach it's
easy to switch to your suggestion and put the memcg pointer nearby the object.

Johannes persistently pushed on the design with a single set of kmem_caches,
shared by *all* allocations. I've implemented this approach as a separate patch
on top of the series and added to v3. It allows to dramatically simplify the code
and remove ~0.5k sloc, but with this approach it's not easy to implement what
you're suggesting without increasing the size of *all* slab objects, which is
sub-optimal.

So it looks like there are two options:
1) switch back to a root- and memcg sets of kmem_caches, put the memcg pointer
   just behind the slab object
2) stick with what we've in v3

I guess the first option might be better from the performance POV, the second
is simpler/cleaner in terms of the code. So I'm ok to switch to 1) if there is
a consensus on what's better.

Thanks!

Powered by blists - more mailing lists