Re: [PATCH v2] mm/slab: simplify SLAB_* flag handling

["Harry (Hyeonggon) Yoo" <42.hyeyoo@gmail.com>]

On Wed, Feb 05, 2025 at 11:38:51AM +0100, Vlastimil Babka wrote:
> On 2/2/25 07:57, Hyeonggon Yoo wrote:
> > On Sat, Jan 25, 2025 at 1:49 AM Kevin Brodsky <kevin.brodsky@arm.com> wrote:
> >>
> >> SLUB is the only remaining allocator. We can therefore get rid of
> >> the logic for allocator-specific flags:
> >>
> >> * Merge SLAB_CACHE_FLAGS into SLAB_CORE_FLAGS.
> >>
> >> * Remove CACHE_CREATE_MASK and instead mask out SLAB_DEBUG_FLAGS if
> >>   !CONFIG_SLUB_DEBUG. SLAB_DEBUG_FLAGS is now defined
> >>   unconditionally (no impact on existing code, which ignores it if
> >>   !CONFIG_SLUB_DEBUG).
> >>
> >> * Define SLAB_FLAGS_PERMITTED in terms of SLAB_CORE_FLAGS and
> >>   SLAB_DEBUG_FLAGS (no functional change).
> >>
> >> While at it also remove misleading comments that suggest that
> >> multiple allocators are available.
> >>
> >> Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>

Apologizes for the noise. My previous feedback was deemed valid to me
at the time of writing, but it's incorrect and doesn't add much value
in improving the patch.

FWIW,
Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

> > Hi Kevin,
> > This patch in general looks fine to me.
> > 
> > However, there are subtle changes that were not documented by the changelog.
> > SLUB currently does not _completely_ ignore debug flags even when
> > CONFIG_SLUB_DEBUG=n. For example, calculate_sizes() relocate the free pointer
> > regardless of CONFIG_SLUB_DEBUG.
> 
> I think the patch makes no difference there effectively? Before the patch,
> CACHE_CREATE_MASK would be used to filter flags and not include the debug
> flags, so that free pointer relocation due to SLAB_POISON or SLAB_RED_ZONE
> would not happen. After the patch, it's filtered effectively the same? Am I
> missing something?

You're not missing anything, it just slipped my mind :/

> > I believe completely ignoring debug flags should be acceptable
> > when CONFIG_SLUB_DEBUG=n, but this change should at least be documented
> > in the changelog.
> > 
> > Additionally, beyond what this patch currently does, we can remove several
> > CONFIG_SLUB_DEBUG #ifdefs in some functions (e.g., in calculate_sizes())
> > on top of this patch.
> 
> So AFAIK that should be possible both before and after this patch

Right.

> and you can try (it's out of scope of this patch).

Yes. it is out of scope.

> But I suspect the reason for the #ifdefs is that it the code would reference
> structures or fields that don't exist without CONFIG_SLUB_DEBUG.
> Maybe that's not (no longer?) true in some cases and we could clean up.

Yes in most of cases it's it won't build without #ifdefs...
There are only few cases we could clean up.

> Another reason would be performance, but we shouldn't care in code that's
> only executed during cache creation.

Or perhaps due to a slight increase in code size, but I doubt the
difference in code size will be significant.

-- 
Harry