Date:   Tue, 7 Dec 2021 12:18:38 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Xiaoming Ni <nixiaoming@...wei.com>, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk, ebiederm@...ssion.com,
        keescook@...omium.org, jlayton@...nel.org, bfields@...ldses.org,
        yzaikin@...gle.com, wangle6@...wei.com,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH] sysctl: Add a group of macro functions to initcall the
 sysctl table of each feature

On Mon, Dec 06, 2021 at 05:38:42PM -0800, Andrew Morton wrote:
> On Tue, 7 Dec 2021 09:13:20 +0800 Xiaoming Ni <nixiaoming@...wei.com> wrote:
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -132,12 +132,7 @@ static struct ctl_table inodes_sysctls[] = {
> >  	{ }
> >  };
> >  
> > -static int __init init_fs_inode_sysctls(void)
> > -{
> > -	register_sysctl_init("fs", inodes_sysctls);
> > -	return 0;
> > -}
> > -early_initcall(init_fs_inode_sysctls);
> > +fs_sysctl_initcall(inodes_sysctls);
> >  #endif
> 
> Here's another, of many.
> 
> Someone made the decision to use early_initcall() here (why?) and this
> patch switches it to late_initcall()!  Worrisome.  Each such stealth
> conversion should be explained and justified, shouldn't it?

I made the decisions for quite a bit of the ordering and yes I agree
this need *very careful* explanation, specially if we are going to
generalize this.

First and foremost. git grep for sysctl_init_bases and you will see
that the bases for now are initialized on proc_sys_init() and that
gets called on proc_root_init() and that in turn on init/main.c's
start_kernel(). And so this happens *before* the init levels.

The proper care for what goes on top of this needs to take into
consideration the different init levels and that the if a sysctl
is using a directory *on top* of a base, then that sysctl registration
must be registered *after* that directory. The *base* directory for
"fs" is now registered through fs/sysctls.c() on init_fs_sysctls()
using register_sysctl_base(). I made these changes with these names
and requiring the DECLARE_SYSCTL_BASE() so it would be easy for us
to look at where these are declared.

So the next step in order to consider is *link* ordering and that
order is maintained by the Makefile. That is why I put this at the
top of the fs Makfile:

obj-$(CONFIG_SYSCTL)            += sysctls.o 

So any file after this can use early_initcall(), because the base
for "fs" was declared first in link order, and it used early_initcall().
It is fine to have the other stuff that goes on top of the "fs" base
use late_initcall() but that assumes that vetting has been done so that
if a directory on "fs" was created, let's call it "foo", vetting was done
to ensure that things on top of "foo" are registered *after* the "foo"
directory.

We now have done the cleanup for "fs", and we can do what we see fine
for "fs", but we may run into surprises later with the other bases, so
I'd be wary of making assumptions at this point if we can use
late_initcall().

So, as a rule of thumb I'd like to see bases use early_initcall(). The
rest requires manual work and vetting.

So, how about this, we define fs_sysctl_initcall() to use also
early_initcall(), and ask susbsystems to do their vetting so that
the base also gets linked first.

After this, if a directory on top of a base is created we should likely create
a new init level and just bump that to use the next init level. So
something like fs_sysctl_base_initcall_subdir_1() map to core_initcall()
and so on.

That would allow us to easily grep for directory structures easily and
puts some implicit onus of ordering on those folks doing these conversions.
We'd document well the link order stuff for those using the base stuff
too as that is likely only where this will matter most.

  Luis

Powered by blists - more mailing lists