On Fri, Sep 12, 2025 at 2:12 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > 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. Sorry, I missed that detail. I think mark_failed_objexts_alloc() should be called there unconditionally if vector allocation fails. > > > > > > > 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.
