Should we use preempt_disable() in sleep_on_common()?

Should we use preempt_disable() in sleep_on_common()?

[liu pf]

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

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

Re: Should we use preempt_disable() in sleep_on_common()?

[Arjan van de Ven]

On Thu, 3 Dec 2009 14:49:14 +0800
liu pf <kernelfans@gmail.com> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[Dmitry Torokhov]

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 <kernelfans@gmail.com> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[Dmitry Torokhov]

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 <kernelfans@gmail.com> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[Dmitry Adamushko]

2009/12/3 liu pf <kernelfans@gmail.com>:
> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[liu pf]

On Thu, Dec 3, 2009 at 3:57 PM, Dmitry Adamushko
<dmitry.adamushko@gmail.com> wrote:
> 2009/12/3 liu pf <kernelfans@gmail.com>:
>> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[liu pf]

On Thu, Dec 3, 2009 at 3:12 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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 <kernelfans@gmail.com> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[Dmitry Torokhov]

On Thu, Dec 03, 2009 at 04:15:49PM +0800, liu pf wrote:
> On Thu, Dec 3, 2009 at 3:12 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> 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 <kernelfans@gmail.com> 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

Re: Should we use preempt_disable() in sleep_on_common()?

[liu pf]

Thank you very much


On Thu, Dec 3, 2009 at 4:22 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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
>> <dmitry.torokhov@gmail.com> 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 <kernelfans@gmail.com> 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
>