Re: [PATCH slab v5 5/6] slab: Reuse first bit for OBJEXTS_ALLOC_FAIL

[Shakeel Butt <shakeel.butt@linux.dev>]

On Fri, Sep 12, 2025 at 02:03:03PM -0700, Suren Baghdasaryan wrote:
[...]
> > I do have some questions on the state of slab->obj_exts even before this
> > patch for Suren, Roman, Vlastimil and others:
> >
> > Suppose we newly allocate struct slab for a SLAB_ACCOUNT cache and tried
> > to allocate obj_exts for it which failed. The kernel will set
> > OBJEXTS_ALLOC_FAIL in slab->obj_exts (Note that this can only be set for
> > new slab allocation and only for SLAB_ACCOUNT caches i.e. vec allocation
> > failure for memory profiling does not set this flag).
> >
> > Now in the post alloc hook, either through memory profiling or through
> > memcg charging, we will try again to allocate the vec and before that we
> > will call slab_obj_exts() on the slab which has:
> >
> >         unsigned long obj_exts = READ_ONCE(slab->obj_exts);
> >
> >         VM_BUG_ON_PAGE(obj_exts && !(obj_exts & MEMCG_DATA_OBJEXTS), slab_page(slab));
> >
> > It seems like the above VM_BUG_ON_PAGE() will trigger because obj_exts
> > will have OBJEXTS_ALLOC_FAIL but it should not, right? Or am I missing
> > something? After the following patch we will aliasing be MEMCG_DATA_OBJEXTS
> > and OBJEXTS_ALLOC_FAIL and will avoid this trigger though which also
> > seems unintended.
> 
> You are correct. Current VM_BUG_ON_PAGE() will trigger if
> OBJEXTS_ALLOC_FAIL is set and that is wrong. When
> alloc_slab_obj_exts() fails to allocate the vector it does
> mark_failed_objexts_alloc() and exits without setting
> MEMCG_DATA_OBJEXTS (which it would have done if the allocation
> succeeded). So, any further calls to slab_obj_exts() will generate a
> warning because MEMCG_DATA_OBJEXTS is not set. I believe the proper
> fix would not be to set MEMCG_DATA_OBJEXTS along with
> OBJEXTS_ALLOC_FAIL because the pointer does not point to a valid
> vector but to modify the warning to:
> 
> VM_BUG_ON_PAGE(obj_exts && !(obj_exts & (MEMCG_DATA_OBJEXTS |
> OBJEXTS_ALLOC_FAIL)), slab_page(slab));
> 
> IOW, we expect the obj_ext to be either NULL or have either
> MEMCG_DATA_OBJEXTS or OBJEXTS_ALLOC_FAIL set.
> >
> > Next question: OBJEXTS_ALLOC_FAIL is for memory profiling and we never
> > set it when memcg is disabled and memory profiling is enabled or even
> > with both memcg and memory profiling are enabled but cache does not have
> > SLAB_ACCOUNT. This seems unintentional as well, right?
> 
> I'm not sure why you think OBJEXTS_ALLOC_FAIL is not set by memory
> profiling (independent of CONFIG_MEMCG state).
> __alloc_tagging_slab_alloc_hook()->prepare_slab_obj_exts_hook()->alloc_slab_obj_exts()
> will attempt to allocate the vector and set OBJEXTS_ALLOC_FAIL if that
> fails.
> 

prepare_slab_obj_exts_hook() calls alloc_slab_obj_exts() with new_slab
as false and alloc_slab_obj_exts() will only call
mark_failed_objexts_alloc() if new_slab is true.

> >
> > Also I think slab_obj_exts() needs to handle OBJEXTS_ALLOC_FAIL explicitly.
> 
> Agree, so is my proposal to update the warning sounds right to you?

Yes that seems right to me.