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 05:07:59PM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 12, 2025 at 5:02 PM Shakeel Butt <shakeel.butt@linux.dev> 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@linux.dev> 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@linux.dev> 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@google.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Sep 12, 2025 at 12:27 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
> > > > > > > > >
> > > > > > > > > +Suren, Roman
> > > > > > > > >
> > > > > > > > > On Mon, Sep 08, 2025 at 06:00:06PM -0700, Alexei Starovoitov wrote:
> > > > > > > > > > From: Alexei Starovoitov <ast@kernel.org>
> > > > > > > > > >
> > > > > > > > > > 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@kernel.org>
> > > > > > > > >
> > > > > > > > > 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.