lock_kernel check...

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

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

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:

error

error

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.

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().

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
[email protected]

#!/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;
    }
  }
}

But broken nonetheless, that kinda thing just looks ugly. Especially when someone tries to call bar multiple times or consecutively or with the lock already held or...

Zwane Mwaikambo

function.linuxpower.ca

Yes, it is horribly ugly, but it is not broken. As a function, if you document what you require your caller to do, there shouldn't be a problem.

Also, it is valid to have nested holds of the BKL. You can never deadlock with another lock_kernel() which was done in the same process.

Dave Hansen
[email protected]