2009-12-03 06:49:10

by Pingfan Liu

[permalink] [raw]
Subject: Should we use preempt_disable() in sleep_on_common()?

Hi:

I am puzzled with the following scenario. Could anyone enlighten me?

Thanks
pfliu


static long __sched
sleep_on_common(wait_queue_head_t *q, int state, long timeout)
{
unsigned long flags;
wait_queue_t wait;

init_waitqueue_entry(&wait, current);

__set_current_state(state);

==========>suppose that after task A set state=TASK_INTERRUPTIBLE
, it is preempted by task B.

spin_lock_irqsave(&q->lock, flags);
...............................................................
}

asmlinkage void __sched schedule(void)
{
.......................................................................................................
if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
if (unlikely(signal_pending_state(prev->state, prev)))
prev->state = TASK_RUNNING;
else
deactivate_task(rq, prev, 1);
=============>This will remove task A from rq, but there are no
wait queue referring to A, so we lose A.
switch_count = &prev->nvcsw;
}

..................................................................................................
}


2009-12-03 07:05:34

by Arjan van de Ven

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

On Thu, 3 Dec 2009 14:49:14 +0800
liu pf <[email protected]> wrote:

> Hi:
>
> I am puzzled with the following scenario. Could anyone enlighten me?


sleep_on family of APIs is very racy and just cannot be used correctly;
I'm not surprised that there's a preempt race in it, but trust me, it's
not the biggest race... never ever use these APIs!!!


--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org

2009-12-03 07:12:43

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

On Wed, Dec 02, 2009 at 11:07:46PM -0800, Arjan van de Ven wrote:
> On Thu, 3 Dec 2009 14:49:14 +0800
> liu pf <[email protected]> wrote:
>
> > Hi:
> >
> > I am puzzled with the following scenario. Could anyone enlighten me?
>
>
> sleep_on family of APIs is very racy and just cannot be used correctly;
> I'm not surprised that there's a preempt race in it, but trust me, it's
> not the biggest race... never ever use these APIs!!!
>
>

BTW, why do we still have them? I checked couple and they don't seem to
be used...

--
Dmitry

2009-12-03 07:15:08

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

On Wed, Dec 02, 2009 at 11:12:44PM -0800, Dmitry Torokhov wrote:
> On Wed, Dec 02, 2009 at 11:07:46PM -0800, Arjan van de Ven wrote:
> > On Thu, 3 Dec 2009 14:49:14 +0800
> > liu pf <[email protected]> wrote:
> >
> > > Hi:
> > >
> > > I am puzzled with the following scenario. Could anyone enlighten me?
> >
> >
> > sleep_on family of APIs is very racy and just cannot be used correctly;
> > I'm not surprised that there's a preempt race in it, but trust me, it's
> > not the biggest race... never ever use these APIs!!!
> >
> >
>
> BTW, why do we still have them? I checked couple and they don't seem to
> be used...
>

Ah, my bad, I now see interruptible_sleep_on used all over drivers/char...

--
Dmitry

2009-12-03 07:57:27

by Dmitry Adamushko

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

2009/12/3 liu pf <[email protected]>:
> Hi:
>
> I am puzzled with the following scenario. Could anyone enlighten me?
>
> Thanks
> pfliu
>
>
> static long __sched
> sleep_on_common(wait_queue_head_t *q, int state, long timeout)
> {
> unsigned long flags;
> wait_queue_t wait;
>
> init_waitqueue_entry(&wait, current);
>
> __set_current_state(state);
>
> ==========>suppose that after task A set state=TASK_INTERRUPTIBLE
> , it is preempted by task B.
>
> spin_lock_irqsave(&q->lock, flags);
> ...............................................................
> }
>
> asmlinkage void __sched schedule(void)
> {
> .......................................................................................................
> if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
> if (unlikely(signal_pending_state(prev->state, prev)))
> prev->state = TASK_RUNNING;
> else
> deactivate_task(rq, prev, 1);
> =============>This will remove task A from rq, but there are no
> wait queue referring to A, so we lose A.
> switch_count = &prev->nvcsw;
> }

In this case, (preempt_count() & PREEMPT_ACTIVE) == 1(see
preempt_schedule_irq() and other use-cases of PREEMPT_ACTIVE) so we
don't enter this block.

