bkl cleanup in do_sysctl

bkl cleanup in do_sysctl

[Josh Aas]

I'd like to hear people's thoughts on replacing the bkl in do_sysctl 
with a localized spin lock that protects the sysctl structures. Instead 
of grabbing the bkl, anyone that needs to mess with those values could 
grab the localized lock (1 to protect all structures). Such a localized 
lock would allow us to get rid of bkl usage in at least one other place 
as well (do_coredump). In order to do this though, we would have to make 
sure all code that grabs the bkl instead of the localized lock while 
using sysctl values switches to the new lock. Might be a big job, but 
perhaps it would be a good one to start after 2.6.8 is out the door.

Thoughts? Comments?

-- 
Josh Aas
Silicon Graphics, Inc. (SGI)
Linux System Software
651-683-3068

Re: bkl cleanup in do_sysctl

[Dave Hansen]

On Tue, 2004-08-10 at 09:58, Josh Aas wrote:
> I'd like to hear people's thoughts on replacing the bkl in do_sysctl 
> with a localized spin lock that protects the sysctl structures. Instead 
> of grabbing the bkl, anyone that needs to mess with those values could 
> grab the localized lock (1 to protect all structures). Such a localized 
> lock would allow us to get rid of bkl usage in at least one other place 
> as well (do_coredump). In order to do this though, we would have to make 
> sure all code that grabs the bkl instead of the localized lock while 
> using sysctl values switches to the new lock. Might be a big job, but 
> perhaps it would be a good one to start after 2.6.8 is out the door.

Remember that the BKL isn't a plain-old spinlock.  You're allowed to
sleep while holding it and it can be recursively held, which isn't true
for other spinlocks.

So, if you want to replace it with a spinlock, you'll need to do audits
looking for sysctl users that might_sleep() or get called recursively
somehow.  The might_sleep() debugging checks should help immensely for
the first part, but all you'll get are deadlocks at runtime for any
recursive holders.  But, those cases are increasingly rare, so you might
luck out and not have any.  

Or, you could just make it a semaphore and forget about the no sleeping
requirement.  

-- Dave

Re: bkl cleanup in do_sysctl

[Lee Revell]

On Tue, 2004-08-10 at 13:28, Dave Hansen wrote:

> Remember that the BKL isn't a plain-old spinlock.  You're allowed to
> sleep while holding it and it can be recursively held, which isn't true
> for other spinlocks.
> 
> So, if you want to replace it with a spinlock, you'll need to do audits
> looking for sysctl users that might_sleep() or get called recursively
> somehow.  The might_sleep() debugging checks should help immensely for
> the first part, but all you'll get are deadlocks at runtime for any
> recursive holders.  But, those cases are increasingly rare, so you might
> luck out and not have any.  
> 
> Or, you could just make it a semaphore and forget about the no sleeping
> requirement.  
> 

Someone once suggested that newbies who show up on LKML wanting to learn
kernel hacking should be assigned to find one use of the BKL and replace
it with proper locking.  Something similar worked very well with my
previous employer, before giving someone root, new hires would first be
assigned some task like writing a script to take the user account
database and generate a report of old accounts on a bunch of machines,
or rewrite the RADIUS accounting scripts, where the point was really to
get them familiar with the system.

This way, even if they come back with a totally botched fix, someone
will probably just post a correct one.  We could get rid of the BKL very
soon, I count only 247 files with lock_kernel in them.

For example reiserfs uses the BKL for all write locking (!), but it
probably would not be too hard to fix, because you can just look at
another filesystem that has proper locking.

Maybe this should be added to the FAQ:

Q:  I want to hack the kernel, and I *think* I know what I am doing. 
Where do I start?

Lee

Re: bkl cleanup in do_sysctl

[Roland Dreier]

    Lee> Someone once suggested that newbies who show up on LKML
    Lee> wanting to learn kernel hacking should be assigned to find
    Lee> one use of the BKL and replace it with proper locking.

