Re: [PATCH 08/11] sysctl: Add size to register_sysctl_init

[Joel Granados]

On Wed, Jun 21, 2023 at 01:36:46PM +0200, Petr Mladek wrote:
> On Wed 2023-06-21 11:09:57, Joel Granados wrote:
> > In order to remove the end element from the ctl_table struct arrays, we
> > explicitly define the size when registering the targes. We add a size
> > argument to the register_sysctl_init call and pass an ARRAY_SIZE for all
> > the callers.
> 
> This does not explain the motivatin why the end element is removed.
I also see that the cover letter also lacks this. Let me clarify in my
V2.

> 
> I agree with Jiri that saving 9k is a questionable gain. According to
> the cover letter it saved 0,00%. It is because it saved 9k with allyes
> config which produces huge kernel. IMHO, the 9k might be interesting
> only for a tiny kernel. But I guess that it would safe much less
> bytes there.
I put the 9K as a upper bound kind of value. To get an idea of exactly
how much we are talking about. A lower bound with tiny config and sysctl
enabled is a good idea to give a range.

> 
> And the code with the added ARRAY_SIZE() parameter looks worse than before.
This might not even be an issue in V2. After analysing Greg's feedback,
these might just go away.
> 
> > diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> > index c228343eeb97..28f37b86414e 100644
> > --- a/kernel/printk/sysctl.c
> > +++ b/kernel/printk/sysctl.c
> > @@ -81,5 +81,6 @@ static struct ctl_table printk_sysctls[] = {
> >  
> >  void __init printk_sysctl_init(void)
> >  {
> > -	register_sysctl_init("kernel", printk_sysctls);
> > +	register_sysctl_init("kernel", printk_sysctls,
> > +			     ARRAY_SIZE(printk_sysctls));
> >  }
> 
> Is register_sysctl_init() still ready to handle the last empty element,
nope, after all the patch set, this functionality would be gone.

> please? I am not in Cc on the related patches.
Not sure what happened there. Should I just add you for the next batch?

> 
> Best Regards,
> Petr

Thx for the feedback

Best
-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature