On Fri, Sep 12, 2025 at 5:33 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > On Fri, Sep 12, 2025 at 05:07:59PM -0700, Alexei Starovoitov wrote: > > On Fri, Sep 12, 2025 at 5:02 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > > > > > On Fri, Sep 12, 2025 at 02:59:08PM -0700, Alexei Starovoitov wrote: > > > > On Fri, Sep 12, 2025 at 2:44 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > > > > > > > > > On Fri, Sep 12, 2025 at 02:31:47PM -0700, Alexei Starovoitov wrote: > > > > > > On Fri, Sep 12, 2025 at 2:29 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > > > > > > > > > > > > > On Fri, Sep 12, 2025 at 02:24:26PM -0700, Alexei Starovoitov wrote: > > > > > > > > On Fri, Sep 12, 2025 at 2:03 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > On Fri, Sep 12, 2025 at 12:27 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > +Suren, Roman > > > > > > > > > > > > > > > > > > > > On Mon, Sep 08, 2025 at 06:00:06PM -0700, Alexei Starovoitov wrote: > > > > > > > > > > > From: Alexei Starovoitov <ast@xxxxxxxxxx> > > > > > > > > > > > > > > > > > > > > > > Since the combination of valid upper bits in slab->obj_exts with > > > > > > > > > > > OBJEXTS_ALLOC_FAIL bit can never happen, > > > > > > > > > > > use OBJEXTS_ALLOC_FAIL == (1ull << 0) as a magic sentinel > > > > > > > > > > > instead of (1ull << 2) to free up bit 2. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx> > > > > > > > > > > > > > > > > > > > > Are we low on bits that we need to do this or is this good to have > > > > > > > > > > optimization but not required? > > > > > > > > > > > > > > > > > > That's a good question. After this change MEMCG_DATA_OBJEXTS and > > > > > > > > > OBJEXTS_ALLOC_FAIL will have the same value and they are used with the > > > > > > > > > same field (page->memcg_data and slab->obj_exts are aliases). Even if > > > > > > > > > page_memcg_data_flags can never be used for slab pages I think > > > > > > > > > overlapping these bits is not a good idea and creates additional > > > > > > > > > risks. Unless there is a good reason to do this I would advise against > > > > > > > > > it. > > > > > > > > > > > > > > > > Completely disagree. You both missed the long discussion > > > > > > > > during v4. The other alternative was to increase alignment > > > > > > > > and waste memory. Saving the bit is obviously cleaner. > > > > > > > > The next patch is using the saved bit. > > > > > > > > > > > > > > I will check out that discussion and it would be good to summarize that > > > > > > > in the commit message. > > > > > > > > > > > > Disgaree. It's not a job of a small commit to summarize all options > > > > > > that were discussed on the list. That's what the cover letter is for > > > > > > and there there are links to all previous threads. > > > > > > > > > > Currently the commit message is only telling what the patch is doing and > > > > > is missing the 'why' part and I think adding the 'why' part would make it > > > > > better for future readers i.e. less effort to find why this is being > > > > > done this way. (Anyways this is just a nit from me) > > > > > > > > I think 'why' here is obvious. Free the bit to use it later. > > > > From time to time people add a sentence like > > > > "this bit will be used in the next patch", > > > > but I never do this and sometimes remove it from other people's > > > > commits, since "in the next patch" is plenty ambiguous and not helpful. > > > > > > Yes, the part about the freed bit being used in later patch was clear. > > > The part about if we really need it was not obvious and if I understand > > > the discussion at [1] (relevant text below), it was not required but > > > good to have. > > > ``` > > > > I was going to say "add a new flag to enum objext_flags", > > > > but all lower 3 bits of slab->obj_exts pointer are already in use? oh... > > > > > > > > Maybe need a magic trick to add one more flag, > > > > like always align the size with 16? > > > > > > > > In practice that should not lead to increase in memory consumption > > > > anyway because most of the kmalloc-* sizes are already at least > > > > 16 bytes aligned. > > > > > > Yes. That's an option, but I think we can do better. > > > OBJEXTS_ALLOC_FAIL doesn't need to consume the bit. > > > ``` > > > > > > Anyways no objection from me but Harry had a followup request [2]: > > > ``` > > > This will work, but it would be helpful to add a comment clarifying that > > > when bit 0 is set with valid upper bits, it indicates > > > MEMCG_DATA_OBJEXTS, but when the upper bits are all zero, it indicates > > > OBJEXTS_ALLOC_FAIL. > > > > > > When someone looks at the code without checking history it might not > > > be obvious at first glance. > > > ``` > > > > > > I think the above requested comment would be really useful. > > > > ... and that comment was added. pretty much verbatim copy paste > > of the above. Don't you see it in the patch? > > Haha it seems I am blind, yup it is there. > > > > > > Suren is > > > fixing the condition of VM_BUG_ON_PAGE() in slab_obj_exts(). With this > > > patch, I think, that condition will need to be changed again. > > > > That's orthogonal and I'm not convinced it's correct. > > slab_obj_exts() is doing the right thing. afaict. > > Currently we have > > VM_BUG_ON_PAGE(obj_exts && !(obj_exts & MEMCG_DATA_OBJEXTS)) > > but it should be (before your patch) something like: > > VM_BUG_ON_PAGE(obj_exts && !(obj_exts & (MEMCG_DATA_OBJEXTS | OBJEXTS_ALLOC_FAIL))) > > After your patch, hmmm, the previous one would be right again and the > newer one will be the same as the previous due to aliasing. This patch > doesn't need to touch that VM_BUG. Older kernels will need to move to > the second condition though. Correct. Currently slab_obj_exts() will issue a warning when (obj_exts == OBJEXTS_ALLOC_FAIL), which is a perfectly valid state indicating that previous allocation of the vector failed due to memory exhaustion. Changing that warning to: VM_BUG_ON_PAGE(obj_exts && !(obj_exts & (MEMCG_DATA_OBJEXTS | OBJEXTS_ALLOC_FAIL))) will correctly avoid this warning and after your change will still work. (MEMCG_DATA_OBJEXTS | OBJEXTS_ALLOC_FAIL) when (MEMCG_DATA_OBJEXTS == OBJEXTS_ALLOC_FAIL) is technically unnecessary but is good for documenting the conditions we are checking.
