Re: sleep under spinlock, sequencer.c, 2.6.12.5

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

On 8/22/05, Peter T. Breuer <ptb@inv.it.uc3m.es> wrote:
> "Also sprach Nish Aravamudan:"
> > 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
> 
> Well spotted.
> 
> > seconds, but actually sleep for 40 milliseconds 3*HZ times
> > (potentially)?
> 
> I presume it should be
> 
>       n -= HZ/25;

Well that's the problem; you're presuming that an (eventual)
schedule_timeout(HZ/25) call would actually sleep for HZ/25 jiffies.
More than likely, though, it may sleep a little longer. Generally,
code that is trying to sleep up to a certain time from now should use
time_after() or time_before().

> (and "n > 0", of course).
> 
> > 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));
> 
> Well, you'd know. Is there something there really not taken care of by
> "HZ"?

1) Makes this code consistent with other users in the kernel (Although
I have tried to reduce the number of users of the sleep_on() family).

2) If HZ eventually is allowed to take other values (e.g., 864 for
x86), then HZ/25 leads to rounding issues. msecs_to_jiffies() takes
care of those issues *and* makes it a little clear what you're doing,
IMO.

Thanks,
Nish