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

On Fri, Sep 12, 2025 at 05:07:59PM -0700, Alexei Starovoitov wrote:

On Fri, Sep 12, 2025 at 5:02 PM Shakeel Butt [off-list ref] wrote:

quoted

On Fri, Sep 12, 2025 at 02:59:08PM -0700, Alexei Starovoitov wrote:

quoted

On Fri, Sep 12, 2025 at 2:44 PM Shakeel Butt [off-list ref] wrote:

quoted

On Fri, Sep 12, 2025 at 02:31:47PM -0700, Alexei Starovoitov wrote:

quoted

On Fri, Sep 12, 2025 at 2:29 PM Shakeel Butt [off-list ref] wrote:

quoted

On Fri, Sep 12, 2025 at 02:24:26PM -0700, Alexei Starovoitov wrote:

quoted

On Fri, Sep 12, 2025 at 2:03 PM Suren Baghdasaryan [off-list ref] wrote:

quoted

On Fri, Sep 12, 2025 at 12:27 PM Shakeel Butt [off-list ref] wrote:

quoted

+Suren, Roman

On Mon, Sep 08, 2025 at 06:00:06PM -0700, Alexei Starovoitov wrote:

quoted

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.

quoted

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.