i.e. a preempted task stays on its queue (with state != TASK_RUNNING
but that doesn't really matter).


-- Dmitry

2009-12-03 08:01:33

by Pingfan Liu

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

On Thu, Dec 3, 2009 at 3:57 PM, Dmitry Adamushko
<[email protected]> wrote:
> 2009/12/3 liu pf <[email protected]>:
>> Hi:
>>
>> I am puzzled with the following scenario. Could anyone enlighten me?
>>
>> Thanks
>> pfliu
>>
>>
>> static long __sched
>> sleep_on_common(wait_queue_head_t *q, int state, long timeout)
>> {
>>    unsigned long flags;
>>    wait_queue_t wait;
>>
>>    init_waitqueue_entry(&wait, current);
>>
>>    __set_current_state(state);
>>
>>    ==========>suppose that after task A  set state=TASK_INTERRUPTIBLE
>> , it is preempted by task B.
>>
>>    spin_lock_irqsave(&q->lock, flags);
>> ...............................................................
>> }
>>
>> asmlinkage void __sched schedule(void)
>> {
>> .......................................................................................................
>>    if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
>>        if (unlikely(signal_pending_state(prev->state, prev)))
>>            prev->state = TASK_RUNNING;
>>        else
>>            deactivate_task(rq, prev, 1);
>>       =============>This will remove task A from rq, but there are no
>> wait queue referring to A, so we lose A.
>>        switch_count = &prev->nvcsw;
>>    }
>
> In this case, (preempt_count() & PREEMPT_ACTIVE) == 1(see
> preempt_schedule_irq() and other use-cases of PREEMPT_ACTIVE) so we
> don't  enter this block.
>
> i.e. a preempted task stays on its queue (with state != TASK_RUNNING
> but that doesn't really matter).
>
>
> -- Dmitry
>


Thank you very much

2009-12-03 08:15:45

by Pingfan Liu

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

On Thu, Dec 3, 2009 at 3:12 PM, Dmitry Torokhov
<[email protected]> wrote:
> On Wed, Dec 02, 2009 at 11:07:46PM -0800, Arjan van de Ven wrote:
>> On Thu, 3 Dec 2009 14:49:14 +0800
>> liu pf <[email protected]> wrote:
>>
>> > Hi:
>> >
>> > I am puzzled with the following scenario. Could anyone enlighten me?
>>
>>
>> sleep_on family of APIs is very racy and just cannot be used correctly;
>> I'm not surprised that there's a preempt race in it, but trust me, it's
>> not the biggest race... never ever use these APIs!!!
>>
>>
>
> BTW, why do we still have them? I checked couple and they don't seem to
> be used...
>
> --
> Dmitry
>

Hi, what is the substitution for sleep_on family of APIs? Any sample code?

Thanks

2009-12-03 08:22:36

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

On Thu, Dec 03, 2009 at 04:15:49PM +0800, liu pf wrote:
> On Thu, Dec 3, 2009 at 3:12 PM, Dmitry Torokhov
> <[email protected]> wrote:
> > On Wed, Dec 02, 2009 at 11:07:46PM -0800, Arjan van de Ven wrote:
> >> On Thu, 3 Dec 2009 14:49:14 +0800
> >> liu pf <[email protected]> wrote:
> >>
> >> > Hi:
> >> >
> >> > I am puzzled with the following scenario. Could anyone enlighten me?
> >>
> >>
> >> sleep_on family of APIs is very racy and just cannot be used correctly;
> >> I'm not surprised that there's a preempt race in it, but trust me, it's
> >> not the biggest race... never ever use these APIs!!!
> >>
> >>
> >
> > BTW, why do we still have them? I checked couple and they don't seem to
> > be used...
> >
> > --
> > Dmitry
> >
>
> Hi, what is the substitution for sleep_on family of APIs? Any sample code?
>

wait_event() and friends. Just look up any non-ancient driver.

--
Dmitry

2009-12-03 08:53:24

by Pingfan Liu

[permalink] [raw]
Subject: Re: Should we use preempt_disable() in sleep_on_common()?

Thank you very much


On Thu, Dec 3, 2009 at 4:22 PM, Dmitry Torokhov
<[email protected]> wrote:
> On Thu, Dec 03, 2009 at 04:15:49PM +0800, liu pf wrote:
>> On Thu, Dec 3, 2009 at 3:12 PM, Dmitry Torokhov
>> <[email protected]> wrote:
>> > On Wed, Dec 02, 2009 at 11:07:46PM -0800, Arjan van de Ven wrote:
>> >> On Thu, 3 Dec 2009 14:49:14 +0800
>> >> liu pf <[email protected]> wrote:
>> >>
>> >> > Hi:
>> >> >
>> >> > I am puzzled with the following scenario. Could anyone enlighten me?
>> >>
>> >>
>> >> sleep_on family of APIs is very racy and just cannot be used correctly;
>> >> I'm not surprised that there's a preempt race in it, but trust me, it's
>> >> not the biggest race... never ever use these APIs!!!
>> >>
>> >>
>> >
>> > BTW, why do we still have them? I checked couple and they don't seem to
>> > be used...
>> >
>> > --
>> > Dmitry
>> >
>>
>> Hi, what is the substitution for  sleep_on family of APIs? Any sample code?
>>
>
> wait_event() and friends. Just look up any non-ancient driver.
>
> --
> Dmitry
>