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
[email protected]
Hi,
On Sun, 7 Jul 2002, Dave Hansen wrote:
> Old Blue? 23 isn't _that_ old!
Obviously, you never read that book about the IBM s/370 named
"Old Blue"...
> 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?
So the BKL isn't wrong here, but incorrectly used?
Is it really okay to "lock the whole kernel" because of one struct file?
This brings us back to spinlocks...
You're possibly right about this one. What did Greg K-H say?
Regards,
Thunder
--
(Use http://www.ebb.org/ungeek if you can't decode)
------BEGIN GEEK CODE BLOCK------
Version: 3.12
GCS/E/G/S/AT d- s++:-- a? C++$ ULAVHI++++$ P++$ L++++(+++++)$ E W-$
N--- o? K? w-- O- M V$ PS+ PE- Y- PGP+ t+ 5+ X+ R- !tv b++ DI? !D G
e++++ h* r--- y-
------END GEEK CODE BLOCK------
> 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.
Perhaps, we could agree that the BKL is used wrongly if it
won't fulfill its presumed function, or fulfills another function
than the obvious without a comment stating that, or fulfills
a non obvious function without any comment ?
The first case is IMHO the worst, because, although BKL
can't hurt technically, it obscures locking.
Regards
Oliver
> > "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?
>
> So the BKL isn't wrong here, but incorrectly used?
The BKL, unless used unbalanced, can never cause a bug.
It could be insufficient or superfluous, but never be really buggy in itself.
> Is it really okay to "lock the whole kernel" because of one struct file?
> This brings us back to spinlocks...
>
> You're possibly right about this one. What did Greg K-H say?
I don't speak for Greg, but in this example it could be dropped IMO.
The spinlock protects the critical section anyway. As a rule, if you
do a kmalloc without GFP_ATOMIC under BKL you are doing either insufficient
locking (you may need a semaphore) or useless locking.
This should have been posted on linux-usb long ago.
Regards
Oliver
On Sun, Jul 07, 2002 at 03:42:56PM -0700, Dave Hansen wrote:
> 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?
Nice example, it proves my previous points:
- you didn't send this to the author of the code, who is very
responsive when you do.
- you didn't send this to the linux-usb-devel list, which is a
very responsive list.
- this is in the file drivers/usb/host/uhci-debug.c, which by
its very nature leads you to believe that this is not critical
code at all. This is true if you look at the code.
- it looks like you could just remove the BKL entirely from this
call, and not just move it around the kmalloc() call. But
since I don't understand all of the different locking rules
inside the uhci-hcd.c driver, I'm not going to do this. I'll
let the author of the driver do that, incase it really matters
(and yes, the locking in the uhci-hcd driver is tricky, but
nicely documented.)
- this file is about to be radically rewritten, to use driverfs
instead of procfs, as the recent messages on linux-usb-devel
state. So any patch you might make will probably be moot in
about 3 days :) Again, contacting the author/maintainer is
the proper thing to do.
- even if you remove the BKL from this code, what have you
achieved? A faster kernel? A very tiny bit smaller kernel,
yes, but not any faster overall. This is not on _any_
critical
path.
thanks,
greg k-h