Re: [PATCH 00/69] faster tree-based sysctl implementation

Lucian Adrian Grijincu
2011-05-01T23:34:42Z

On Mon, May 2, 2011 at 12:49 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>> Patch 60 makes sure that if a directory is not found a
>> ctl_table_header will be created for it. That directory can be freed
>> by anyone else when the reference count becomes 0.
>> E.g. register /newdir/file1, register /newdir/file2, unregister
>> /newdir/file1, unregister /newdir/file1
>> - 1st registration created 'newdir' and takes a reference to it.
>> - 2nd regisitration incs the reference count.
>> - 1st unregister decs the reference count.
>> - 2nd unregister decs the reference count which becomes 0 and frees 'newdir'.
>>
>> I may have misunderstood your comment.
>
> While you can make directory lifetimes work by ref counting.  Making it
> work by ref counting removes a concept of ownership and makes it hard to
> see when a sysctl directory should exist.

I can do what you want but I see no good reason to this restriction.
Entities that register sysctls are interested in their files showing
up there.
In my previous example if two modules want to add /newdir/file1 and
/newdir/file2 they must either:
- find a third party that will register /newdir for them
- make sure they have a strict dependency relation (only registere
/newdir/file2 if the first module is loaded and it registered
/newdir/file1 or at least /newdir)

This is why we register an empty directory for "/proc/sys/dev/" (a
third party that registers the 'dev' dir and all others use it).

Again, I can do what you ask, but it either means rethinking some of
my patches, or adding the restriction on top of them.

> If we are reforming this we should go all of the way and make strict
> lifetime rules of directories.  So that it is required to do:
>
> register newdir
> register newdir/file1
> unregister newdir/file1
> unregister newdir
>
> Today there is a WARN_ON in unregister_sysctl_table enforcing the
> ordering that directories must come first and be removed after
> the files in those directories.  If we change the table registrations
> to only include leaves (as your removal of .child patches do) I
> believe that rule becomes absolute.

> That WARN_ON came in with Al Viro's last major sweep through the code,
> and it started moving sysctl in the direction of a normal filesystem.

Regarding that: I'm now trying to reproduce a NULL access in
sysctl_is_seen that I got only once. It's coupled with a
"rcu_sched_state stall detected" warning from RCU.

> For long term maintainability and comprehensibility I think moving
> sysctl in the direction of a normal filesystem makes a lot of sense.

Ok, I agree that a later redesign of sysctl would suffer if there
would be a mess regarding registration/unregistration of directories
and no ownership information/enforcement. I'll see how this can be
addressed in the next respin of this series.

--
.
..: Lucian