Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 3 Oct 2023 10:47:49 +0200
From:   Joel Granados <j.granados@samsung.com>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
CC:     Luis Chamberlain <mcgrof@kernel.org>,
        "willy@infradead.org" <willy@infradead.org>,
        "josh@joshtriplett.org" <josh@joshtriplett.org>,
        Kees Cook <keescook@chromium.org>,
        Phillip Potter <phil@philpotter.co.uk>,
        Clemens Ladisch <clemens@ladisch.de>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Juergen Gross <jgross@suse.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Doug Gilbert <dgilbert@interlog.com>,
        Sudip Mukherjee <sudipm.mukherjee@gmail.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Leon Romanovsky <leon@kernel.org>,
        Corey Minyard <minyard@acm.org>, Theodore Ts'o <tytso@mit.edu>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        David Ahern <dsahern@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Robin Holt <robinmholt@gmail.com>,
        Steve Wahl <steve.wahl@hpe.com>,
        Russ Weight <russell.h.weight@intel.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Song Liu <song@kernel.org>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-raid@vger.kernel.org" <linux-raid@vger.kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
        "openipmi-developer@lists.sourceforge.net" 
        <openipmi-developer@lists.sourceforge.net>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH v2 00/15] sysctl: Remove sentinel elements from drivers
Message-ID: <20231003084749.4xxi4z64hgq5a5lw@localhost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
        protocol="application/pgp-signature"; boundary="fnnv6en4biooyeg3"
Content-Disposition: inline
In-Reply-To: <64fd22df-616e-9f5a-26fb-44c4b3423b0c@csgroup.eu>
CMS-TYPE: 201P
References: <20231002-jag-sysctl_remove_empty_elem_drivers-v2-0-02dd0d46f71e@samsung.com>
        <CGME20231002122730eucas1p17643da82bb9aa655b35c3562446ad395@eucas1p1.samsung.com>
        <64fd22df-616e-9f5a-26fb-44c4b3423b0c@csgroup.eu>
Precedence: bulk

--fnnv6en4biooyeg3
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Oct 02, 2023 at 12:27:18PM +0000, Christophe Leroy wrote:
>=20
>=20
> Le 02/10/2023 =E0 10:55, Joel Granados via B4 Relay a =E9crit=A0:
> > From: Joel Granados <j.granados@samsung.com>
> >=20
<--- 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 a=
ny
> >          of the drivers in the patch. To measure it comment the printk =
in
> >          `new_dir` and uncomment the if conditional in `new_links` [3].
> >=20
> > ---
> > Changes in v2:
> > - Left the dangling comma in the ctl_table arrays.
> > - Link to v1: https://lore.kernel.org/r/20230928-jag-sysctl_remove_empt=
y_elem_drivers-v1-0-e59120fca9f9@samsung.com
> >=20
> > Comments/feedback greatly appreciated
>=20
> Same problem on powerpc CI tests, all boot target failed, most of them=20
> with similar OOPS, see=20
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20231002-jag-sysc=
tl_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.

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

>=20
> Christophe
>=20
> >=20
> > Best
> >=20
> > Joel
> >=20
> > [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 =3D=3D NULL, while the sentinel is still present. And when t=
he
> > sentinel is removed, it will stop on the table_size. You can go to
> > (https://lore.kernel.org/all/20230809105006.1198165-1-j.granados@samsun=
g.com/)
> > for more information.
> >=20
> > [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.or=
g/
> > * 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.or=
g/
> > * Patch set to deprecate register_sysctl_paths()
> >    https://lore.kernel.org/all/20230302202826.776286-1-mcgrof@kernel.or=
g/
> > * 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@uniontec=
h.com
> >=20
> > [3]
> > To measure the in memory savings apply this on top of this patchset.
> >=20
> > "
> > 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 =3D new_name;
> >          table[0].mode =3D S_IFDIR|S_IRUGO|S_IXUGO;
> >          init_header(&new->header, set->dir.header.root, set, node, tab=
le, 1);
> > +       // Counts additional sentinel used for each new dir.
> > +       printk("%ld sysctl saved mem kzalloc \n", sizeof(struct ctl_tab=
le));
> >=20
> >          return new;
> >   }
> > @@ -1199,6 +1201,9 @@ static struct ctl_table_header *new_links(struct =
ctl_dir *dir, struct ctl_table_
> >                  link_name +=3D 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, li=
nk_table,
> >                      head->ctl_table_size);
> >          links->nreg =3D nr_entries;
> > "
> > and then run the following bash script in the kernel:
> >=20
> > accum=3D0
> > for n in $(dmesg | grep kzalloc | awk '{print $3}') ; do
> >      echo $n
> >      accum=3D$(calc "$accum + $n")
> > done
> > echo $accum
> >=20
> > [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=3D429940038, After=3D429937606, chg -0.00%
> >=20
> > [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=3D1885527, After=3D1885463, chg -0.00%
> >=20
> > [6] https://lore.kernel.org/all/20230913-jag-sysctl_remove_empty_elem_a=
rch-v2-0-d1bd13a29bae@samsung.com/
> >=20
> > Signed-off-by: Joel Granados <j.granados@samsung.com>
> >=20
> > 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
> >=20
> > ---
> >=20
> > ---
> > Joel Granados (15):
> >        cdrom: Remove now superfluous sentinel element from ctl_table ar=
ray
> >        hpet: Remove now superfluous sentinel element from ctl_table arr=
ay
> >        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 arr=
ay
> >        parport: Remove the now superfluous sentinel element from ctl_ta=
ble 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_tab=
le array
> >        fw loader: Remove the now superfluous sentinel element from ctl_=
table array
> >        raid: Remove now superfluous sentinel element from ctl_table arr=
ay
> >        Drivers: hv: Remove now superfluous sentinel element from ctl_ta=
ble array
> >        intel drm: Remove now superfluous sentinel element from ctl_tabl=
e array
> >=20
> >   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
> >=20
> > Best regards,

--=20

Joel Granados

--fnnv6en4biooyeg3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmUb1TMACgkQupfNUreW
QU+dSwv/VdmQFmwGJjil1rJHyE+Sc/SnIzc2236zL26xGDgEbQ5tSma2ncYbXdL4
nIY2i4Z9kQshrOjHGUx911wqp3BpmOkRtWDCSjn5uDIKCZ2ZjtNvRaYn10/iahkc
606iTKcJXw2XkjU6j7DqorK+iBQ7Dy6glPHp0vKbS6wxFAVsH/IubN/jhiEgK6qA
Q8Iv2LbaECMpNl5fvrI8Gze2IPEWCSv9ZoUFGUt1ARzsCpCQHbAG3fGYJStQk6tM
w5G0yNVOOLjBjYtXxn4ZkZEwRcnpQ5cvI10GdCP5XtGGcK1qEBgpJqaU1pwraZCW
0zhvgLRVFHuJZCpLoqJscGGNZq3aUNky/gOAoKOdoh9n1magAhx2Q8VXuNDO7P0c
peGouYGzJPLDuxYqWXV5qJ+5sAd0L1upStkDSOWEq9plxYTbdcdU/ej77mZrwJcu
sQOWoEt3YL+G3pPRDQzJDEGIijra1TI9FPTIbbuHEwQKpyWoFP+EKAFDrQosXWyn
sCovmRlT
=9d/Z
-----END PGP SIGNATURE-----

--fnnv6en4biooyeg3--