Re: [PATCH 00/11] Remove the end element in sysctl table arrays.

From: Greg KH <greg@kroah.com>
To: Joel Granados <j.granados@samsung.com>
Cc: mcgrof@kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH 00/11] Remove the end element in sysctl table arrays.
Date: Wed, 21 Jun 2023 15:10:57 +0200	[thread overview]
Message-ID: <2023062102-letdown-roving-921d@gregkh> (raw)
In-Reply-To: <20230621123816.ufqbob6qthz4hujx@localhost>

On Wed, Jun 21, 2023 at 02:38:16PM +0200, Joel Granados wrote:
> On Wed, Jun 21, 2023 at 12:46:47PM +0200, Greg KH wrote:
> > On Wed, Jun 21, 2023 at 11:09:49AM +0200, Joel Granados wrote:
> > > This is part of the effort to remove the empty element from the ctl_table
> > > structures (used to calculate size) and replace it with the ARRAY_SIZE macro.
> > > The "sysctl: Remove the end element in sysctl table arrays" commit is the one that
> > > actually removes the empty element. With a "yesall" configuration the bloat-o-meter
> > > says that 9158 bytes where saved (report at the end of the cover letter).
> > 
> > 9k in ram or read-only memory?
> AFAIK its ro as I'm removing all the "empty" end elements from ctl_table
> array that are hardcoded all over the place.
> > 
> > > Main changes:
> > > 1. Add the ctl_table size into the ctl_table_header
> > > 2. Remove the empty element at the end of all ctl_table arrays
> > > 
> > > Commit Overview:
> > > 1. There are preparation commits that make sure that we have the
> > >    ctl_table_header in all the places that we need to have the array size.
> > >       sysctl: Prefer ctl_table_header in proc_sysct
> > >       sysctl: Use the ctl header in list ctl_table macro
> > >       sysctl: Add ctl_table_size to ctl_table_header
> > > 
> > > 2. Add size to relevant register calls. Calculate the ctl_table array size
> > >    where register_sysctl is called. Add a table_size argument to the relevant
> > >    sysctl register functions (init_header, __register_sysctl_table,
> > >    register_net_sysctl, register_sysctl and register_sysctl_init). Important to
> > >    note that these commits do NOT change the way we calculate size; they plumb
> > >    things in preparation for the empty element removal commit. Care is taken to
> > >    leave the tree in a state where it can be compiled which is the reason to
> > >    not separate the "big" commits (like "sysctl: Add size to the
> > >    register_net_sysctl function"). If you have an alternative way of dealing
> > >    with such a big commit while leaving it in a compilable state, please let me
> > >    know.
> > >       sysctl: Add size argument to init_header
> > >       sysctl: Add a size arg to __register_sysctl_table
> > >       sysctl: Add size to the register_net_sysctl function
> > >       sysctl: Add size to register_sysctl
> > >       sysctl: Add size to register_sysctl_init
> > 
> > Why not make these calls automatically calculate the size based on the
> > structure passed into them by using a #define instead of having to touch
> > the code everywhere?  That would make this much simpler AND make it
> > impossible for future people to get this wrong.
> I considered this at the outset, but it will not work with callers that
> use a pointer instead of the actual array.

Then make 2 functions, one a "normal" one where you can't get it wrong
as you pass in the structure that you can compute ARRAY_SIZE() and one
that you have to do it manually.

Don't force developers to think about stuff like this as now you are
going to have to constantly audit the code to verify that the array size
is correct.  Right now it always "just works" due to the null
termination, and now you are going to add complexity to the author in
order to save a trivial amount of memory that no one is asking for :)

> Additionally, we would not avoid big commits as we would have to go
> looking in all the files where register is called directly or indirectly
> and make sure the logic is sound.

Then you need to think about how this could be done better, having "flag
days" like this just doesn't work, sorry.  There are ways to evolve
common apis, and it's not like this patch set :)

I'm all for saving space, but do NOT do it at the expense of making apis
harder to use and easier to get incorrect.  That will just cause more
long-term problems and bugs, which is NOT a good trade off you ever want
to make.

thanks,

greg k-h