[PATCH 00/14] Fix bug about invalid wake up problem

[PATCH 00/14] Fix bug about invalid wake up problem

[Libin]

1)Problem Description:
The prototype of invalid wakeup problem is as follows:
========================================================================
----------------------------
Consumer Thread
----------------------------
...
if (list_empty(&list)){
	//location a 
	set_current_state(TASK_INTERRUPTIBLE);
	schedule();
}
...
----------------------------
Producer Thread
----------------------------
...
list_add_tail(&item, &list);
wake_up_process(A);
...
========================================================================

Invalid wakeup problem occurs in preemptable kernel environment. The list
is empty initially, if consumer is preempted after condition validated at
location a, and at this time producer is woken up, adding a node to the
list and calling wake_up_process to wake up consumer. After that when
consumer being scheduled by scheduler, it calls set_current_state to set
itself as state TASK_INTERRUPTIBLE, and calling schedule() to request
scheduled. After consumer being scheduled out, it has no condition to be
woken up, and causing invalid wakeup problem.

In most cases, the kernel codes use a form similar to the following:
--------------------------------------------
 ...
 //location a
 ...
 set_current_state(TASK_INTERRUPTIBLE);
 //location b
 while (list_empty(&product_list)){
	schedule();	//location c
	set_current_state(TASK_INTERRUPTIBLE);
	//location d
}
__set_current_state(TASK_RUNNING);
...
--------------------------------------------
At location a, consumer is preempted, and producer is scheduled,
adding item to product_list and waking up consumer. After consumer is
re-scheduled, calling set_current_state to set itself as state
TASK_INTERRUPTIBLE, if it is preempted again before __set_current_state(TASK_RUNNING),
also trigger the invalid wakeup problem that the consumer will not be scheduled
and the item be added by producer can't be consumed.

The following circumstance will also trigger this issue:
At location c, consumer is scheduled out, and be preempted after calling
set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.

2)Test:
I have written a test module to trigger the problem by adding some
synchronization condition. I will post it in the form of an attachment soon.

Test result as follows:
[103135.332683] wakeup_test: create two kernel threads - producer & consumer
[103135.332686] wakeup_test: module loaded successfully
[103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
[103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
trigger an invalid wakeup problem!

3)Solution:
Using preempt_disable() to bound the operaion that setting the task state
and the conditions(set by the wake thread) validation.

----------------------------------------------
...
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
while (list_empty(&product_list)){
	preempt_enable();
	schedule();
	preempt_disable();
	set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
preempt_enable();
...
----------------------------------------------
And it also can be solved as follows:
----------------------------------------------
...
preempt_disable();
while (list_empty(&product_list)){
	set_current_state(TASK_INTERRUPTIBLE);
	preempt_enable();
	schedule();
	preempt_disable();	
}
preempt_enable();
...
----------------------------------------------

Libin (14):
  kthread: Fix invalid wakeup in kthreadd
  audit: Fix invalid wakeup in kauditd_thread
  audit: Fix invalid wakeup in wait_for_auditd
  exit: Fix invalid wakeup in do_wait
  hrtimer: Fix invalid wakeup in do_nanosleep
  irq: Fix invalid wakeup in irq_wait_for_interrupt
  module: Fix invalid wakeup in wait_for_zero_refcount
  namespace: Fix invalid wakeup in zap_pid_ns_processes
  rcutree: Fix invalid wakeup in rcu_wait
  time: Fix invalid wakeup in alarmtimer_do_nsleep
  trace: Fix invalid wakeup in wait_to_die
  trace: Fix invalid wakeup in ring_buffer_consumer_thread
  workqueue: Fix invalid wakeup in rescuer_thread
  klist: Fix invalid wakeup in klist_remove

 kernel/audit.c                       | 11 ++++-
 kernel/exit.c                        |  7 ++-
 kernel/hrtimer.c                     |  7 ++-
 kernel/irq/manage.c                  |  5 ++
 kernel/kthread.c                     |  7 ++-
 kernel/module.c                      |  6 ++-
 kernel/pid_namespace.c               |  4 ++
 kernel/rcutree.h                     |  4 ++
 kernel/time/alarmtimer.c             |  7 ++-
 kernel/trace/ring_buffer_benchmark.c | 14 ++++++
 kernel/workqueue.c                   | 92 +++++++++++++++++++-----------------
 lib/klist.c                          |  4 ++
 12 files changed, 118 insertions(+), 50 deletions(-)

-- 
1.8.2.1

[PATCH 01/14] kthread: Fix invalid wakeup in kthreadd

[Libin]

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))
+		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

Re: [PATCH 01/14] kthread: Fix invalid wakeup in kthreadd

[Paul E. McKenney]

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

[PATCH 02/14] audit: Fix invalid wakeup in kauditd_thread

[Libin]

