Le 02/10/2023 à 10:55, Joel Granados via B4 Relay a écrit : > From: Joel Granados <j.granados@xxxxxxxxxxx> > > What? > These commits remove the sentinel element (last empty element) from the > sysctl arrays of all the files under the "drivers/" directory that use a > sysctl array for registration. The merging of the preparation patches > (in https://lore.kernel.org/all/ZO5Yx5JFogGi%2FcBo@xxxxxxxxxxxxxxxxxxxxxx/) > to mainline allows us to just remove sentinel elements without changing > behavior (more info here [1]). > > These commits are part of a bigger set (here > https://github.com/Joelgranados/linux/tree/tag/sysctl_remove_empty_elem_V4) > that remove the ctl_table sentinel. Make the review process easier by > chunking the commits into manageable pieces. Each chunk can be reviewed > separately without noise from parallel sets. > > Now that the architecture chunk has been mostly reviewed [6], we send > the "drivers/" directory. Once this one is done, it will be follwed by > "fs/*", "kernel/*", "net/*" and miscellaneous. The final set will remove > the unneeded check for ->procname == NULL. > > Why? > By removing the sysctl sentinel elements we avoid kernel bloat as > ctl_table arrays get moved out of kernel/sysctl.c into their own > respective subsystems. This move was started long ago to avoid merge > conflicts; the sentinel removal bit came after Mathew Wilcox suggested > it to avoid bloating the kernel by one element as arrays moved out. This > patchset will reduce the overall build time size of the kernel and run > time memory bloat by about ~64 bytes per declared ctl_table array. I > have consolidated some links that shed light on the history of this > effort [2]. > > Testing: > * Ran sysctl selftests (./tools/testing/selftests/sysctl/sysctl.sh) > * Ran this through 0-day with no errors or warnings > > Size saving after removing all sentinels: > These are the bytes that we save after removing all the sentinels > (this plus all the other chunks). I included them to get an idea of > how much memory we are talking about. > * bloat-o-meter: > - The "yesall" configuration results save 9158 bytes > https://lore.kernel.org/all/20230621091000.424843-1-j.granados@xxxxxxxxxxx/ > - The "tiny" config + CONFIG_SYSCTL save 1215 bytes > https://lore.kernel.org/all/20230809105006.1198165-1-j.granados@xxxxxxxxxxx/ > * memory usage: > In memory savings are measured to be 7296 bytes. (here is how to > measure [3]) > > Size saving after this patchset: > * bloat-o-meter > - 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/ 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 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,
