On Aug 29, 2024, at 23:49, Shakeel Butt [off-list ref] wrote:

On Thu, Aug 29, 2024 at 10:36:01AM GMT, Muchun Song wrote:

quoted

quoted

On Aug 29, 2024, at 03:03, Shakeel Butt [off-list ref] wrote:

Hi Muchun,

On Wed, Aug 28, 2024 at 10:36:06AM GMT, Muchun Song wrote:

quoted

quoted

On Aug 28, 2024, at 01:23, Shakeel Butt [off-list ref] wrote:

[...]

quoted

quoted

quoted

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.

Fine. No problem.

Thanks.

thanks,
Shakeel