Re: [Openipmi-developer] [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers
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-e59120fca...@samsung.com Thanks! Pushed onto sysctl-next for wider testing. Luis ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers
On Mon, Oct 02, 2023 at 12:27:18PM +, Christophe Leroy wrote:
>
>
> Le 02/10/2023 à 10:55, Joel Granados via B4 Relay a écrit :
> > From: Joel Granados
> >
<--- 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-e59120fca...@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-02dd0d46f...@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.grana...@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%2f...@bombadil.infradead.org/
> > * Another very good summary from Luis.
> >https://lore.kernel.org/all/zmfizkfkvxuft...@bombadil.infradead.org/
> > * This is a patch set that replaces register_sysctl_table with
> > register_sysctl
> >https://lore.kernel.org/all/20230302204612.782387-1-mcg...@kernel.org/
> > * Patch set to deprecate register_sysctl_paths()
> >https://lore.kernel.org/all/20230302202826.776286-1-mcg...@kernel.org/
> > * Here there is an explicit expectation for the removal of the sentinel
> > element.
> >https://lore.kernel.org/all/20230321130908.6972-1-frank...@vivo.com
> > * The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread
> >https://lore.kernel.org/all/20220220060626.15885-1-tangm...@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(>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)
> >
Re: [Openipmi-developer] [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers
Le 02/10/2023 à 10:55, Joel Granados via B4 Relay a écrit : > From: Joel Granados > > 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%2f...@bombadil.infradead.org/) > 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.grana...@samsung.com/ > - The "tiny" config + CONFIG_SYSCTL save 1215 bytes > > https://lore.kernel.org/all/20230809105006.1198165-1-j.grana...@samsung.com/ > * 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-e59120fca...@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-02dd0d46f...@samsung.com/ 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.grana...@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%2f...@bombadil.infradead.org/ > * Another very good summary from Luis. >https://lore.kernel.org/all/zmfizkfkvxuft...@bombadil.infradead.org/ > * This is a patch set that replaces register_sysctl_table with register_sysctl >https://lore.kernel.org/all/20230302204612.782387-1-mcg...@kernel.org/ > * Patch set to deprecate register_sysctl_paths() >https://lore.kernel.org/all/20230302202826.776286-1-mcg...@kernel.org/ > * Here there is an explicit expectation for the removal of the sentinel > element. >https://lore.kernel.org/all/20230321130908.6972-1-frank...@vivo.com > * The "ARRAY_SIZE" approach was mentioned (proposed?) in this thread >https://lore.kernel.org/all/20220220060626.15885-1-tangm...@uniontech.com > > [3] >
