lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PH0PR11MB51928A8212F2EE25916524A3EC89A@PH0PR11MB5192.namprd11.prod.outlook.com>
Date:   Sat, 9 Dec 2023 01:02:38 +0000
From:   "Song, Xiongwei" <Xiongwei.Song@...driver.com>
To:     Vlastimil Babka <vbabka@...e.cz>,
        "sxwjean@...com" <sxwjean@...com>,
        "42.hyeyoo@...il.com" <42.hyeyoo@...il.com>,
        "cl@...ux.com" <cl@...ux.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
CC:     "penberg@...nel.org" <penberg@...nel.org>,
        "rientjes@...gle.com" <rientjes@...gle.com>,
        "iamjoonsoo.kim@....com" <iamjoonsoo.kim@....com>,
        "roman.gushchin@...ux.dev" <roman.gushchin@...ux.dev>,
        "corbet@....net" <corbet@....net>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH v2 2/3] mm/slub: unify all sl[au]b parameters with
 "slab_$param"



> -----Original Message-----
> From: Vlastimil Babka <vbabka@...e.cz>
> Sent: Thursday, December 7, 2023 12:15 AM
> To: sxwjean@...com; 42.hyeyoo@...il.com; cl@...ux.com; linux-mm@...ck.org
> Cc: penberg@...nel.org; rientjes@...gle.com; iamjoonsoo.kim@....com;
> roman.gushchin@...ux.dev; corbet@....net; keescook@...omium.org; arnd@...db.de;
> akpm@...ux-foundation.org; gregkh@...uxfoundation.org; linux-doc@...r.kernel.org; linux-
> kernel@...r.kernel.org; Song, Xiongwei <Xiongwei.Song@...driver.com>
> 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@...com wrote:
> > From: Xiongwei Song <xiongwei.song@...driver.com>
> >
> > 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@suse.cz/
> >
> > Signed-off-by: Xiongwei Song <xiongwei.song@...driver.com>
> > ---
> >  .../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!

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