If this thread is preempted at(or before) location a, and the other thread
set the condition followed with wake_up_process. After that
when this thread 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.
----------------
kauditd_thread()
----------------
while (!kthread_should_stop()) {
	...
		//location a
		set_current_state(TASK_INTERRUPTIBLE);
	...

		if (!skb_queue_len(&audit_skb_queue)) {
			//location b
			try_to_freeze();
			schedule();
			//location c
		}

	__set_current_state(TASK_RUNNING);
	//location d
	...
}

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.
----------------
kauditd_thread()
----------------
while (!kthread_should_stop()) {
	...
		preempt_disable();
	set_current_state(TASK_INTERRUPTIBLE);
	...

		if (!skb_queue_len(&audit_skb_queue)) {
			preempt_enable();
			try_to_freeze();
			schedule();
			preempt_disable();
		}

	__set_current_state(TASK_RUNNING);
	preempt_enable();
	...
}

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/audit.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 91e53d0..a7d1346 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -455,15 +455,19 @@ static int kauditd_thread(void *dummy)
 				audit_printk_skb(skb);
 			continue;
 		}
+		preempt_disable();
 		set_current_state(TASK_INTERRUPTIBLE);
 		add_wait_queue(&kauditd_wait, &wait);
 
 		if (!skb_queue_len(&audit_skb_queue)) {
+			preempt_enable();
 			try_to_freeze();
 			schedule();
+			preempt_disable();
 		}
 
 		__set_current_state(TASK_RUNNING);
+		preempt_enable();
 		remove_wait_queue(&kauditd_wait, &wait);
 	}
 	return 0;
-- 
1.8.2.1

[PATCH 03/14] audit: Fix invalid wakeup in wait_for_auditd

[Libin]

If this thread is preempted at(or before) location a, and the other thread
set the condition followed with wake_up_process. After that
when this thread is re-scheduled, calling set_current_state to set itself as
state TASK_UNINTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
----------------
wait_for_auditd()
----------------
...
//location a
set_current_state(TASK_UNINTERRUPTIBLE);
...

if (audit_backlog_limit &&
			skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
	//location b
	schedule_timeout(sleep_time);
	//location c

__set_current_state(TASK_RUNNING);
//location d
...

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.
----------------
wait_for_auditd()
----------------
...
preempt_disable();
set_current_state(TASK_UNINTERRUPTIBLE);
...

if (audit_backlog_limit &&
			skb_queue_len(&audit_skb_queue) > audit_backlog_limit) {
	preempt_enable();
	schedule_timeout(sleep_time);
	preempt_disable();
}

__set_current_state(TASK_RUNNING);
preempt_enable();
...

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/audit.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index a7d1346..48d2f76 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1060,14 +1060,19 @@ static inline void audit_get_stamp(struct audit_context *ctx,
 static void wait_for_auditd(unsigned long sleep_time)
 {
 	DECLARE_WAITQUEUE(wait, current);
+	preempt_disable();
 	set_current_state(TASK_UNINTERRUPTIBLE);
 	add_wait_queue(&audit_backlog_wait, &wait);
 
 	if (audit_backlog_limit &&
-	    skb_queue_len(&audit_skb_queue) > audit_backlog_limit)
+	    skb_queue_len(&audit_skb_queue) > audit_backlog_limit) {
+		preempt_enable();
 		schedule_timeout(sleep_time);
+		preempt_disable();
+	}
 
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 	remove_wait_queue(&audit_backlog_wait, &wait);
 }
 
-- 
1.8.2.1

[PATCH 04/14] exit: Fix invalid wakeup in do_wait

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/exit.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index a949819..1ea7736 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1521,7 +1521,6 @@ repeat:
 	   (!wo->wo_pid || hlist_empty(&wo->wo_pid->tasks[wo->wo_type])))
 		goto notask;
 
-	set_current_state(TASK_INTERRUPTIBLE);
 	read_lock(&tasklist_lock);
 	tsk = current;
 	do {
@@ -1539,16 +1538,20 @@ repeat:
 	read_unlock(&tasklist_lock);
 
 notask:
+	preempt_disable();
+	set_current_state(TASK_INTERRUPTIBLE);
 	retval = wo->notask_error;
 	if (!retval && !(wo->wo_flags & WNOHANG)) {
 		retval = -ERESTARTSYS;
 		if (!signal_pending(current)) {
+			preempt_enable();
 			schedule();
 			goto repeat;
 		}
 	}
-end:
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
+end:
 	remove_wait_queue(&current->signal->wait_chldexit, &wo->child_wait);
 	return retval;
 }
-- 
1.8.2.1

[PATCH 05/14] hrtimer: Fix invalid wakeup in do_nanosleep

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/hrtimer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 383319b..a7845ba 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1539,14 +1539,18 @@ static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mod
 {
 	hrtimer_init_sleeper(t, current);
 
+	preempt_disable();
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 		hrtimer_start_expires(&t->timer, mode);
 		if (!hrtimer_active(&t->timer))
 			t->task = NULL;
 
-		if (likely(t->task))
+		if (likely(t->task)) {
+			preempt_enable();
 			freezable_schedule();
+			preempt_disable();
+		}
 
 		hrtimer_cancel(&t->timer);
 		mode = HRTIMER_MODE_ABS;
@@ -1554,6 +1558,7 @@ static int __sched do_nanosleep(struct hrtimer_sleeper *t, enum hrtimer_mode mod
 	} while (t->task && !signal_pending(current));
 
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 
 	return t->task == NULL;
 }
-- 
1.8.2.1

Re: [PATCH 05/14] hrtimer: Fix invalid wakeup in do_nanosleep

