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

[Lucian Adrian Grijincu <lucian.grijincu@gmail.com>]

On Sun, May 1, 2011 at 5:28 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> The first patches remove the .child field of ctl_table. This is a
>> requirement for the new algorithm. These patches are scattered all
>> over the tree :(
>
> Only registering sysctl leaves seems very reasonable, the code has
> been slowly moving in that direction for quite a while.
>
> We need to make it a firm rule that directories are created before
> they are used. (aka normal filesystem semantics) before we churn
> through and change everything.


I don't understand what you want to say.

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.


> This also addresses a question you asked in patch 60.
>
>> This can be fixed in two ways:
>>
>> - A) by making sure to never register a netns specific directory and
>>   after that register that directory as a common one. From what I can
>>   tell there isn't such a problem in the kernel at the moment, but I
>>   did not study the source in detail.
>
> Currently it is a requirement that if you are going to have a netns
> component and a non nents component the non-netns directory must
> be created first.  However that requirement is not currently enforced.
>
> It is a bit too easy to get that wrong.  So we need enforcement of that
> rule and enforcement of duplicate checks.  The sysctl_check code should
> become mandatory at this point, or at least the duplicate checks.


I'll post a modified check enforcing this.


> Since we are touching most if not all of the sysctl registrations this
> would also be a good time to pass a path string instead of the weird
> ctl_path data structure.  We needed ctl_path when we had both binary
> and proc paths to worry about but we no longer have that concern.


I still find good use for it in the next patches (some optimisations).
Getting rid of it makes some things more difficult:
- I wouldn't like to parse strings into path components at registeration
- users of the register_sysctl_paths would need to create strings with
dynamic components (for example "net/conf/%s/" - where %s is a
netdevice-name or "kernel/sched_domain/%s/%s" with cpu-name and
domain-name).


> We may also want to add a special sysctl directory creation and removal
> operations.

That can be done very easily now: register an empty table. But I can
add something special for directories only if needed.


>> People interested in the core sysctl changes/networking should read:
>>
>>    [PATCH 60/69] sysctl: faster tree-based sysctl implementation
>>
>> which introduces the new algorithm (commit message and comments have
>> more details), and the next few patches which add some further (simple
>> and effective) optimisations for networking (and not only).
>
> Patch 60 does too many things all in one patch.  It would be very good
> if it could be split up some more, so it could be more easily verified.


Ok. I'll see what I can do.


-- 
 .
..: Lucian