sleep under spinlock, sequencer.c, 2.6.12.5

sleep under spinlock, sequencer.c, 2.6.12.5

[Peter T. Breuer]

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).


This code found during an analysis of about 1.5 million lines of kernel
code by the static kernel code analyser at

   ftp://oboe.it.uc3m.es/pub/Programs/c-1.2.*.tgz

(and yes, I am the author - if anyone wants to help please contact me).

Peter

Re: sleep under spinlock, sequencer.c, 2.6.12.5

[Alan Cox]

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.

Re: sleep under spinlock, sequencer.c, 2.6.12.5

[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
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

Re: sleep under spinlock, sequencer.c, 2.6.12.5

[Peter T. Breuer]

"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;

(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"?

>      spin_lock_irqsave(&lock, flags);
> }
> spin_lock_irqrestore(&lock, flags);


Peter

Re: sleep under spinlock, sequencer.c, 2.6.12.5

[Nish Aravamudan]

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