lkml.org 
[lkml]   [2021]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] sysctl: Add a group of macro functions to initcall the sysctl table of each feature
From
Date
On 2021/12/8 10:44, Luis Chamberlain wrote:
> On Wed, Dec 08, 2021 at 10:10:08AM +0800, Xiaoming Ni wrote:
>> On 2021/12/8 6:39, Luis Chamberlain wrote:
>>> On Tue, Dec 07, 2021 at 03:08:03PM -0600, Eric W. Biederman wrote:
>>>> Luis Chamberlain <mcgrof@kernel.org> writes:
>>>>
>>>>> 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@huawei.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.
>>>>
>>>> I am a bit confused at this explanation of things.
>>>>
>>>> Last I looked the implementation of sysctls allocated the directories
>>>> independently of the sysctls entries that populated them.
>>>
>>> With most sysctls being created using the same kernel/sysctl.c file and
>>> structure, yes, this was true. With the changes now on linux-next things
>>> change a bit. The goal is to move sysctls to be registered where they
>>> are actually defined. But the directory that holds them must be
>>> registered first. During the first phase of cleanups now on linux-next
>>> all filesystem "fs" syscls were moved to be delcared in the kernel's
>>> fs/ directory. The last part was to register the base "fs" directory.
>>> For this declareres were added to simplify that and to clarify which
>>> are base directories:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=ededd3fc701668743087c77ceeeb7490107cc12c
>>>
>>> Then, this commit moves the "fs" base to be declared to fs/ as well:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d0f885a73ec6e05803ce99f279232b3116061ed8
>>>
>>> This used early_initcall() for the base for "fs" and that is
>>> because there are no built-in sysctls for "fs" which need to
>>> be exposed prior to the init levels.
>>>
>>> So after this then order is important. If you are using the same
>>> init level, the the next thing which will ensure order is the order
>>> of things being linked, so what order they appear on the Makefile.
>>> And this is why the base move for the "fs" sysctl directory is kept
>>> at the top of fs/Makfile:
>>>
>>> obj-$(CONFIG_SYSCTL) += sysctls.o
>>>
>>> Luis
>>> .
>>>
>>
>> Root node of the tree, using "early_initcall":
>> Basic framework, "fs", "kernel", "debug", "vm", "dev", "net"
>
> register_sysctl_base() and yes these use early_initcall() as-is on
> linux-next.
>
>> Fork node. Select initcall_level based on the number of directory levels:
>> Registration directory shared by multiple features.
>
> Sure.
>
/proc/sys/kernel/random/
random_table
driver/char/random.c
/proc/sys/kernel/usermodehelper/
usermodehelper_table
kernel/umh.c
/proc/sys/kernel/firmware_config/
firmware_config_table
drivers/base/firmware_loader/fallback_table.c
/proc/sys/kernel/keys/
key_sysctls
security/keys/sysctl.c
/proc/sys/fs/inotify/
inotify_table
fs/notify/inotify/inotify_user.c
/proc/sys/fs/fanotify/
fanotify_table
fs/notify/fanotify/fanotify_user.c
/proc/sys/fs/epoll
epoll_table
fs/eventpoll.c

I haven't checked all the sysctl subdirectories, but it seems that many
are not shared by multiple features.
Most features use the sysctl mechanism simply to create a file interface
for configuring parameters.
There are few scenarios for creating directories for other features.
There may be tree fork nodes, but only a few.


>> Leaf node, use "late_initcall":
>> File Interface
>
> I am not sure this gives enough guidance. What is the difference between
> fork node and a leaf node?
Leaf node:
a) File, .child = NULL
b) Directory, which is not shared by multiple features, .child != NULL


Thanks
Xiaoming Ni

\
 
 \ /
  Last update: 2021-12-08 13:35    [W:1.291 / U:1.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site