Re: [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers

From: Joel Granados
Date: Tue Oct 03 2023 - 04:45:48 EST

Next message: Ingo Molnar: "Re: [PATCH 6/6] selftests: futex: remove duplicate unneeded defines"
Previous message: Hengqi Chen: "[RFC PATCH 2/2] seccomp: Introduce SECCOMP_ATTACH_FILTER operation"
In reply to: Christophe Leroy: "Re: [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers"
Next in thread: David Ahern: "Re: [PATCH v2 10/15] vrf: Remove the now superfluous sentinel element from ctl_table array"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Oct 02, 2023 at 12:27:18PM +0000, Christophe Leroy wrote:
>
>
> Le 02/10/2023 à 10:55, Joel Granados via B4 Relay a écrit :
> > From: Joel Granados <j.granados@xxxxxxxxxxx>
> >
<--- snip --->
> > - The "yesall" config saves 2432 bytes [4]
> > - The "tiny" config saves 64 bytes [5]
> > * memory usage:
> > In this case there were no bytes saved because I do not have any
> > of the drivers in the patch. To measure it comment the printk in
> > `new_dir` and uncomment the if conditional in `new_links` [3].
> >
> > ---
> > Changes in v2:
> > - Left the dangling comma in the ctl_table arrays.
> > - Link to v1: https://lore.kernel.org/r/20230928-jag-sysctl_remove_empty_elem_drivers-v1-0-e59120fca9f9@xxxxxxxxxxx
> >
> > Comments/feedback greatly appreciated
>
> Same problem on powerpc CI tests, all boot target failed, most of them
> with similar OOPS, see
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20231002-jag-sysctl_remove_empty_elem_drivers-v2-15-02dd0d46f71e@xxxxxxxxxxx/
I found the culprit!. Here you are rebasing on top of v6.5.0-rc6 "INFO:
Looking for kernel version: 6.5.0-rc6-gbf2ac4d7d596". The error makes
sense becuase in that version we have not introduced the stopping
criteria based on the ctl_table array size, so the loop continues
looking for an empty sentinel past valid memory (and does not find it).
The ctl_table check catches it but then fails to do a proper error
because we have already tried to access invalid memory. The solution
here is to make sure to rebase in on top of the latest rc in v6.6.

>
> What is strange is that I pushed your series into my github account, and
> got no failure, see https://github.com/chleroy/linux/actions/runs/6378951278
And here it works because you use the latest rc : "INFO: Looking for
kernel version: 6.6.0-rc3-g23d4b5db743c"

>
> Christophe
>
> >
> > Best
> >
> > Joel
> >
> > [1]
> > We are able to remove a sentinel table without behavioral change by
> > introducing a table_size argument in the same place where procname is
> > checked for NULL. The idea is for it to keep stopping when it hits
> > ->procname == NULL, while the sentinel is still present. And when the
> > sentinel is removed, it will stop on the table_size. You can go to
> > (https://lore.kernel.org/all/20230809105006.1198165-1-j.granados@xxxxxxxxxxx/)
> > for more information.
> >
> > [2]
> > Links Related to the ctl_table sentinel removal:
> > * Good summary from Luis sent with the "pull request" for the
> > preparation patches.
> > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@xxxxxxxxxxxxxxxxxxxxxx/
> > * Another very good summary from Luis.
> > https://lore.kernel.org/all/ZMFizKFkVxUFtSqa@xxxxxxxxxxxxxxxxxxxxxx/
> > * This is a patch set that replaces register_sysctl_table with register_sysctl
> > https://lore.kernel.org/all/20230302204612.782387-1-mcgrof@xxxxxxxxxx/
> > * Patch set to deprecate register_sysctl_paths()
> > https://lore.kernel.org/all/20230302202826.776286-1-mcgrof@xxxxxxxxxx/
> > * Here there is an explicit expectation for the removal of the sentinel element.
> > https://lore.kernel.org/all/20230321130908.6972-1-frank.li@xxxxxxxx
> > * The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread
> > https://lore.kernel.org/all/20220220060626.15885-1-tangmeng@xxxxxxxxxxxxx
> >
> > [3]
> > To measure the in memory savings apply this on top of this patchset.
> >
> > "
> > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> > index c88854df0b62..e0073a627bac 100644
> > --- a/fs/proc/proc_sysctl.c
> > +++ b/fs/proc/proc_sysctl.c
> > @@ -976,6 +976,8 @@ static struct ctl_dir *new_dir(struct ctl_table_set *set,
> > table[0].procname = new_name;
> > table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO;
> > init_header(&new->header, set->dir.header.root, set, node, table, 1);
> > + // Counts additional sentinel used for each new dir.
> > + printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_table));
> >
> > return new;
> > }
> > @@ -1199,6 +1201,9 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_
> > link_name += len;
> > link++;
> > }
> > + // Counts additional sentinel used for each new registration
> > + //if ((head->ctl_table + head->ctl_table_size)->procname)
> > + printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_table));
> > init_header(links, dir->header.root, dir->header.set, node, link_table,
> > head->ctl_table_size);
> > links->nreg = nr_entries;
> > "
> > and then run the following bash script in the kernel:
> >
> > accum=0
> > for n in $(dmesg | grep kzalloc | awk '{print $3}') ; do
> > echo $n
> > accum=$(calc "$accum + $n")
> > done
> > echo $accum
> >
> > [4]
> > add/remove: 0/0 grow/shrink: 0/21 up/down: 0/-2432 (-2432)
> > Function old new delta
> > xpc_sys_xpc_hb 192 128 -64
> > xpc_sys_xpc 128 64 -64
> > vrf_table 128 64 -64
> > ucma_ctl_table 128 64 -64
> > tty_table 192 128 -64
> > sg_sysctls 128 64 -64
> > scsi_table 128 64 -64
> > random_table 448 384 -64
> > raid_table 192 128 -64
> > oa_table 192 128 -64
> > mac_hid_files 256 192 -64
> > iwcm_ctl_table 128 64 -64
> > ipmi_table 128 64 -64
> > hv_ctl_table 128 64 -64
> > hpet_table 128 64 -64
> > firmware_config_table 192 128 -64
> > cdrom_table 448 384 -64
> > balloon_table 128 64 -64
> > parport_sysctl_template 912 720 -192
> > parport_default_sysctl_table 584 136 -448
> > parport_device_sysctl_template 776 136 -640
> > Total: Before=429940038, After=429937606, chg -0.00%
> >
> > [5]
> > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-64 (-64)
> > Function old new delta
> > random_table 448 384 -64
> > Total: Before=1885527, After=1885463, chg -0.00%
> >
> > [6] https://lore.kernel.org/all/20230913-jag-sysctl_remove_empty_elem_arch-v2-0-d1bd13a29bae@xxxxxxxxxxx/
> >
> > Signed-off-by: Joel Granados <j.granados@xxxxxxxxxxx>
> >
> > To: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > To: willy@xxxxxxxxxxxxx
> > To: josh@xxxxxxxxxxxxxxxx
> > To: Kees Cook <keescook@xxxxxxxxxxxx>
> > To: Phillip Potter <phil@xxxxxxxxxxxxxxxx>
> > To: Clemens Ladisch <clemens@xxxxxxxxxx>
> > To: Arnd Bergmann <arnd@xxxxxxxx>
> > To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > To: Juergen Gross <jgross@xxxxxxxx>
> > To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > To: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > To: Jiri Slaby <jirislaby@xxxxxxxxxx>
> > To: "James E.J. Bottomley" <jejb@xxxxxxxxxxxxx>
> > To: "Martin K. Petersen" <martin.petersen@xxxxxxxxxx>
> > To: Doug Gilbert <dgilbert@xxxxxxxxxxxx>
> > To: Sudip Mukherjee <sudipm.mukherjee@xxxxxxxxx>
> > To: Jason Gunthorpe <jgg@xxxxxxxx>
> > To: Leon Romanovsky <leon@xxxxxxxxxx>
> > To: Corey Minyard <minyard@xxxxxxx>
> > To: Theodore Ts'o <tytso@xxxxxxx>
> > To: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
> > To: David Ahern <dsahern@xxxxxxxxxx>
> > To: "David S. Miller" <davem@xxxxxxxxxxxxx>
> > To: Eric Dumazet <edumazet@xxxxxxxxxx>
> > To: Jakub Kicinski <kuba@xxxxxxxxxx>
> > To: Paolo Abeni <pabeni@xxxxxxxxxx>
> > To: Robin Holt <robinmholt@xxxxxxxxx>
> > To: Steve Wahl <steve.wahl@xxxxxxx>
> > To: Russ Weight <russell.h.weight@xxxxxxxxx>
> > To: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > To: Song Liu <song@xxxxxxxxxx>
> > To: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx>
> > To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > To: Wei Liu <wei.liu@xxxxxxxxxx>
> > To: Dexuan Cui <decui@xxxxxxxxxxxxx>
> > To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > To: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > To: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > To: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > To: David Airlie <airlied@xxxxxxxxx>
> > To: Daniel Vetter <daniel@xxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: linux-serial@xxxxxxxxxxxxxxx
> > Cc: linux-scsi@xxxxxxxxxxxxxxx
> > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> > Cc: linux-rdma@xxxxxxxxxxxxxxx
> > Cc: openipmi-developer@xxxxxxxxxxxxxxxxxxxxx
> > Cc: netdev@xxxxxxxxxxxxxxx
> > Cc: linux-raid@xxxxxxxxxxxxxxx
> > Cc: linux-hyperv@xxxxxxxxxxxxxxx
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >
> > ---
> >
> > ---
> > Joel Granados (15):
> > cdrom: Remove now superfluous sentinel element from ctl_table array
> > hpet: Remove now superfluous sentinel element from ctl_table array
> > xen: Remove now superfluous sentinel element from ctl_table array
> > tty: Remove now superfluous sentinel element from ctl_table array
> > scsi: Remove now superfluous sentinel element from ctl_table array
> > parport: Remove the now superfluous sentinel element from ctl_table array
> > macintosh: Remove the now superfluous sentinel element from ctl_table array
> > infiniband: Remove the now superfluous sentinel element from ctl_table array
> > char-misc: Remove the now superfluous sentinel element from ctl_table array
> > vrf: Remove the now superfluous sentinel element from ctl_table array
> > sgi-xp: Remove the now superfluous sentinel element from ctl_table array
> > fw loader: Remove the now superfluous sentinel element from ctl_table array
> > raid: Remove now superfluous sentinel element from ctl_table array
> > Drivers: hv: Remove now superfluous sentinel element from ctl_table array
> > intel drm: Remove now superfluous sentinel element from ctl_table array
> >
> > drivers/base/firmware_loader/fallback_table.c | 1 -
> > drivers/cdrom/cdrom.c | 1 -
> > drivers/char/hpet.c | 1 -
> > drivers/char/ipmi/ipmi_poweroff.c | 1 -
> > drivers/char/random.c | 1 -
> > drivers/gpu/drm/i915/i915_perf.c | 1 -
> > drivers/hv/hv_common.c | 1 -
> > drivers/infiniband/core/iwcm.c | 1 -
> > drivers/infiniband/core/ucma.c | 1 -
> > drivers/macintosh/mac_hid.c | 1 -
> > drivers/md/md.c | 1 -
> > drivers/misc/sgi-xp/xpc_main.c | 2 --
> > drivers/net/vrf.c | 1 -
> > drivers/parport/procfs.c | 28 +++++++++++----------------
> > drivers/scsi/scsi_sysctl.c | 1 -
> > drivers/scsi/sg.c | 1 -
> > drivers/tty/tty_io.c | 1 -
> > drivers/xen/balloon.c | 1 -
> > 18 files changed, 11 insertions(+), 35 deletions(-)
> > ---
> > base-commit: 0e945134b680040b8613e962f586d91b6d40292d
> > change-id: 20230927-jag-sysctl_remove_empty_elem_drivers-f034962a0d8c
> >
> > Best regards,

--

Joel Granados

Attachment: signature.asc
Description: PGP signature

Next message: Ingo Molnar: "Re: [PATCH 6/6] selftests: futex: remove duplicate unneeded defines"
Previous message: Hengqi Chen: "[RFC PATCH 2/2] seccomp: Introduce SECCOMP_ATTACH_FILTER operation"
In reply to: Christophe Leroy: "Re: [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers"
Next in thread: David Ahern: "Re: [PATCH v2 10/15] vrf: Remove the now superfluous sentinel element from ctl_table array"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]