Re: sleep under spinlock, sequencer.c, 2.6.12.5

[Nish Aravamudan <nish.aravamudan@gmail.com>]

On 8/19/05, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Gwe, 2005-08-19 at 10:13 +0200, Peter T. Breuer wrote:
> > The following "sleep under spinlock" is still present as of linux
> > 2.6.12.5 in sound/oss/sequencer.c in midi_outc:
> >
> >
> >         n = 3 * HZ;             /* Timeout */
> >
> >         spin_lock_irqsave(&lock,flags);
> >         while (n && !midi_devs[dev]->outputc(dev, data)) {
> >                 interruptible_sleep_on_timeout(&seq_sleeper, HZ/25);
> >                 n--;
> >         }
> >         spin_unlock_irqrestore(&lock,flags);
> >
> >
> > I haven't thought about it, just noted it. It's been there forever
> > (some others in the sound architecture have been gradually disappearing
> > as newer kernels come out).
> 
> Yep thats a blind substition of lock_kernel in an old tree it seems.
> Probably my fault. Should drop it before the sleep and take it straight
> after.

Also, the use of n makes no sense. Indicates total sleep for 3
seconds, but actually sleep for 40 milliseconds 3*HZ times
(potentially)?

In any case, probably should be:

timeout = jiffies + 3*HZ;

spin_lock_irqsave(&lock, flags);
while (time_before(jiffies, timeout) && !midi_devs[dev]->outputc(dev, data)) {
     spin_unlock_irqrestore(&lock, flags);
     interruptible_sleep_on_timeout(&seq_sleeper, msecs_to_jiffies(40));
     spin_lock_irqsave(&lock, flags);
}
spin_lock_irqrestore(&lock, flags);

Or something similar....

If those locks weren't there, we could use
wait_event_interruptible_timeout(). Should we create a locked version?

Thanks,
Nish
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/