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.
