On Wed, Jun 21, 2023 at 03:10:57PM +0200, Greg KH wrote: > 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. Yes. And I actually think that there is a patch somewhere (I can't find it in my history) that does something similar. Additionally, when the call that does not have the size is used with a non-array, then the developer gets a warning. Still unsure about the indirection calls. But I'm guessing that they can have the same define. > > 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. All good points. thx for the feedback, let me see if my V2 can be less invasive :) Best > > thanks, > > greg k-h -- Joel Granados
Attachment:
signature.asc
Description: PGP signature
