2002-04-05 02:53:09

by Jean Tourrilhes

[permalink] [raw]
Subject: [QUESTION] How to use interruptible_sleep_on() without races ?

Hi,

I've got a problem on using interruptible_sleep_on(). I hope
you will help me fix that ;-)

I want to wait for a task to finish :
----------------------------------
if(my_condition == TRUE)
interruptible_sleep_on(&my_wait_queue);
----------------------------------

Then, at some point, a timer/BH/soft-irq will do :
-------------------------------
my_condition == FALSE;
wake_up_interruptible(&my_wait_queue);
-------------------------------

It seems straightforward, but it doesn't work. There is a race
condition between the test of the condition and the call to
sleep_on().
I looked at it in every possible way, and I don't see how it
is possible to use safely interruptible_sleep_on(). And I wonder :
what's the point of having a function in the kernel if you can't use
it safely ?
As a matter of fact, the TCP code doesn't use
interruptible_sleep_on() but use some complex code around schedule()
(and there must be a simpler and cleaner solution for such a simple
problem).

Any comments ?

Jean


2002-04-05 03:03:21

by Alan

[permalink] [raw]
Subject: Re: [QUESTION] How to use interruptible_sleep_on() without races ?

> if(my_condition == TRUE)
> interruptible_sleep_on(&my_wait_queue);
> ----------------------------------
>
> Then, at some point, a timer/BH/soft-irq will do :
> -------------------------------
> my_condition == FALSE;
> wake_up_interruptible(&my_wait_queue);
>
> I looked at it in every possible way, and I don't see how it
> is possible to use safely interruptible_sleep_on(). And I wonder :

It isnt for interrupt stuff - its going back to the old kernel behaviour
when it used to be usable

> interruptible_sleep_on() but use some complex code around schedule()
> (and there must be a simpler and cleaner solution for such a simple
> problem).

Actually the code it uses is clean, slightly verbose but clean. It puts
the phases in the right order and that fixes the race cleanly. You
could just use completions in that case or you could use

wait_event_interruptible(&my_wait_queue, my_condition==FALSE)

which is a macro that generates the right stuff.

Alan

2002-04-05 03:09:11

by Jean Tourrilhes

[permalink] [raw]
Subject: Re: [QUESTION] How to use interruptible_sleep_on() without races ?

On Fri, Apr 05, 2002 at 04:20:04AM +0100, Alan Cox wrote:
> >
> > I looked at it in every possible way, and I don't see how it
> > is possible to use safely interruptible_sleep_on(). And I wonder :
>
> It isnt for interrupt stuff - its going back to the old kernel behaviour
> when it used to be usable

So, maybe it would be a nice idea to remove it from the 2.5.X
kernel to force a "spring cleanup" of the old code. If it's no longer
usable and only confusing, it should be purged...

> Actually the code it uses is clean, slightly verbose but clean. It puts
> the phases in the right order and that fixes the race cleanly. You
> could just use completions in that case or you could use
>
> wait_event_interruptible(&my_wait_queue, my_condition==FALSE)
>
> which is a macro that generates the right stuff.

And it might even want to be defined in include/linux/sched.h
as a replacement for interruptible_sleep_on(). It seems like a generic
need, and I would feel much safer if one of the guru wrote it properly
for me ;-)

> Alan

Regards,

Jean

2002-04-05 04:50:10

by Linus Torvalds

[permalink] [raw]
Subject: Re: [QUESTION] How to use interruptible_sleep_on() without races ?

In article <[email protected]>,
Jean Tourrilhes <[email protected]> wrote:
>On Fri, Apr 05, 2002 at 04:20:04AM +0100, Alan Cox wrote:
>> >
>> > I looked at it in every possible way, and I don't see how it
>> > is possible to use safely interruptible_sleep_on(). And I wonder :
>>
>> It isnt for interrupt stuff - its going back to the old kernel behaviour
>> when it used to be usable
>
> So, maybe it would be a nice idea to remove it from the 2.5.X
>kernel to force a "spring cleanup" of the old code. If it's no longer
>usable and only confusing, it should be purged...

It's still usable, but under rather specific conditions, namely:
- both sleeper and waker in process context and with BKL held.
- OR if missing a wakeup isn't a horrible problem.

And there does seem to be a lot of legacy users out there.

I wouldn't mind a spring cleaning, but the fact is that right now in
2.5.x I'd rather have driver writers wake up to the fact that we had a
spring cleaning in the block layer several months ago, rather than
introduce a new one ;)

Linus

2002-04-05 07:55:00

by bert hubert

[permalink] [raw]
Subject: Re: [QUESTION] How to use interruptible_sleep_on() without races ?

On Fri, Apr 05, 2002 at 04:51:06AM +0000, Linus Torvalds wrote:

> I wouldn't mind a spring cleaning, but the fact is that right now in
> 2.5.x I'd rather have driver writers wake up to the fact that we had a
> spring cleaning in the block layer several months ago, rather than
> introduce a new one ;)

http://www.ibiblio.org/Dave/Dr-Fun/df200204/df20020402.jpg

Couldn't resist :-)

--
http://www.PowerDNS.com Versatile DNS Software & Services
http://www.tk the dot in .tk
http://lartc.org Linux Advanced Routing & Traffic Control HOWTO

2002-04-05 12:41:52

by Alan

[permalink] [raw]
Subject: Re: [QUESTION] How to use interruptible_sleep_on() without races ?

> > could just use completions in that case or you could use
> >
> > wait_event_interruptible(&my_wait_queue, my_condition==FALSE)
> >
> > which is a macro that generates the right stuff.
>
> And it might even want to be defined in include/linux/sched.h

It is 8)

Alan

2002-04-05 20:17:25

by Jean Tourrilhes

[permalink] [raw]
Subject: Re: [QUESTION] How to use interruptible_sleep_on() without races ?

On Fri, Apr 05, 2002 at 01:58:44PM +0100, Alan Cox wrote:
> > > could just use completions in that case or you could use
> > >
> > > wait_event_interruptible(&my_wait_queue, my_condition==FALSE)
> > >
> > > which is a macro that generates the right stuff.
> >
> > And it might even want to be defined in include/linux/sched.h
>
> It is 8)
>
> Alan

*Blush*. Yes it is.

Jean