On 2023/12/6 03:57, Vlastimil Babka wrote:
> On 12/5/23 09:19, Chengming Zhou wrote:
>> On 2023/12/5 03:34, Vlastimil Babka wrote:
>>> Currently, when __kmem_cache_alloc_bulk() fails, it frees back the
>>> objects that were allocated before the failure, using
>>> kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free
>>> hooks (KASAN etc.) and those expect objects that were processed by the
>>> post alloc hooks, slab_post_alloc_hook() is called before
>>> kmem_cache_free_bulk().
>>>
>>> This is wasteful, although not a big concern in practice for the rare
>>> error path. But in order to efficiently handle percpu array batch refill
>>> and free in the near future, we will also need a variant of
>>> kmem_cache_free_bulk() that avoids the free hooks. So introduce it now
>>> and use it for the failure path.
>>>
>>> As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg
>>> parameter, remove it.
>>
>> The objects may have been charged before, but it seems __kmem_cache_alloc_bulk()
>> forget to uncharge them? I can't find "uncharge" in do_slab_free(), or maybe
>> the bulk interface won't be used on chargeable slab?
>
> You're right! I missed that the memcg_pre_alloc_hook() already does the
> charging, so we need to uncharge. How does this look? Thanks for noticing!
>
> ----8<----
> From 52f8e77fdfeabffffdce6b761ba5508e940df3be Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@xxxxxxx>
> Date: Thu, 2 Nov 2023 16:34:39 +0100
> Subject: [PATCH 2/4] mm/slub: introduce __kmem_cache_free_bulk() without free
> hooks
>
> Currently, when __kmem_cache_alloc_bulk() fails, it frees back the
> objects that were allocated before the failure, using
> kmem_cache_free_bulk(). Because kmem_cache_free_bulk() calls the free
> hooks (KASAN etc.) and those expect objects that were processed by the
> post alloc hooks, slab_post_alloc_hook() is called before
> kmem_cache_free_bulk().
>
> This is wasteful, although not a big concern in practice for the rare
> error path. But in order to efficiently handle percpu array batch refill
> and free in the near future, we will also need a variant of
> kmem_cache_free_bulk() that avoids the free hooks. So introduce it now
> and use it for the failure path.
>
> In case of failure we however still need to perform memcg uncharge so
> handle that in a new memcg_slab_alloc_error_hook(). Thanks to Chengming
> Zhou for noticing the missing uncharge.
>
> As a consequence, __kmem_cache_alloc_bulk() no longer needs the objcg
> parameter, remove it.
>
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
Looks good to me!
Reviewed-by: Chengming Zhou <zhouchengming@xxxxxxxxxxxxx>
Thanks!
> ---
> mm/slub.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index d7b0ca6012e0..0a9e4bd0dd68 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2003,6 +2003,14 @@ void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab, void **p,
>
> __memcg_slab_free_hook(s, slab, p, objects, objcgs);
> }
> +
> +static inline
> +void memcg_slab_alloc_error_hook(struct kmem_cache *s, int objects,
> + struct obj_cgroup *objcg)
> +{
> + if (objcg)
> + obj_cgroup_uncharge(objcg, objects * obj_full_size(s));
> +}
> #else /* CONFIG_MEMCG_KMEM */
> static inline struct mem_cgroup *memcg_from_slab_obj(void *ptr)
> {
> @@ -2032,6 +2040,12 @@ static inline void memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
> void **p, int objects)
> {
> }
> +
> +static inline
> +void memcg_slab_alloc_error_hook(struct kmem_cache *s, int objects,
> + struct obj_cgroup *objcg)
> +{
> +}
> #endif /* CONFIG_MEMCG_KMEM */
>
> /*
> @@ -4478,6 +4492,27 @@ int build_detached_freelist(struct kmem_cache *s, size_t size,
> return same;
> }
>
> +/*
> + * Internal bulk free of objects that were not initialised by the post alloc
> + * hooks and thus should not be processed by the free hooks
> + */
> +static void __kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)
> +{
> + if (!size)
> + return;
> +
> + do {
> + struct detached_freelist df;
> +
> + size = build_detached_freelist(s, size, p, &df);
> + if (!df.slab)
> + continue;
> +
> + do_slab_free(df.s, df.slab, df.freelist, df.tail, df.cnt,
> + _RET_IP_);
> + } while (likely(size));
> +}
> +
> /* Note that interrupts must be enabled when calling this function. */
> void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)
> {
> @@ -4498,8 +4533,9 @@ void kmem_cache_free_bulk(struct kmem_cache *s, size_t size, void **p)
> EXPORT_SYMBOL(kmem_cache_free_bulk);
>
> #ifndef CONFIG_SLUB_TINY
> -static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags,
> - size_t size, void **p, struct obj_cgroup *objcg)
> +static inline
> +int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> + void **p)
> {
> struct kmem_cache_cpu *c;
> unsigned long irqflags;
> @@ -4563,14 +4599,13 @@ static inline int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags,
>
> error:
> slub_put_cpu_ptr(s->cpu_slab);
> - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size);
> - kmem_cache_free_bulk(s, i, p);
> + __kmem_cache_free_bulk(s, i, p);
> return 0;
>
> }
> #else /* CONFIG_SLUB_TINY */
> static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags,
> - size_t size, void **p, struct obj_cgroup *objcg)
> + size_t size, void **p)
> {
> int i;
>
> @@ -4593,8 +4628,7 @@ static int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags,
> return i;
>
> error:
> - slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size);
> - kmem_cache_free_bulk(s, i, p);
> + __kmem_cache_free_bulk(s, i, p);
> return 0;
> }
> #endif /* CONFIG_SLUB_TINY */
> @@ -4614,15 +4648,19 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> if (unlikely(!s))
> return 0;
>
> - i = __kmem_cache_alloc_bulk(s, flags, size, p, objcg);
> + i = __kmem_cache_alloc_bulk(s, flags, size, p);
>
> /*
> * memcg and kmem_cache debug support and memory initialization.
> * Done outside of the IRQ disabled fastpath loop.
> */
> - if (i != 0)
> + if (likely(i != 0)) {
> slab_post_alloc_hook(s, objcg, flags, size, p,
> slab_want_init_on_alloc(flags, s), s->object_size);
> + } else {
> + memcg_slab_alloc_error_hook(s, size, objcg);
> + }
> +
> return i;
> }
> EXPORT_SYMBOL(kmem_cache_alloc_bulk);
