Messages in this thread | | | Date | Thu, 29 Aug 2013 17:20:21 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 01/14] kthread: Fix invalid wakeup in kthreadd |
| |
On Thu, Aug 29, 2013 at 09:57:36PM +0800, Libin wrote: > If kthreadd is preempted at(or before) location a, and the other thread, > such as calling kthread_create_on_node(), adds a list item to > the kthread_create_list followed with wake_up_process(kthread). After that > when kthreadd is re-scheduled, calling set_current_state to set itself as > state TASK_INTERRUPTIBLE, if it is preempted again after that and before > __set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem. > ------------------------ > kthreadd() > ------------------------ > ... > for (;;) { > //location a > set_current_state(TASK_INTERRUPTIBLE); > if (list_empty(&kthread_create_list)) { > //location b > schedule(); > //location c > } > __set_current_state(TASK_RUNNING); > //location d > ... > ------------------------ > kthread_create_on_node() > ------------------------ > ... > spin_lock(&kthread_create_lock); > list_add_tail(&create.list, &kthread_create_list); > spin_unlock(&kthread_create_lock); > ... > wake_up_process(kthreadd_task); > ... > > To solve this problem, using preempt_disable() to bound the operaion that > setting the task state and the conditions(set by the wake thread) validation. > ------------------------ > kthreadd() > ------------------------ > ... > for (;;) { > preempt_disable(); > set_current_state(TASK_INTERRUPTIBLE); > if (list_empty(&kthread_create_list)) { > preempt_enable(); > schedule(); > preempt_disable(); > } > ... > > Signed-off-by: Libin <huawei.libin@huawei.com> > --- > kernel/kthread.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/kernel/kthread.c b/kernel/kthread.c > index 760e86d..25c3fed 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -456,10 +456,15 @@ int kthreadd(void *unused) > current->flags |= PF_NOFREEZE; > > for (;;) { > + preempt_disable(); > set_current_state(TASK_INTERRUPTIBLE); > - if (list_empty(&kthread_create_list))
And this one already has the list_empty() check after the call to set_current_state(), so no change should be needed here.
On the other hand, if your testing shows that you are losing wakeups with this exact code, please let us know!
Thanx, Paul
> + if (list_empty(&kthread_create_list)) { > + preempt_enable(); > schedule(); > + preempt_disable(); > + } > __set_current_state(TASK_RUNNING); > + preempt_enable(); > > spin_lock(&kthread_create_lock); > while (!list_empty(&kthread_create_list)) { > -- > 1.8.2.1 > >
| |