RE: [RFC PATCH v2 2/3] mm/slub: unify all sl[au]b parameters with "slab_$param"

[Song, Xiongwei]

> -----Original Message-----
> From: Vlastimil Babka <vbabka@xxxxxxx>
> Sent: Thursday, December 7, 2023 12:15 AM
> To: sxwjean@xxxxxx; 42.hyeyoo@xxxxxxxxx; cl@xxxxxxxxx; linux-mm@xxxxxxxxx
> Cc: penberg@xxxxxxxxxx; rientjes@xxxxxxxxxx; iamjoonsoo.kim@xxxxxxx;
> roman.gushchin@xxxxxxxxx; corbet@xxxxxxx; keescook@xxxxxxxxxxxx; arnd@xxxxxxxx;
> akpm@xxxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Song, Xiongwei <Xiongwei.Song@xxxxxxxxxxxxx>
> Subject: Re: [RFC PATCH v2 2/3] mm/slub: unify all sl[au]b parameters with "slab_$param"
> 
> 
> On 12/3/23 01:15, sxwjean@xxxxxx wrote:
> > From: Xiongwei Song <xiongwei.song@xxxxxxxxxxxxx>
> >
> > Since the SLAB allocator has been removed, so we need to clean up the
> 
> "we can clean up", as we don't really "need"
> 
> > sl[au]b_$params. However, the "slab/SLAB" terms should be keep for
> > long-term rather than "slub/SLUB". Hence, we should use "slab_$param"
> 
> I'd phrase it: With only one slab allocator left, it's better to use the
> generic "slab" term instead of "slub" which is an implementation detail.
> Hence ...
> 
> > as the primary prefix, which is pointed out by Vlastimil Babka. For more
> > information please see [1].
> >
> > This patch is changing the following slab parameters
> > - slub_max_order
> > - slub_min_order
> > - slub_min_objects
> > - slub_debug
> > to
> > - slab_max_order
> > - slab_min_order
> > - slab_min_objects
> > - slab_debug
> > as the primary slab parameters in
> > Documentation/admin-guide/kernel-parameters.txt and source, and rename all
> > setup functions of them too. Meanwhile, "slub_$params" can also be passed
> 
> Not sure about renaming the code at this point, I would just rename the
> user-visible parameters and their documentation and any comment that refers
> to the parameters. Functions and variables can come later as part of wider
> slub/slab change if we decide to do so?

I think we can rename these global variables: 
    slub_max_order,
    slub_min_order,
    slub_min_objects,
    slub_debug 
, which are used to save values that are from parameters. Because some comments
are referring to parameters, the others are referring to these global variables, which
looks inconsistent, e.g. slub_debug/slab_debug.  Is it acceptable to make them
consistent?

