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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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?

> 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.