[Thomas Gleixner]

On Thu, 29 Aug 2013, Libin wrote:

> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread 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.

do_nanosleep() is only called from sys_nanosleep() and
sys_clock_nanosleep() user space interfaces.

So the task is not going to be woken up by some magic other thread
except by a signal. The latter is handled by the scheduler which will
return with state running. So what kind of problem are you trying to
solve?

Thanks,

	tglx



 

[PATCH 06/14] irq: Fix invalid wakeup in irq_wait_for_interrupt

[Libin]

If this thread is preempted at(or before) location a, and the other thread
set the condition(kthread_stop). After that when this thread 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.
-----------------------
irq_wait_for_interrupt()
-----------------------
...
//location a
set_current_state(TASK_INTERRUPTIBLE);
//location b
while (!kthread_should_stop()) {

	if (test_and_clear_bit(IRQTF_RUNTHREAD,
					   &action->thread_flags)) {
			__set_current_state(TASK_RUNNING);
			return 0;
	}
	schedule();//location c
	set_current_state(TASK_INTERRUPTIBLE);
	//location d
}
__set_current_state(TASK_RUNNING);
...

The following circumstance will also trigger this issue:
At location c, consumer is scheduled out, and be preempted after calling
set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/irq/manage.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 514bcfd..09cb02f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -655,6 +655,7 @@ static irqreturn_t irq_nested_primary_handler(int irq, void *dev_id)
 
 static int irq_wait_for_interrupt(struct irqaction *action)
 {
+	preempt_disable();
 	set_current_state(TASK_INTERRUPTIBLE);
 
 	while (!kthread_should_stop()) {
@@ -662,12 +663,16 @@ static int irq_wait_for_interrupt(struct irqaction *action)
 		if (test_and_clear_bit(IRQTF_RUNTHREAD,
 				       &action->thread_flags)) {
 			__set_current_state(TASK_RUNNING);
+			preempt_enable();
 			return 0;
 		}
+		preempt_enable();
 		schedule();
+		preempt_disable();
 		set_current_state(TASK_INTERRUPTIBLE);
 	}
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 	return -1;
 }
 
-- 
1.8.2.1

Re: [PATCH 06/14] irq: Fix invalid wakeup in irq_wait_for_interrupt

[Thomas Gleixner]

On Thu, 29 Aug 2013, Libin wrote:
> If this thread is preempted at(or before) location a, and the other thread
> set the condition(kthread_stop). After that when this thread 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.
> -----------------------
> irq_wait_for_interrupt()
> -----------------------
> ...
> //location a
> set_current_state(TASK_INTERRUPTIBLE);
> //location b
> while (!kthread_should_stop()) {
> 
> 	if (test_and_clear_bit(IRQTF_RUNTHREAD,
> 					   &action->thread_flags)) {
> 			__set_current_state(TASK_RUNNING);
> 			return 0;
> 	}
> 	schedule();//location c
> 	set_current_state(TASK_INTERRUPTIBLE);
> 	//location d
> }
> __set_current_state(TASK_RUNNING);
> ...
> 
> The following circumstance will also trigger this issue:
> At location c, consumer is scheduled out, and be preempted after calling
> set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.
> 
> 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.

This is a completely pointless exercise. The irq threads are only
woken from the core code itself and not by some random other threads.

The core code already handles the legit wakeups correctly without
having the need for preemption disable.

Thanks,

	tglx

[PATCH 07/14] module: Fix invalid wakeup in wait_for_zero_refcount

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.
-----------------------
wait_for_zero_refcount()
-----------------------
...
for (;;) {
	pr_debug("Looking at refcount...\n");
	set_current_state(TASK_UNINTERRUPTIBLE);
	if (module_refcount(mod) == 0)
		break;
	schedule();
}
__set_current_state(TASK_RUNNING);
...

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.
-----------------------
wait_for_zero_refcount()
-----------------------
...
preempt_disable();
for (;;) {
	pr_debug("Looking at refcount...\n");
	set_current_state(TASK_UNINTERRUPTIBLE);
	if (module_refcount(mod) == 0)
		break;
	preempt_enable();
	schedule();
	preempt_disable();
}
__set_current_state(TASK_RUNNING);
preempt_enable();
...

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/module.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 2069158..22064e9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -816,14 +816,18 @@ static void wait_for_zero_refcount(struct module *mod)
 {
 	/* Since we might sleep for some time, release the mutex first */
 	mutex_unlock(&module_mutex);
+	preempt_disable();
 	for (;;) {
 		pr_debug("Looking at refcount...\n");
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (module_refcount(mod) == 0)
 			break;
+		preempt_enable();
 		schedule();
+		preempt_disable();
 	}
-	current->state = TASK_RUNNING;
+	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 	mutex_lock(&module_mutex);
 }
 
-- 
1.8.2.1

[PATCH 08/14] namespace: Fix invalid wakeup in zap_pid_ns_processes

[Libin]

