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