| Message ID | 20231002-jag-sysctl_remove_empty_elem_drivers-v2-0-02dd0d46f71e@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 10/2/23 2:55 AM, Joel Granados via B4 Relay wrote: > From: Joel Granados <j.granados@samsung.com> > > This commit comes at the tail end of a greater effort to remove the > empty elements at the end of the ctl_table arrays (sentinels) which > will reduce the overall build time size of the kernel and run time > memory bloat by ~64 bytes per sentinel (further information Link : > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/) > > Remove sentinel from vrf_table > > Signed-off-by: Joel Granados <j.granados@samsung.com> > --- > drivers/net/vrf.c | 1 - > 1 file changed, 1 deletion(-) > Reviewed-by: David Ahern <dsahern@kernel.org>
On Mon, Oct 02, 2023 at 10:55:28AM +0200, Joel Granados via B4 Relay wrote: > From: Joel Granados <j.granados@samsung.com> > > This commit comes at the tail end of a greater effort to remove the > empty elements at the end of the ctl_table arrays (sentinels) which > will reduce the overall build time size of the kernel and run time > memory bloat by ~64 bytes per sentinel (further information Link : > https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@bombadil.infradead.org/) > > Remove sentinel from xpc_sys_xpc_hb and xpc_sys_xpc > > Signed-off-by: Joel Granados <j.granados@samsung.com> > --- > drivers/misc/sgi-xp/xpc_main.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/misc/sgi-xp/xpc_main.c b/drivers/misc/sgi-xp/xpc_main.c > index 6da509d692bb..3186421e82c3 100644 > --- a/drivers/misc/sgi-xp/xpc_main.c > +++ b/drivers/misc/sgi-xp/xpc_main.c > @@ -110,7 +110,6 @@ static struct ctl_table xpc_sys_xpc_hb[] = { > .proc_handler = proc_dointvec_minmax, > .extra1 = &xpc_hb_check_min_interval, > .extra2 = &xpc_hb_check_max_interval}, > - {} > }; > static struct ctl_table xpc_sys_xpc[] = { > { > @@ -121,7 +120,6 @@ static struct ctl_table xpc_sys_xpc[] = { > .proc_handler = proc_dointvec_minmax, > .extra1 = &xpc_disengage_min_timelimit, > .extra2 = &xpc_disengage_max_timelimit}, > - {} > }; > > static struct ctl_table_header *xpc_sysctl; > > -- > 2.30.2 > Reviewed-by: Steve Wahl <steve.wahl@hpe.com>
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@samsung.com> > > <--- 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@samsung.com > > > > 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@samsung.com/ 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@samsung.com/) > > 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@bombadil.infradead.org/ > > * Another very good summary from Luis. > > https://lore.kernel.org/all/ZMFizKFkVxUFtSqa@bombadil.infradead.org/ > > * This is a patch set that replaces register_sysctl_table with register_sysctl > > https://lore.kernel.org/all/20230302204612.782387-1-mcgrof@kernel.org/ > > * Patch set to deprecate register_sysctl_paths() > > https://lore.kernel.org/all/20230302202826.776286-1-mcgrof@kernel.org/ > > * Here there is an explicit expectation for the removal of the sentinel element. > > https://lore.kernel.org/all/20230321130908.6972-1-frank.li@vivo.com > > * The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread > > https://lore.kernel.org/all/20220220060626.15885-1-tangmeng@uniontech.com > > > > [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@samsung.com/ > > > > Signed-off-by: Joel Granados <j.granados@samsung.com> > > > > To: Luis Chamberlain <mcgrof@kernel.org> > > To: willy@infradead.org > > To: josh@joshtriplett.org > > To: Kees Cook <keescook@chromium.org> > > To: Phillip Potter <phil@philpotter.co.uk> > > To: Clemens Ladisch <clemens@ladisch.de> > > To: Arnd Bergmann <arnd@arndb.de> > > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > To: Juergen Gross <jgross@suse.com> > > To: Stefano Stabellini <sstabellini@kernel.org> > > To: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > To: Jiri Slaby <jirislaby@kernel.org> > > To: "James E.J. Bottomley" <jejb@linux.ibm.com> > > To: "Martin K. Petersen" <martin.petersen@oracle.com> > > To: Doug Gilbert <dgilbert@interlog.com> > > To: Sudip Mukherjee <sudipm.mukherjee@gmail.com> > > To: Jason Gunthorpe <jgg@ziepe.ca> > > To: Leon Romanovsky <leon@kernel.org> > > To: Corey Minyard <minyard@acm.org> > > To: Theodore Ts'o <tytso@mit.edu> > > To: "Jason A. Donenfeld" <Jason@zx2c4.com> > > To: David Ahern <dsahern@kernel.org> > > To: "David S. Miller" <davem@davemloft.net> > > To: Eric Dumazet <edumazet@google.com> > > To: Jakub Kicinski <kuba@kernel.org> > > To: Paolo Abeni <pabeni@redhat.com> > > To: Robin Holt <robinmholt@gmail.com> > > To: Steve Wahl <steve.wahl@hpe.com> > > To: Russ Weight <russell.h.weight@intel.com> > > To: "Rafael J. Wysocki" <rafael@kernel.org> > > To: Song Liu <song@kernel.org> > > To: "K. Y. Srinivasan" <kys@microsoft.com> > > To: Haiyang Zhang <haiyangz@microsoft.com> > > To: Wei Liu <wei.liu@kernel.org> > > To: Dexuan Cui <decui@microsoft.com> > > To: Jani Nikula <jani.nikula@linux.intel.com> > > To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > To: Rodrigo Vivi <rodrigo.vivi@intel.com> > > To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > To: David Airlie <airlied@gmail.com> > > To: Daniel Vetter <daniel@ffwll.ch> > > Cc: linux-kernel@vger.kernel.org > > Cc: xen-devel@lists.xenproject.org > > Cc: linux-serial@vger.kernel.org > > Cc: linux-scsi@vger.kernel.org > > Cc: linuxppc-dev@lists.ozlabs.org > > Cc: linux-rdma@vger.kernel.org > > Cc: openipmi-developer@lists.sourceforge.net > > Cc: netdev@vger.kernel.org > > Cc: linux-raid@vger.kernel.org > > Cc: linux-hyperv@vger.kernel.org > > Cc: intel-gfx@lists.freedesktop.org > > Cc: dri-devel@lists.freedesktop.org > > > > --- > > > > --- > > 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,
On Mon, Oct 02, 2023 at 10:55:17AM +0200, Joel Granados via B4 Relay wrote: > 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@samsung.com Thanks! Pushed onto sysctl-next for wider testing. Luis
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;