Regards,
Xiongwei.
> 
> > by command line, which is to keep backward compatibility. Also mark all
> > "slub_$params" as legacy.
> >
> > The function
> >     static int __init setup_slub_debug(char *str);
> > , which is to setup debug flags inside a slab during kernel init, is
> > changed to setup_slab_debug_flags(), which is to prevent the name
> > conflict. Because there is another function
> >     void setup_slab_debug(struct kmem_cache *s, struct slab *slab,
> >               void *addr);
> > , which is to poison slab space, would have name conflict with the prior
> > one.
> 
> Another reason to defer code naming changes.
> 
> > For parameter "slub_debug", beside replacing it with "slab_debug", there
> > are several global variables, local variables and functions which are
> > related with the parameter, let's rename them all.
> >
> > Remove the separate descriptions for slub_[no]merge, append legacy tip
> > for them at the end of descriptions of slab_[no]merge.
> >
> > I didn't change the parameters in Documentation/mm/slub.rst because the
> > file name is still "slub.rst", and slub_$params still can be used in
> > kernel command line to keep backward compatibility.
> >
> > [1] https://lore.kernel.org/linux-mm/7512b350-4317-21a0-fab3-4101bc4d8f7a@xxxxxxx/
> >
> > Signed-off-by: Xiongwei Song <xiongwei.song@xxxxxxxxxxxxx>
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  44 +++---
> >  drivers/misc/lkdtm/heap.c                     |   2 +-
> >  mm/Kconfig.debug                              |   6 +-
> >  mm/slab.h                                     |  16 +-
> >  mm/slab_common.c                              |   8 +-
> >  mm/slub.c                                     | 142 +++++++++---------
> >  6 files changed, 109 insertions(+), 109 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-
> guide/kernel-parameters.txt
> > index 9f94baeb2f82..d01c12e2a247 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5869,6 +5869,8 @@
> >       slab_merge      [MM]
> >                       Enable merging of slabs with similar size when the
> >                       kernel is built without CONFIG_SLAB_MERGE_DEFAULT.
> > +                     (slub_merge is accepted too, but it's supported for
> > +                     legacy)
> 
> How about a shorter note (and always start on new line)
> 
>                 (slub_merge legacy name also accepted for now)
> 
> >
> >       slab_nomerge    [MM]
> >                       Disable merging of slabs with similar size. May be
> > @@ -5882,47 +5884,41 @@
> >                       unchanged). Debug options disable merging on their
> >                       own.
> >                       For more information see Documentation/mm/slub.rst.
> > +                     (slub_nomerge is accepted too, but it's supported for
> > +                     legacy)
> >
> > -     slab_max_order= [MM, SLAB]
> > -                     Determines the maximum allowed order for slabs.
> > -                     A high setting may cause OOMs due to memory
> > -                     fragmentation.  Defaults to 1 for systems with
> > -                     more than 32MB of RAM, 0 otherwise.
> > -
> > -     slub_debug[=options[,slabs][;[options[,slabs]]...]      [MM, SLUB]
> > -                     Enabling slub_debug allows one to determine the
> > +     slab_debug[=options[,slabs][;[options[,slabs]]...]      [MM]
> 
> I think we should re-sort alphabetically after the slub_ -> slab_ change.
> 
> > +                     Enabling slab_debug allows one to determine the
> >                       culprit if slab objects become corrupted. Enabling
> > -                     slub_debug can create guard zones around objects and
> > +                     slab_debug can create guard zones around objects and
> >                       may poison objects when not in use. Also tracks the
> >                       last alloc / free. For more information see
> > -                     Documentation/mm/slub.rst.
> > +                     Documentation/mm/slub.rst. (slub_debug is accepted
> > +                     too, but it's supported for legacy)
> >
> > -     slub_max_order= [MM, SLUB]
> > +     slab_max_order= [MM]
> >                       Determines the maximum allowed order for slabs.
> >                       A high setting may cause OOMs due to memory
> >                       fragmentation. For more information see
> > -                     Documentation/mm/slub.rst.
> > +                     Documentation/mm/slub.rst. (slub_max_order is
> > +                     accepted too, but it's supported for legacy)
> >
> > -     slub_min_objects=       [MM, SLUB]
> > +     slab_min_objects=       [MM]
> >                       The minimum number of objects per slab. SLUB will
> > -                     increase the slab order up to slub_max_order to
> > +                     increase the slab order up to slab_max_order to
> >                       generate a sufficiently large slab able to contain
> >                       the number of objects indicated. The higher the number
> >                       of objects the smaller the overhead of tracking slabs
> >                       and the less frequently locks need to be acquired.
> >                       For more information see Documentation/mm/slub.rst.
> > +                     (slub_min_objects is accepted too, but it's supported
> > +                     for legacy)
> >
> > -     slub_min_order= [MM, SLUB]
> > +     slab_min_order= [MM]
> >                       Determines the minimum page order for slabs. Must be
> > -                     lower than slub_max_order.
> > -                     For more information see Documentation/mm/slub.rst.
> > -
> > -     slub_merge      [MM, SLUB]
> > -                     Same with slab_merge.
> > -
> > -     slub_nomerge    [MM, SLUB]
> > -                     Same with slab_nomerge. This is supported for legacy.
> > -                     See slab_nomerge for more information.
> > +                     lower than slab_max_order. For more information see
> 
>                 "lower or equal to" (more precise, while at it)
> 
> > +                     Documentation/mm/slub.rst. (slub_min_order is accepted
> > +                     too, but it's supported for legacy)
> >
> >       smart2=         [HW]
> >                       Format: <io1>[,<io2>[,...,<io8>]]
> 
> Thanks!