On Fri, 17 May 96 23:26:43 EDT, Tom Dyas <tdyas@eden.rutgers.edu>
said:
> Now that I have looked at your example in your patch #2, it became
> clear how to register sysctl entries that overlap with existing
> functions. However, I do not think the existing
> (un)register_sysctl_table interface is all that intuitive compared to
> the simpler {un}register_sysctl() interface.
It seems simple enough to me! The only difference is that the current
code requires a full hierarchy at once rather than a piecemeal
registration.
> I think there should be a redesign for several major reasons:
> 1) My implementation uses slightly less memory and more importantly
> avoids invoking kmalloc() to allocate space for the associated
> proc_dir_entry's (now embedded in sysctl_entry since there is a
> one-to-one relationship between sysctl_entry's and proc_dir_entry's)
> and ctl_table_header's (no longer needed).
The kmalloc overhead is not too bad. I'll just mention here that if
we use your own technique, then we have to register each subentry
separately; and about 20 bytes of assembler code wil be required to
make the various calls to register_ and unregister_syscall, so you've
got a hidden overhead of perhaps 40 bytes per entry (much more than
this for a RISC architecture). I'm not sure that there's a big win
either way.
> 2) Also, your code also needs to use a recursive function
> (register_proc_table) to traverse the sysctl tables. IMHO, recursive
> procdures in the kernel is broken behavior unless said function is
> *truly* needed.
Recursion isn't necessarily bad if we're doing it over fixed-depth
static data structures. Checking the assembler, we're using 44 bytes
per level of recursion in total, so this is really not a problem
here. I can't see the sysctl tree depth getting much above 5, and
even 20 levels deep wouldn't hurt.
> Calls to my {un}register_sysctl() function map directly onto single
> calls to proc_{un}register.
That's an internal implementation issue and doesn't affect the
exported sysctl interface, but I think that having a single wrapper to
insert and remove multiple entries is actually a benefit, not a
disadvantage.
> 3) register_symtab() exists to allow kernel code to decide locally
> what symbols get exported. ...
> ... imagine what would happen if we did local registration for most
> of the stuff. We would have to have a "root_table" for each local
> registration which just wastes memory space and does not look nice.
For each root table saved, we'd have the extra overhead of the asm
instructions to do the registration. We'd not actually be saving
space. It's swings and roundabouts again. As for aesthetics, well, I
wrote the code this way and I'm used to it, so I suppose I'm the last
person who should be expected to judge how nice it looks --- it looks
OK to me!
The bottom line is that I can't see that your new mechanism has any
really obvious advantages or disadvantages, so it's probably just as
well to leave the code as it is --- if it ain't broke, don't fix it.
Cheers,
Stephen.
-- Stephen Tweedie <sct@dcs.ed.ac.uk> Department of Computer Science, Edinburgh University, Scotland.