netdev - Re: [PATCH v1] memcg: add charging of already allocated slab objects

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: <nt5zhccndtrj2pyyjm6wkah4iizzijdamaqce24t7nqioy4c5y@3vtipktwtzkn>
Date: Thu, 29 Aug 2024 08:49:29 -0700
From: Shakeel Butt <shakeel.butt@...ux.dev>
To: Muchun Song <muchun.song@...ux.dev>
Cc: Roman Gushchin <roman.gushchin@...ux.dev>, 
	Andrew Morton <akpm@...ux-foundation.org>, Johannes Weiner <hannes@...xchg.org>, 
	Michal Hocko <mhocko@...nel.org>, Vlastimil Babka <vbabka@...e.cz>, 
	David Rientjes <rientjes@...gle.com>, Hyeonggon Yoo <42.hyeyoo@...il.com>, 
	Eric Dumazet <edumazet@...gle.com>, "David S . Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Linux Memory Management List <linux-mm@...ck.org>, LKML <linux-kernel@...r.kernel.org>, 
	Meta kernel team <kernel-team@...a.com>, cgroups@...r.kernel.org, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v1] memcg: add charging of already allocated slab objects

On Thu, Aug 29, 2024 at 10:36:01AM GMT, Muchun Song wrote:
> 
> 
> > On Aug 29, 2024, at 03:03, Shakeel Butt <shakeel.butt@...ux.dev> wrote:
> > 
> > Hi Muchun,
> > 
> > On Wed, Aug 28, 2024 at 10:36:06AM GMT, Muchun Song wrote:
> >> 
> >> 
> >>> On Aug 28, 2024, at 01:23, Shakeel Butt <shakeel.butt@...ux.dev> wrote:
> >>> 
> > [...]
> >>>> 
> >>>> Does it handle the case of a too-big-to-be-a-slab-object allocation?
> >>>> I think it's better to handle it properly. Also, why return false here?
> >>>> 
> >>> 
> >>> Yes I will fix the too-big-to-be-a-slab-object allocations. I presume I
> >>> should just follow the kfree() hanlding on !folio_test_slab() i.e. that
> >>> the given object is the large or too-big-to-be-a-slab-object.
> >> 
> >> Hi Shakeel,
> >> 
> >> If we decide to do this, I suppose you will use memcg_kmem_charge_page
> >> to charge big-object. To be consistent, I suggest renaming kmem_cache_charge
> >> to memcg_kmem_charge to handle both slab object and big-object. And I saw
> >> all the functions related to object charging is moved to memcontrol.c (e.g.
> >> __memcg_slab_post_alloc_hook), so maybe we should also do this for
> >> memcg_kmem_charge?
> >> 
> > 
> > If I understand you correctly, you are suggesting to handle the general
> > kmem charging and slab's large kmalloc (size > KMALLOC_MAX_CACHE_SIZE)
> > together with memcg_kmem_charge(). However that is not possible due to
> > slab path updating NR_SLAB_UNRECLAIMABLE_B stats while no updates for
> > this stat in the general kmem charging path (__memcg_kmem_charge_page in
> > page allocation code path).
> > 
> > Also this general kmem charging path is used by many other users like
> > vmalloc, kernel stack and thus we can not just plainly stuck updates to
> > NR_SLAB_UNRECLAIMABLE_B in that path.
> 
> Sorry, maybe I am not clear . To make sure we are on the same page, let
> me clarify my thought. In your v2, I thought if we can rename
> kmem_cache_charge() to memcg_kmem_charge() since kmem_cache_charge()
> already has handled both big-slab-object (size > KMALLOC_MAX_CACHE_SIZE)
> and small-slab-object cases. You know, we have a function of
> memcg_kmem_charge_page() which could be used for charging big-slab-object
> but not small-slab-object. So I thought maybe memcg_kmem_charge() is a
> good name for it to handle both cases. And if we do this, how about moving
> this new function to memcontrol.c since all memcg charging functions are
> moved to memcontrol.c instead of slub.c.
> 

Oh you want the core function to be in memcontrol.c. I don't have any
strong opinion where the code should exist but I do want the interface
to still be kmem_cache_charge() because that is what we are providing to
the users which charging slab objects. Yes some of those might be
big-slab-objects but that is transparent to the users.

Anyways, for now I will go with my current approach but on the followup
will explore and discuss with you on which code should exist in which
file. I hope that is acceptable to you.

thanks,
Shakeel