Re: lock_kernel check...

[Dave Hansen <haveblue@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]

cc'ing LKML 'cause this is interesting...

dan carpenter wrote:
 > As you can see, the attached script is dead simple.  It prints an
 > error every time you call return while lock_kernel is held.  On
 > your computer you will want to comment out print_url() and
 > uncomment the regular print statement.

I am continually amazed at all the simple, useful, cool stuff that 
people come up with.  I like!

 > I tested it on linux-2.5.7 because then I could just put a link to
 > the offending lines and get your feedback.  I compiled fs/*/*.c

Time to run it on some more recent versions.  Don't worry about not 
giving links, filename:line# is just fine for most people.  I promise 
_I_ won't complain :)

time for the breakdown:
  > http://lxr.linux.no/source/fs/affs/namei.c?v=2.5.7#L349
error

 > http://lxr.linux.no/source/fs/hpfs/dir.c?v=2.5.7#L194
error

 > http://lxr.linux.no/source/fs/intermezzo/dir.c?v=2.5.7#L580
 > http://lxr.linux.no/source/fs/intermezzo/dir.c?v=2.5.7#L594
 > http://lxr.linux.no/source/fs/intermezzo/file.c?v=2.5.7#L303
 > http://lxr.linux.no/source/fs/intermezzo/vfs.c?v=2.5.7#L1952
 > http://lxr.linux.no/source/fs/intermezzo/vfs.c?v=2.5.7#L2053

Intermezzo is doing some subtle things here.  It needs to do some 
dentry lookups and doesn't want to deadlock with dcache_lock (I 
think).  It releases the BKL to do these, then reacquires it so that 
the caller function doesn't double-unlock.  These are weird, but 
correct.

 > http://lxr.linux.no/source/fs/jbd/journal.c?v=2.5.7#L270

Another subtle one.  This is a throwback to the earliest days of the 
BKL where kernel daemons did a lock_kernel() while they were 
executing, and an unlock_kernel() when they were ready to give up the 
CPU.  This is still the case for kjournald, but in a much more 
convoluted way.  Since kjournald is a thread, when it returns, the 
process is destroyed and the BKL is released implcitly during the exit 
sequence.  That is why you never see an unlock_kernel().

Here's the basic process (may god help us all):

kernel_thread(kjournald,...) // start the new kjournald thread
lock_kernel()
work: // do work

// go to sleep for some reason
schedule()
	unlock_kernel()
	sleeping...
	wake_up()
	lock_kernel()

if( more to do )
	goto work;
else
	exit()
	unlock_kernel().


 > The question is are any of these actual errors?  What other things
 > are possibly illegal to do while lock_kernel is held?

It isn't absoulutely a bad thing to return while you have a lock held. 
    It might be hard to understand, or even crazy, but not immediately 
wrong :)

// BKL protects both "a", and io port 0xF00D
bar()
{
	lock_kernel();
	return inb(0xF00D);
}

int a;
foo()
{
	a = bar();
	a--;
	unlock_kernel();
}


But, back to the subject at hand.  Yes, you've found some errors here 
because in almost all normal cases, you don't mean to return with a 
lock still held.   These were most likely caused by myself or others 
pushing the BKL into these functions from above and are bugs.

Would you like to do the patches to fix it, or do you want me to do 
them?  They shouldn't be hard to do.

What is smatch.pm? I can't find it anywhere.

-- 
Dave Hansen
haveblue@us.ibm.com

[-- Attachment #2: lock-check.pl --]
[-- Type: text/plain, Size: 883 bytes --]

#!/usr/bin/perl -w
use smatch;

sub print_url {
  if (get_filename() =~ /home\/carp0110\/progs\/kernel\/devel\/linux-2.5.7\/(.*)/){
    print 'http://lxr.linux.no/source/', $1, '?v=2.5.7#', get_lineno(), "\n";
  }
}

while ($data = get_data()){
  # if we see a lock_kernel then we set the state to "locked"
  if ($data =~ /call_expr\(\(addr_expr function_decl\(lock_kernel/){
    set_state("locked");
    next;
  }

  # if we see an unlock_kernel we set the state to "unlocked"
  if ($data =~ /call_expr\(\(addr_expr function_decl\(unlock_kernel/){
    set_state("unlocked");
    next;
  }

  # if we see a return statement and the kernel is 
  # locked then we print a mesg.
  if ($data =~ /^return_stmt/){
    if (get_state() =~ /^locked$/){
      #print "Not unlocked: ", get_filename(), " ", get_start_line(), " ", get_lineno(), "\n";
      print_url();
      next;
    }
  }
}