On Thu, Nov 26, 2020 at 8:14 PM Roman Gushchin <guro@xxxxxx> 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@xxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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;
> }
