Oliver Neukum wrote:
>>> "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.
Does incredibly high lock contention qualify as a bug?
>> 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.
Don't forget that the BKL is released on sleep. It is OK to hold it
over a schedule()able operation. If I remember right, there is no
real protection needed for the file->private_data either because there
is 1 and only 1 struct file per open, and the data is not shared among
struct files.
> This should have been posted on linux-usb long ago.
I just pulled it out of thin air 10 minutes ago!
--
Dave Hansen
[email protected]
On Sun, Jul 07, 2002 at 04:45:21PM -0700, Dave Hansen wrote:
> Don't forget that the BKL is released on sleep. It is OK to hold it
> over a schedule()able operation. If I remember right, there is no
> real protection needed for the file->private_data either because there
> is 1 and only 1 struct file per open, and the data is not shared among
> struct files.
one struct file per open(), yes. however, fork() shares a struct file,
as does unix domain fd passing. so we need protection between different
processes. there's some pretty good reasons to want to use a semaphore
to protect the struct file (see fasync code.. bleugh).
however, our semaphores currently suck. they attempt to acquire the sem
immediately and if they fail go straight to sleep. schimmel (i think..)
suggests spinning for a certain number of iterations before sleeping.
the great thing is, it's all out of line slowpath code so the additional
size shouldn't matter. obviously this is SMP-only, and it does require
someone to do it who can measure the difference (and figure out how may
iterations to spin for before sleeping).
i was wondering if this might be a project you'd like to take on which
would upset far fewer people and perhaps yield greater advantage.
--
Revolutions do not require corporate support.
On Mon, 8 Jul 2002, Matthew Wilcox wrote:
> one struct file per open(), yes. however, fork() shares a struct file,
> as does unix domain fd passing. so we need protection between different
> processes. there's some pretty good reasons to want to use a semaphore
> to protect the struct file (see fasync code.. bleugh).
??? What exactly do you want to protect there?
ObAnotherQuestion: no, new_inode() doesn't need BKL.
> > The BKL, unless used unbalanced, can never cause a bug. It could be
> > insufficient or superfluous, but never be really buggy in itself.
>
> Does incredibly high lock contention qualify as a bug?
Only if it occurs at a place that is used often. Even then it's
a minor bug, while a race is always a race.
> >> 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.
>
> Don't forget that the BKL is released on sleep. It is OK to hold it
Exactly therefore it won't protect you from reentrance if you do
something that sleeps. If that's the case, you've found a race condition.
> over a schedule()able operation. If I remember right, there is no
> real protection needed for the file->private_data either because there
> is 1 and only 1 struct file per open, and the data is not shared among
> struct files.
But between threads. Anyway, this was an open, if you use struct file
before open returns even the BKL can't help you.
> > This should have been posted on linux-usb long ago.
>
> I just pulled it out of thin air 10 minutes ago!
Yes, I understand. But this is a symptom. We should have heard
about it.
You need to understand that people who run UP mostly won't
care about SMP beyond making sure that the drivers are SMP-safe.
This is not true for core kernel code, but for devices outside
the block layer and perhaps some network drivers, that's the case.
So you do the greps and question locking usage on the right lists.
It's quite important, you might uncover bugs and speed up the kernel.
And you need to be persistent about it. Take your time to find out why
the lock is used. Or why it makes no sense.
You should be able to improve the locking situation that way, with respect
to BKL contention and driver quality.
Regards
Oliver
On Sun, Jul 07, 2002 at 10:58:04PM -0400, Alexander Viro wrote:
> On Mon, 8 Jul 2002, Matthew Wilcox wrote:
> > one struct file per open(), yes. however, fork() shares a struct file,
> > as does unix domain fd passing. so we need protection between different
> > processes. there's some pretty good reasons to want to use a semaphore
> > to protect the struct file (see fasync code.. bleugh).
>
> ??? What exactly do you want to protect there?
andrea & i discussed this off-list a few days ago... see fs/fcntl.c
case F_SETFL:
lock_kernel();
err = setfl(fd, filp, arg);
unlock_kernel();
setfl() does:
if ((arg ^ filp->f_flags) & FASYNC) {
if (filp->f_op && filp->f_op->fasync) {
error = filp->f_op->fasync(fd, filp, (arg & FASYNC) != 0);
and:
if (arg & O_DIRECT) {
down(&inode->i_sem);
if (!filp->f_iobuf)
error = alloc_kiovec(1, &filp->f_iobuf);
up(&inode->i_sem);
and finally:
filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
i pointed out that if alloc_kiovec slept, the BKL provides no protection
against someone else doing a setfl at the same time, so we can get the
wrong number of fasync events sent. Marcus Alanen pointed out that
fasync can also sleep, so we're at risk anyway. i don't think that
abusing i_sem as andrea did is the Right Thing to do...
--
Revolutions do not require corporate support.