Re: BKL removal

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

Thunder from the hill wrote:

 >> "As long as I comment [and understand] why I am using the BKL."
 >> would be a bit more accurate.  How many places in the kernel have
 >> you seen comments about what the BKL is actually doing?  Could
 >> you point me to some of your comments where _you_ are using the
 >> BKL?  Once you fully understand why it is there, the extra step
 >> of removal is usually very small.
 >
 > Old Blue, could you please bring me an example on where in USB the
 > bkl shouldn't be used, but is?  And can you explain why it's wrong
 > to use bkl there?

Old Blue?  23 isn't _that_ old!

BKL use isn't right or wrong -- it isn't a case of creating a deadlock 
or a race.  I'm picking a relatively random function from "grep -r 
lock_kernel * | grep /usb/".  I'll show what I think isn't optimal 
about it.

"up" is a local variable.  There is no point in protecting its 
allocation.  If the goal is to protect data inside "up", there should 
probably be a subsystem-level lock for all "struct uhci_hcd"s or a 
lock contained inside of the structure itself.  Is this the kind of 
example you're looking for?

  static int uhci_proc_open(struct inode *inode, struct file *file)
  {
         const struct proc_dir_entry *dp = PDE(inode);
         struct uhci_hcd *uhci = dp->data;
         struct uhci_proc *up;
         unsigned long flags;
         int ret = -ENOMEM;

-       lock_kernel();

         up = kmalloc(sizeof(*up), GFP_KERNEL);
         if (!up)
                 goto out;

         up->data = kmalloc(MAX_OUTPUT, GFP_KERNEL);
         if (!up->data) {
                 kfree(up);
                 goto out;
         }

+       lock_kernel();
         spin_lock_irqsave(&uhci->frame_list_lock, flags);
         up->size = uhci_sprint_schedule(uhci, up->data, MAX_OUTPUT);
         spin_unlock_irqrestore(&uhci->frame_list_lock, flags);

         file->private_data = up;
+ 
unlock_kernel();

         ret = 0;
  out:
-       unlock_kernel();
         return ret;
  }



-- 
Dave Hansen
haveblue@us.ibm.com