Clarification required about select vs wake_up race condition

Clarification required about select vs wake_up race condition

[Ravinandan Arakali (rarakali)]

Hi,
I am facing following problem and was wondering if somebody could help
me out.
Our char driver(pretty much like all other char drivers) does a
poll_wait() 
and returns status depending on whether data is available to be read.
Even though some data is available to be read(verified using one of our
internal
commands), the select() never wakes up, inspite of any no. of messages
sent.

To understand this, I was looking at the code of select vs
wake_up_interruptible().
I feel I am misunderstanding some part of the kernel code but will be
glad if
somebody can point it out.

My understanding:
The do_select() sets the state of task to TASK_INTERRUPTIBLE and calls
the driver's
poll entry point. In our poll(), let's say immediately after we
determine that there's 
nothing to be read, some data arrives causing a wake_up_interruptible()
on another CPU. 
The wake up happens in the context of process sending the data. Since
the receiving 
process was already added to the list of listeners, from looking at the
code of 
try_to_wake_up(), it looks like it can set the state of the receiving
process to 
TASK_RUNNING(I don't see any lock preventing this). After this happens,
the receiving
process goes to sleep (because of schedule_timeout called by do_select)
but
state is still set to TASK_RUNNING. In this state, when another message
arrives,
the wake_up_interruptible will not wake the process because of following
code in
try_to_wake_up() ?

old_state = p->state;
if (!(old_state & state))
   goto out;

The above situation seems simplistic so I'm wondering what I am missing
here ?
Advice from gurus is highly appreciated.

Thanks,
Ravi

Re: Clarification required about select vs wake_up race condition

[Arjan van de Ven]

On Mon, 2007-03-12 at 12:44 -0700, Ravinandan Arakali (rarakali) wrote:
> Hi,
> I am facing following problem and was wondering if somebody could help
> me out.
> Our char driver(pretty much like all other char drivers) does a
> poll_wait() 


you forgot to include your (full) sourcecode or a pointer to that...

RE: Clarification required about select vs wake_up race condition

[Ravinandan Arakali (rarakali)]

Hi,
Our driver is not in the kernel tree yet.
The code snippet from poll entry point is as follows:

    mask = 0;
    poll_wait(filp, &(sap_desc->recv_any_wq), poll_table);
    /* We don't have yet a waiting queue for writers... */
    lock_mts();
    if ((sap_desc->recv_q.first != NULL) &&
        (block_ssync_event(sap_desc->recv_q.first) == FALSE)) {
        /* The queue can be read. */
        mask |= POLLIN | POLLRDNORM;
    }
    unlock_mts();
    //MTS_HOOK(mts_syscall_exit,__FUNCTION__,mask);
    return mask; 

The wake_up_interruptible(&(sap_desc->recv_any_wq)) is executed
from the sending process.

Hope this is sufficient. As I mentioned in my previous mail, what I
am trying to understand better is the kernel code path of do_select() 
versus wake_up_interruptible.

Thanks,
Ravi

-----Original Message-----
From: Arjan van de Ven [mailto:arjan@infradead.org] 
Sent: Wednesday, March 14, 2007 4:08 AM
To: Ravinandan Arakali (rarakali)
Cc: linux-kernel@vger.kernel.org
Subject: Re: Clarification required about select vs wake_up race
condition

On Mon, 2007-03-12 at 12:44 -0700, Ravinandan Arakali (rarakali) wrote:
> Hi,
> I am facing following problem and was wondering if somebody could help

> me out.
> Our char driver(pretty much like all other char drivers) does a
> poll_wait()


you forgot to include your (full) sourcecode or a pointer to that...

Re: Clarification required about select vs wake_up race condition

[Dmitry Adamushko]

On 12/03/07, Ravinandan Arakali (rarakali) <rarakali@cisco.com> wrote:
> Hi,
> I am facing following problem and was wondering if somebody could help
> me out.
> Our char driver(pretty much like all other char drivers) does a
> poll_wait()
> and returns status depending on whether data is available to be read.
> Even though some data is available to be read(verified using one of our
> internal
> commands), the select() never wakes up, inspite of any no. of messages
> sent.
>
> To understand this, I was looking at the code of select vs
> wake_up_interruptible().
> I feel I am misunderstanding some part of the kernel code but will be
> glad if
> somebody can point it out.
>
> My understanding:
> The do_select() sets the state of task to TASK_INTERRUPTIBLE and calls
> the driver's
> poll entry point. In our poll(), let's say immediately after we
> determine that there's
> nothing to be read, some data arrives causing a wake_up_interruptible()
> on another CPU.
> The wake up happens in the context of process sending the data. Since
> the receiving
> process was already added to the list of listeners, from looking at the
> code of
> try_to_wake_up(), it looks like it can set the state of the receiving
> process to
> TASK_RUNNING(I don't see any lock preventing this). After this happens,
> the receiving
> process goes to sleep (because of schedule_timeout called by do_select)
> but
> state is still set to TASK_RUNNING.

No, it's not going to sleep then.

The effect of schedule() being called with current->state ==
TASK_RUNNING is a re-scheduling to another task with a higher prio (if
any) or just getting back (iow, the task doesn't lose a cpu). For both
cases, the task is on the runqueue.

/ Look how/when deactivate_task() is called in schedule() /

Maybe there is a race in your code between (1) how you check "data is
available" in poll and (2) a part that sets this fact (data is
available)...


-- 
Best regards,
Dmitry Adamushko

RE: Clarification required about select vs wake_up race condition

[Ravinandan Arakali (rarakali)]

Dmitry,
Thanks for the explanation. That would mean that if
process is not put to sleep, it will go back to
top of for() loop in do_select(), set state back
to TASK_INTERRUPTIBLE and re-check for incoming data.

Ravi 

-----Original Message-----
From: Dmitry Adamushko [mailto:dmitry.adamushko@gmail.com] 
Sent: Wednesday, March 14, 2007 10:08 AM
To: Ravinandan Arakali (rarakali)
Cc: Linux Kernel
Subject: Re: Clarification required about select vs wake_up race
condition

On 12/03/07, Ravinandan Arakali (rarakali) <rarakali@cisco.com> wrote:
> Hi,
> I am facing following problem and was wondering if somebody could help

> me out.
> Our char driver(pretty much like all other char drivers) does a
> poll_wait()
> and returns status depending on whether data is available to be read.
> Even though some data is available to be read(verified using one of 
> our internal commands), the select() never wakes up, inspite of any 
> no. of messages sent.
>
> To understand this, I was looking at the code of select vs 
> wake_up_interruptible().
> I feel I am misunderstanding some part of the kernel code but will be 
> glad if somebody can point it out.
>
> My understanding:
> The do_select() sets the state of task to TASK_INTERRUPTIBLE and calls

> the driver's poll entry point. In our poll(), let's say immediately 
> after we determine that there's nothing to be read, some data arrives 
> causing a wake_up_interruptible() on another CPU.
> The wake up happens in the context of process sending the data. Since 
> the receiving process was already added to the list of listeners, from

> looking at the code of try_to_wake_up(), it looks like it can set the 
> state of the receiving process to TASK_RUNNING(I don't see any lock 
> preventing this). After this happens, the receiving process goes to 
> sleep (because of schedule_timeout called by do_select) but state is 
> still set to TASK_RUNNING.

No, it's not going to sleep then.

The effect of schedule() being called with current->state ==
TASK_RUNNING is a re-scheduling to another task with a higher prio (if
any) or just getting back (iow, the task doesn't lose a cpu). For both
cases, the task is on the runqueue.

/ Look how/when deactivate_task() is called in schedule() /

Maybe there is a race in your code between (1) how you check "data is
available" in poll and (2) a part that sets this fact (data is
available)...


--
Best regards,
Dmitry Adamushko