Re: [PATCH] mm: memcg/slab: fix obj_cgroup_charge() return value handling

[Shakeel Butt <shakeelb@google.com>]

On Thu, Nov 26, 2020 at 8:14 PM Roman Gushchin <guro@fb.com> wrote:
>
> Commit 10befea91b61 ("mm: memcg/slab: use a single set of kmem_caches
> for all allocations") introduced a regression into the handling of the
> obj_cgroup_charge() return value. If a non-zero value is returned
> (indicating of exceeding one of memory.max limits), the allocation
> should fail, instead of falling back to non-accounted mode.
>
> To make the code more readable, move memcg_slab_pre_alloc_hook()
> and memcg_slab_post_alloc_hook() calling conditions into bodies
> of these hooks.
>
> Fixes: 10befea91b61 ("mm: memcg/slab: use a single set of kmem_caches for all allocations")
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: stable@vger.kernel.org
> ---
>  mm/slab.h | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/mm/slab.h b/mm/slab.h
> index 59aeb0d9f11b..5dc89d8fb05e 100644
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -257,22 +257,32 @@ static inline size_t obj_full_size(struct kmem_cache *s)
>         return s->size + sizeof(struct obj_cgroup *);
>  }
>
> -static inline struct obj_cgroup *memcg_slab_pre_alloc_hook(struct kmem_cache *s,
> -                                                          size_t objects,
> -                                                          gfp_t flags)
> +/*
> + * Returns true if the allocation should fail.

IMO returning false if the allocation should fail makes this more
clear. Otherwise the patch looks good to me.

> + */
> +static inline bool memcg_slab_pre_alloc_hook(struct kmem_cache *s,
> +                                            struct obj_cgroup **objcgp,
> +                                            size_t objects, gfp_t flags)
>  {
>         struct obj_cgroup *objcg;
>
> +       if (!memcg_kmem_enabled())
> +               return false;
> +
> +       if (!(flags & __GFP_ACCOUNT) && !(s->flags & SLAB_ACCOUNT))
> +               return false;
> +
>         objcg = get_obj_cgroup_from_current();
>         if (!objcg)
> -               return NULL;
> +               return false;
>
>         if (obj_cgroup_charge(objcg, flags, objects * obj_full_size(s))) {
>                 obj_cgroup_put(objcg);
> -               return NULL;
> +               return true;
>         }
>
> -       return objcg;
> +       *objcgp = objcg;
> +       return false;
>  }