If thread is preempted before calling set_current_state(TASK_UNINTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread is re-scheduled, calling set_current_state to set itself
as state TASK_UNINTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
----------------------
zap_pid_ns_processes()
----------------------
...
for (;;) {
	set_current_state(TASK_UNINTERRUPTIBLE);
	if (pid_ns->nr_hashed == init_pids)
		break;
	schedule();
}
__set_current_state(TASK_RUNNING);
...

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/pid_namespace.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 6917e8e..e3696a1 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -227,13 +227,17 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 	 * sys_wait4() above can't reap the TASK_DEAD children.
 	 * Make sure they all go away, see free_pid().
 	 */
+	preempt_disable();
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (pid_ns->nr_hashed == init_pids)
 			break;
+		preempt_enable();
 		schedule();
+		preempt_disable();
 	}
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 
 	if (pid_ns->reboot)
 		current->signal->group_exit_code = pid_ns->reboot;
-- 
1.8.2.1

[PATCH 09/14] rcutree: Fix invalid wakeup in rcu_wait

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/rcutree.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/rcutree.h b/kernel/rcutree.h
index b383258..e3f1278 100644
--- a/kernel/rcutree.h
+++ b/kernel/rcutree.h
@@ -357,13 +357,17 @@ struct rcu_data {
 
 #define rcu_wait(cond)							\
 do {									\
+	preempt_disable();					\
 	for (;;) {							\
 		set_current_state(TASK_INTERRUPTIBLE);			\
 		if (cond)						\
 			break;						\
+		preempt_enable();				\
 		schedule();						\
+		preempt_disable();				\
 	}								\
 	__set_current_state(TASK_RUNNING);				\
+	preempt_enable();				\
 } while (0)
 
 /*
-- 
1.8.2.1

Re: [PATCH 09/14] rcutree: Fix invalid wakeup in rcu_wait

[Paul E. McKenney]

On Thu, Aug 29, 2013 at 09:57:44PM +0800, Libin wrote:
> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread 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.
> 
> 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.
> 
> Signed-off-by: Libin <huawei.libin@huawei.com>
> ---
>  kernel/rcutree.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/rcutree.h b/kernel/rcutree.h
> index b383258..e3f1278 100644
> --- a/kernel/rcutree.h
> +++ b/kernel/rcutree.h
> @@ -357,13 +357,17 @@ struct rcu_data {
> 
>  #define rcu_wait(cond)							\
>  do {									\
> +	preempt_disable();					\
>  	for (;;) {							\
>  		set_current_state(TASK_INTERRUPTIBLE);			\
>  		if (cond)						\
>  			break;						\

Here also the condition check follows the call to set_current_state(),
so this patch should not be needed.

Again, if you are seeing lost wakeups on this exact code path, please
let me know.

Please re-check your other patches.  If they really do follow the
buggy pattern you called out in your patch 0, namely the test preceding
the call to set_current_state(), please send patches that restore the
correct ordering.

Your use of preemption is not fixing anything.  It is at best hiding
the problem by making it less likely to occur.

							Thanx, Paul

> +		preempt_enable();				\
>  		schedule();						\
> +		preempt_disable();				\
>  	}								\
>  	__set_current_state(TASK_RUNNING);				\
> +	preempt_enable();				\
>  } while (0)
> 
>  /*
> -- 
> 1.8.2.1
> 
> 

[PATCH 10/14] time: Fix invalid wakeup in alarmtimer_do_nsleep

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/time/alarmtimer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index eec50fc..624f2ed 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -629,16 +629,21 @@ static enum alarmtimer_restart alarmtimer_nsleep_wakeup(struct alarm *alarm,
 static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp)
 {
 	alarm->data = (void *)current;
+	preempt_disable();
 	do {
 		set_current_state(TASK_INTERRUPTIBLE);
 		alarm_start(alarm, absexp);
-		if (likely(alarm->data))
+		if (likely(alarm->data)) {
+			preempt_enable();
 			schedule();
+			preempt_disable();
+		}
 
 		alarm_cancel(alarm);
 	} while (alarm->data && !signal_pending(current));
 
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 
 	return (alarm->data == NULL);
 }
-- 
1.8.2.1

Re: [PATCH 10/14] time: Fix invalid wakeup in alarmtimer_do_nsleep

[Thomas Gleixner]

On Thu, 29 Aug 2013, Libin wrote:

> If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
> and the other thread set the condition followed with wake_up_process. After
> that when this thread 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.
> 
> 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.

See the reply to the hrtimer patch. You're solving a non issue.

Thanks,

	tglx

[PATCH 11/14] trace: Fix invalid wakeup in wait_to_die

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition(kthread_stop). After that when this
thread 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.
-------------
wait_to_die()
-------------
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
	schedule();
	set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);

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.
-------------
wait_to_die()
-------------
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
	preempt_enable();
	schedule();
	preempt_disable();
	set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
preempt_enable();

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/trace/ring_buffer_benchmark.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index a5457d5..3c0bc03 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -359,12 +359,16 @@ static void ring_buffer_producer(void)
 
 static void wait_to_die(void)
 {
+	preempt_disable();
 	set_current_state(TASK_INTERRUPTIBLE);
 	while (!kthread_should_stop()) {
+		preempt_enable();
 		schedule();
+		preempt_disable();
 		set_current_state(TASK_INTERRUPTIBLE);
 	}
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 }
 
 static int ring_buffer_consumer_thread(void *arg)
-- 
1.8.2.1

[PATCH 12/14] trace: Fix invalid wakeup in ring_buffer_consumer_thread

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/trace/ring_buffer_benchmark.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index 3c0bc03..4f43a96 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -172,6 +172,9 @@ static enum event_status read_page(int cpu)
 	return EVENT_FOUND;
 }
 
+/*
+ * Called in preemptation disabled environment.
+ */
 static void ring_buffer_consumer(void)
 {
 	/* toggle between reading pages and events */
@@ -204,9 +207,12 @@ static void ring_buffer_consumer(void)
 		if (reader_finish)
 			break;
 
+		preempt_enable();
 		schedule();
 		__set_current_state(TASK_RUNNING);
+		preempt_disable();
 	}
+
 	reader_finish = 0;
 	complete(&read_done);
 }