Unfortunately most of the remaining BKL uses seem to be very subtle.
Removing lock_kernel() correctly requires a deep understanding of the
global locking semantics and may be very invasive.  In the end it's
also hard to be sure bugs haven't been introduced.

    Lee> For example reiserfs uses the BKL for all write locking (!),
    Lee> but it probably would not be too hard to fix, because you can
    Lee> just look at another filesystem that has proper locking.

Fixing up a filesystem's write locking doesn't sound like a very good
newbie project to me.

 - R.

Re: bkl cleanup in do_sysctl

[Lee Revell]

On Tue, 2004-08-10 at 15:16, Roland Dreier wrote:
>     Lee> For example reiserfs uses the BKL for all write locking (!),
>     Lee> but it probably would not be too hard to fix, because you can
>     Lee> just look at another filesystem that has proper locking.
> 
> Fixing up a filesystem's write locking doesn't sound like a very good
> newbie project to me.
> 

Exactly, the point isn't to get them to fix it, but as a way to learn
the internals of Linux, with the side benefit that someone might propose
a correct fix.  Also, many people might be new to Linux kernel hacking
but are knowledgeable re: operating systems.  Just a thought.

Lee

Re: bkl cleanup in do_sysctl

[Hans Reiser]

Lee Revell wrote:

>On Tue, 2004-08-10 at 13:28, Dave Hansen wrote:
>
>  
>
>>Remember that the BKL isn't a plain-old spinlock.  You're allowed to
>>sleep while holding it and it can be recursively held, which isn't true
>>for other spinlocks.
>>
>>So, if you want to replace it with a spinlock, you'll need to do audits
>>looking for sysctl users that might_sleep() or get called recursively
>>somehow.  The might_sleep() debugging checks should help immensely for
>>the first part, but all you'll get are deadlocks at runtime for any
>>recursive holders.  But, those cases are increasingly rare, so you might
>>luck out and not have any.  
>>
>>Or, you could just make it a semaphore and forget about the no sleeping
>>requirement.  
>>
>>    
>>
>
>Someone once suggested that newbies who show up on LKML wanting to learn
>kernel hacking should be assigned to find one use of the BKL and replace
>it with proper locking.  Something similar worked very well with my
>previous employer, before giving someone root, new hires would first be
>assigned some task like writing a script to take the user account
>database and generate a report of old accounts on a bunch of machines,
>or rewrite the RADIUS accounting scripts, where the point was really to
>get them familiar with the system.
>
>This way, even if they come back with a totally botched fix, someone
>will probably just post a correct one.  We could get rid of the BKL very
>soon, I count only 247 files with lock_kernel in them.
>
>For example reiserfs uses the BKL for all write locking (!), but it
>probably would not be too hard to fix, because you can just look at
>another filesystem that has proper locking.
>  
>
Wrong. ;-)  Balancing makes it way hard.  Use reiser4.  That has very 
sophisticated locking that pushes the research envelope, if you want to 
read code to learn about locking.....

>Maybe this should be added to the FAQ:
>
>Q:  I want to hack the kernel, and I *think* I know what I am doing. 
>Where do I start?
>
>Lee
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>
>  
>

Re: bkl cleanup in do_sysctl

[Lee Revell]

On Tue, 2004-08-10 at 15:46, Hans Reiser wrote:
> Lee Revell wrote:
> >For example reiserfs uses the BKL for all write locking (!), but it
> >probably would not be too hard to fix, because you can just look at
> >another filesystem that has proper locking.
> >  
> >
> Wrong. ;-)  Balancing makes it way hard.  Use reiser4.  That has very 
> sophisticated locking that pushes the research envelope, if you want to 
> read code to learn about locking.....
> 

OK, I will give it a try.  Are any of the reiser3 latency issues
reported in the voluntary preemption thread addressed in reiser4?

Lee