@@ -373,6 +379,7 @@ static void wait_to_die(void)
 
 static int ring_buffer_consumer_thread(void *arg)
 {
+	preempt_disable();
 	while (!kthread_should_stop() && !kill_test) {
 		complete(&read_start);
 
@@ -382,10 +389,13 @@ static int ring_buffer_consumer_thread(void *arg)
 		if (kthread_should_stop() || kill_test)
 			break;
 
+		preempt_enable();
 		schedule();
 		__set_current_state(TASK_RUNNING);
+		preempt_disable();
 	}
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 
 	if (kill_test)
 		wait_to_die();
-- 
1.8.2.1

[PATCH 13/14] workqueue: Fix invalid wakeup in rescuer_thread

[Libin]

If thread is preempted before calling set_current_state(TASK_INTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread 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.

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.

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 kernel/workqueue.c | 92 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 7f5d4be..2dcdd30 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2358,63 +2358,69 @@ static int rescuer_thread(void *__rescuer)
 	 * doesn't participate in concurrency management.
 	 */
 	rescuer->task->flags |= PF_WQ_WORKER;
-repeat:
-	set_current_state(TASK_INTERRUPTIBLE);
+	for (;;) {
 
-	if (kthread_should_stop()) {
+		if (kthread_should_stop()) {
+			rescuer->task->flags &= ~PF_WQ_WORKER;
+			return 0;
+		}
+
+		preempt_disable();
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (list_empty(&wq->maydays)) {
+			preempt_enable();
+			schedule();
+			preempt_disable();
+		}
 		__set_current_state(TASK_RUNNING);
-		rescuer->task->flags &= ~PF_WQ_WORKER;
-		return 0;
-	}
+		preempt_enable();
 
-	/* see whether any pwq is asking for help */
-	spin_lock_irq(&wq_mayday_lock);
+		/* see whether any pwq is asking for help */
+		spin_lock_irq(&wq_mayday_lock);
 
-	while (!list_empty(&wq->maydays)) {
-		struct pool_workqueue *pwq = list_first_entry(&wq->maydays,
+		while (!list_empty(&wq->maydays)) {
+			struct pool_workqueue *pwq = list_first_entry(&wq->maydays,
 					struct pool_workqueue, mayday_node);
-		struct worker_pool *pool = pwq->pool;
-		struct work_struct *work, *n;
+			struct worker_pool *pool = pwq->pool;
+			struct work_struct *work, *n;
 
-		__set_current_state(TASK_RUNNING);
-		list_del_init(&pwq->mayday_node);
+			list_del_init(&pwq->mayday_node);
 
-		spin_unlock_irq(&wq_mayday_lock);
+			spin_unlock_irq(&wq_mayday_lock);
 
-		/* migrate to the target cpu if possible */
-		worker_maybe_bind_and_lock(pool);
-		rescuer->pool = pool;
+			/* migrate to the target cpu if possible */
+			worker_maybe_bind_and_lock(pool);
+			rescuer->pool = pool;
 
-		/*
-		 * Slurp in all works issued via this workqueue and
-		 * process'em.
-		 */
-		WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
-		list_for_each_entry_safe(work, n, &pool->worklist, entry)
-			if (get_work_pwq(work) == pwq)
-				move_linked_works(work, scheduled, &n);
+			/*
+			 * Slurp in all works issued via this workqueue and
+			 * process'em.
+			 */
+			WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
+			list_for_each_entry_safe(work, n, &pool->worklist, entry)
+				if (get_work_pwq(work) == pwq)
+					move_linked_works(work, scheduled, &n);
 
-		process_scheduled_works(rescuer);
+			process_scheduled_works(rescuer);
 
-		/*
-		 * Leave this pool.  If keep_working() is %true, notify a
-		 * regular worker; otherwise, we end up with 0 concurrency
-		 * and stalling the execution.
-		 */
-		if (keep_working(pool))
-			wake_up_worker(pool);
+			/*
+			 * Leave this pool.  If keep_working() is %true, notify a
+			 * regular worker; otherwise, we end up with 0 concurrency
+			 * and stalling the execution.
+			 */
+			if (keep_working(pool))
+				wake_up_worker(pool);
 
-		rescuer->pool = NULL;
-		spin_unlock(&pool->lock);
-		spin_lock(&wq_mayday_lock);
-	}
+			rescuer->pool = NULL;
+			spin_unlock(&pool->lock);
+			spin_lock(&wq_mayday_lock);
+		}
 
-	spin_unlock_irq(&wq_mayday_lock);
+		spin_unlock_irq(&wq_mayday_lock);
 
-	/* rescuers should never participate in concurrency management */
-	WARN_ON_ONCE(!(rescuer->flags & WORKER_NOT_RUNNING));
-	schedule();
-	goto repeat;
+		/* rescuers should never participate in concurrency management */
+		WARN_ON_ONCE(!(rescuer->flags & WORKER_NOT_RUNNING));
+	}
 }
 
 struct wq_barrier {
-- 
1.8.2.1

[PATCH 14/14] klist: Fix invalid wakeup in klist_remove

[Libin]

If thread is preempted before calling set_current_state(TASK_UNINTERRUPTIBLE),
and the other thread set the condition followed with wake_up_process. After
that when this thread is re-scheduled, calling set_current_state to set itself
as state TASK_UNINTERRUPTIBLE, if it is preempted again after that and before
__set_current_state(TASK_RUNNING), it triggers the invalid wakeup problem.
--------------
klist_remove()
--------------
...
for (;;) {
	set_current_state(TASK_UNINTERRUPTIBLE);
	if (waiter.woken)
		break;
	schedule();
}
__set_current_state(TASK_RUNNING);
...

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.
--------------
klist_remove()
--------------
...
preempt_disable();
for (;;) {
	set_current_state(TASK_UNINTERRUPTIBLE);
	if (waiter.woken)
		break;
	preempt_enable();
	schedule();
	preempt_disable();
}
__set_current_state(TASK_RUNNING);
preempt_enable();
...

Signed-off-by: Libin <huawei.libin@huawei.com>
---
 lib/klist.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/klist.c b/lib/klist.c
index 358a368..e7c7208 100644
--- a/lib/klist.c
+++ b/lib/klist.c
@@ -249,13 +249,17 @@ void klist_remove(struct klist_node *n)
 
 	klist_del(n);
 
+	preempt_disable();
 	for (;;) {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (waiter.woken)
 			break;
+		preempt_enable();
 		schedule();
+		preempt_disable();
 	}
 	__set_current_state(TASK_RUNNING);
+	preempt_enable();
 }
 EXPORT_SYMBOL_GPL(klist_remove);
 
-- 
1.8.2.1

Re: [PATCH 00/14] Fix bug about invalid wake up problem

[Libin]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On 2013/8/29 21:57, Libin wrote:
....
> 2)Test:
> I have written a test module to trigger the problem by adding some
> synchronization condition. I will post it in the form of an attachment soon.
> 
> Test result as follows:
> [103135.332683] wakeup_test: create two kernel threads - producer & consumer
> [103135.332686] wakeup_test: module loaded successfully
> [103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
> [103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
> trigger an invalid wakeup problem!
> 
....







[-- Attachment #2: wakeup_test.c --]
[-- Type: text/plain, Size: 3040 bytes --]

/*
 * wakeup_test.c -- Linux kernel invalid wake up problem simulation test module
 *
 * Written By: Libin <huawei.libin@huawei.com>
 *
 * History
 * -------
 */
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/kthread.h>
#include <linux/sched.h>
#include <linux/list.h>
#include <linux/slab.h>
#include <linux/completion.h>
#include <linux/timer.h>

#define NAME	"wakeup_test"
#define WAIT_TIMEOUT	HZ*30
static LIST_HEAD(product_list);
struct product_struct{
	int data;
	struct list_head list;
};
static struct task_struct *producer, *consumer;
static struct completion done;

static int producer_thread(void *unused)
{
	struct product_struct *product;
	product = (struct product_struct *)kmalloc(sizeof (struct product_struct), GFP_KERNEL);	
	product->data = 1;	

	list_add_tail(&product->list, &product_list);

	wake_up_process(consumer);
	printk(KERN_INFO "%s: kthread producer try to wake up the kthread consumer\n", NAME);
	complete(&done); /* NOTE: added for problem trigger simulation */

	while (!kthread_should_stop()){
		set_current_state(TASK_INTERRUPTIBLE);
		schedule_timeout(HZ);
	}

	printk(KERN_INFO "%s: kthread producer exit\n", NAME);
	return 0;	
}
static void simulate_preempted(void)
{
	schedule();
}

static void wakeup_wait_timeout(unsigned long unused)
{
	printk(KERN_ERR "%s: kthread consumer have waited for %ds, "
		"indicating trigger an invalid wakeup problem!\n", NAME, WAIT_TIMEOUT/HZ);
}
static int consumer_thread(void *unused)
{
	struct timer_list wakeup_wait_timer;
	setup_timer(&wakeup_wait_timer, wakeup_wait_timeout, (unsigned long)NULL);
	wait_for_completion(&done); /* NOTE: added for problem trigger simulation */

	mod_timer(&wakeup_wait_timer, jiffies + WAIT_TIMEOUT);
	set_current_state(TASK_INTERRUPTIBLE);

	simulate_preempted(); /* NOTE: added for problem trigger simulation */
	while (list_empty(&product_list)){
		schedule();
		set_current_state(TASK_INTERRUPTIBLE);
	}
	__set_current_state(TASK_RUNNING);

	del_timer_sync(&wakeup_wait_timer);
	if (kthread_should_stop()){
		goto out;
	}

	if (!list_empty(&product_list)){
		printk(KERN_INFO "%s: kthread consumer be woken up successfully, all right!\n", NAME);
	}

	while (!kthread_should_stop()){
		set_current_state(TASK_INTERRUPTIBLE);
		schedule_timeout(HZ);
	}

out:
	printk(KERN_INFO "%s: kthread consumer exit\n", NAME);
	return 0;	
}

static int __init wakeup_test_init(void)
{
	producer = kthread_create(producer_thread, NULL, "producer");
	consumer = kthread_create(consumer_thread, NULL, "consumer");
	init_completion(&done);
	
	wake_up_process(producer);
	wake_up_process(consumer);
	printk(KERN_INFO "%s: create two kernel threads - producer & consumer\n", NAME);
	printk(KERN_INFO "%s: module loaded successfully\n", NAME);
	return 0;
}
static void __exit wakeup_test_exit(void)
{
	kthread_stop(producer);
	kthread_stop(consumer);
	printk(KERN_INFO "%s: module unloaded successfully\n", NAME);
}
module_init(wakeup_test_init);
module_exit(wakeup_test_exit);
MODULE_LICENSE("GPL");

Re: [PATCH 00/14] Fix bug about invalid wake up problem

[Tejun Heo]

Hello, again.

On Thu, Aug 29, 2013 at 10:08:13PM +0800, Libin wrote:
...
> static void simulate_preempted(void)
> {
> 	schedule();
> }

Gees... of course, your test code will stall.  Preemption !=
schedule().  Please take a look at PREEMPT_ACTIVE handling in
__schedule().

Thanks.

-- 
tejun

Re: [PATCH 00/14] Fix bug about invalid wake up problem

[Tejun Heo]

Hello, Libin.

I'm completely confused by this series....

On Thu, Aug 29, 2013 at 09:57:35PM +0800, Libin wrote:
> 1)Problem Description:
> The prototype of invalid wakeup problem is as follows:
> ========================================================================
> ----------------------------
> Consumer Thread
> ----------------------------
> ...
> if (list_empty(&list)){
> 	//location a 
> 	set_current_state(TASK_INTERRUPTIBLE);
> 	schedule();
> }
> ...
> ----------------------------
> Producer Thread
> ----------------------------
> ...
> list_add_tail(&item, &list);
> wake_up_process(A);
> ...

This is of course broken.  set_current_state() should of course come
*before* the conditions is checked.  This is just plain broken code.

> In most cases, the kernel codes use a form similar to the following:
> --------------------------------------------
>  ...
>  //location a
>  ...
>  set_current_state(TASK_INTERRUPTIBLE);
>  //location b
>  while (list_empty(&product_list)){
> 	schedule();	//location c
> 	set_current_state(TASK_INTERRUPTIBLE);
> 	//location d
> }
> __set_current_state(TASK_RUNNING);
> ...
> --------------------------------------------
> At location a, consumer is preempted, and producer is scheduled,
> adding item to product_list and waking up consumer. After consumer is
> re-scheduled, calling set_current_state to set itself as state
> TASK_INTERRUPTIBLE, if it is preempted again before __set_current_state(TASK_RUNNING),
> also trigger the invalid wakeup problem that the consumer will not be scheduled

Why?  Getting preempted != calling schedule().  A preempted task will
be rescheduled *regardless* of ifs current->state; otherwise, the
whole kernel is severely broken.  Tasks never get deactivated while
preempted.

> and the item be added by producer can't be consumed.
> 
> The following circumstance will also trigger this issue:
> At location c, consumer is scheduled out, and be preempted after calling
> set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.

What?

> 2)Test:
> I have written a test module to trigger the problem by adding some
> synchronization condition. I will post it in the form of an attachment soon.
> 
> Test result as follows:
> [103135.332683] wakeup_test: create two kernel threads - producer & consumer
> [103135.332686] wakeup_test: module loaded successfully
> [103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
> [103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
> trigger an invalid wakeup problem!

The most interesting question here is "what were you testing?".  Can
you please post the test code?

For the whole series,

 Nacked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 00/14] Fix bug about invalid wake up problem

[Libin]

Hi all,
I'm so sorry, please ignore this patch set!
I have realized that there is no this problem in our kernel.
Preempt_schedule() has set PREEMPT_ACTIVE before calling
__schedule() and __schedule will check it if current state is not
TASK_RUNNING, avoiding this preemption.

Libin

On 2013/8/29 21:57, Libin wrote:
> 1)Problem Description:
> The prototype of invalid wakeup problem is as follows:
> ========================================================================
> ----------------------------
> Consumer Thread
> ----------------------------
> ...

Re: [PATCH 00/14] Fix bug about invalid wake up problem

[Paul E. McKenney]

On Thu, Aug 29, 2013 at 09:57:35PM +0800, Libin wrote:
> 1)Problem Description:
> The prototype of invalid wakeup problem is as follows:
> ========================================================================
> ----------------------------
> Consumer Thread
> ----------------------------
> ...
> if (list_empty(&list)){
> 	//location a 
> 	set_current_state(TASK_INTERRUPTIBLE);

You need the list_empty() test here, not above.  Or you at least need
to retest here.

The reason for this is that set_current_state() has a memory
barrier following the assignment, which matches the memory barrier in
try_to_wake_up().  This means that if the test at this point sees the
list as empty, then the checks in try_to_wake_up() must see the task in
TASK_INTERRUPTIBLE state and must therefore wake it up.

							Thanx, Paul

> 	schedule();
> }
> ...
> ----------------------------
> Producer Thread
> ----------------------------
> ...
> list_add_tail(&item, &list);
> wake_up_process(A);
> ...
> ========================================================================
> 
> Invalid wakeup problem occurs in preemptable kernel environment. The list
> is empty initially, if consumer is preempted after condition validated at
> location a, and at this time producer is woken up, adding a node to the
> list and calling wake_up_process to wake up consumer. After that when
> consumer being scheduled by scheduler, it calls set_current_state to set
> itself as state TASK_INTERRUPTIBLE, and calling schedule() to request
> scheduled. After consumer being scheduled out, it has no condition to be
> woken up, and causing invalid wakeup problem.
> 
> In most cases, the kernel codes use a form similar to the following:
> --------------------------------------------
>  ...
>  //location a
>  ...
>  set_current_state(TASK_INTERRUPTIBLE);
>  //location b
>  while (list_empty(&product_list)){
> 	schedule();	//location c
> 	set_current_state(TASK_INTERRUPTIBLE);
> 	//location d
> }
> __set_current_state(TASK_RUNNING);
> ...
> --------------------------------------------
> At location a, consumer is preempted, and producer is scheduled,
> adding item to product_list and waking up consumer. After consumer is
> re-scheduled, calling set_current_state to set itself as state
> TASK_INTERRUPTIBLE, if it is preempted again before __set_current_state(TASK_RUNNING),
> also trigger the invalid wakeup problem that the consumer will not be scheduled
> and the item be added by producer can't be consumed.
> 
> The following circumstance will also trigger this issue:
> At location c, consumer is scheduled out, and be preempted after calling
> set_current_state(TASK_INTERRUPTIBLE) when it be re-schdeuled.
> 
> 2)Test:
> I have written a test module to trigger the problem by adding some
> synchronization condition. I will post it in the form of an attachment soon.
> 
> Test result as follows:
> [103135.332683] wakeup_test: create two kernel threads - producer & consumer
> [103135.332686] wakeup_test: module loaded successfully
> [103135.332692] wakeup_test: kthread producer try to wake up the kthread consumer
> [103165.299865] wakeup_test: kthread consumer have waited for 30s, indicating
> trigger an invalid wakeup problem!
> 
> 3)Solution:
> Using preempt_disable() to bound the operaion that setting the task state
> and the conditions(set by the wake thread) validation.
> 
> ----------------------------------------------
> ...
> preempt_disable();
> set_current_state(TASK_INTERRUPTIBLE);
> while (list_empty(&product_list)){
> 	preempt_enable();
> 	schedule();
> 	preempt_disable();
> 	set_current_state(TASK_INTERRUPTIBLE);
> }
> __set_current_state(TASK_RUNNING);
> preempt_enable();
> ...
> ----------------------------------------------
> And it also can be solved as follows:
> ----------------------------------------------
> ...
> preempt_disable();
> while (list_empty(&product_list)){
> 	set_current_state(TASK_INTERRUPTIBLE);
> 	preempt_enable();
> 	schedule();
> 	preempt_disable();	
> }
> preempt_enable();
> ...
> ----------------------------------------------
> 
> Libin (14):
>   kthread: Fix invalid wakeup in kthreadd
>   audit: Fix invalid wakeup in kauditd_thread
>   audit: Fix invalid wakeup in wait_for_auditd
>   exit: Fix invalid wakeup in do_wait
>   hrtimer: Fix invalid wakeup in do_nanosleep
>   irq: Fix invalid wakeup in irq_wait_for_interrupt
>   module: Fix invalid wakeup in wait_for_zero_refcount
>   namespace: Fix invalid wakeup in zap_pid_ns_processes
>   rcutree: Fix invalid wakeup in rcu_wait
>   time: Fix invalid wakeup in alarmtimer_do_nsleep
>   trace: Fix invalid wakeup in wait_to_die
>   trace: Fix invalid wakeup in ring_buffer_consumer_thread
>   workqueue: Fix invalid wakeup in rescuer_thread
>   klist: Fix invalid wakeup in klist_remove
> 
>  kernel/audit.c                       | 11 ++++-
>  kernel/exit.c                        |  7 ++-
>  kernel/hrtimer.c                     |  7 ++-
>  kernel/irq/manage.c                  |  5 ++
>  kernel/kthread.c                     |  7 ++-
>  kernel/module.c                      |  6 ++-
>  kernel/pid_namespace.c               |  4 ++
>  kernel/rcutree.h                     |  4 ++
>  kernel/time/alarmtimer.c             |  7 ++-
>  kernel/trace/ring_buffer_benchmark.c | 14 ++++++
>  kernel/workqueue.c                   | 92 +++++++++++++++++++-----------------
>  lib/klist.c                          |  4 ++
>  12 files changed, 118 insertions(+), 50 deletions(-)
> 
> -- 
> 1.8.2.1
> 
>