[PATCH 2/3] stop_machine: dequeue work before signal completion

[PATCH 2/3] stop_machine: dequeue work before signal completion

[Hillf Danton]

As handled by the kernel thread, work is dequeued first for further actions.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/kernel/stop_machine.c	Wed Feb  6 19:57:12 2013
+++ b/kernel/stop_machine.c	Wed Feb  6 20:02:12 2013
@@ -334,23 +334,24 @@ static int __cpuinit cpu_stop_cpu_callba
 #ifdef CONFIG_HOTPLUG_CPU
 	case CPU_UP_CANCELED:
 	case CPU_POST_DEAD:
-	{
-		struct cpu_stop_work *work;
-
 		sched_set_stop_task(cpu, NULL);
 		/* kill the stopper */
 		kthread_stop(stopper->thread);
 		/* drain remaining works */
 		spin_lock_irq(&stopper->lock);
-		list_for_each_entry(work, &stopper->works, list)
+		while (!list_empty(&stopper->works)) {
+			struct cpu_stop_work *work;
+			work = list_first_entry(&stopper->works,
+					struct cpu_stop_work, list);
+			list_del_init(&work->list);
 			cpu_stop_signal_done(work->done, false, 0);
+		}
 		stopper->enabled = false;
 		spin_unlock_irq(&stopper->lock);
 		/* release the stopper */
 		put_task_struct(stopper->thread);
 		stopper->thread = NULL;
 		break;
-	}
 #endif
 	}

--

Re: [PATCH 2/3] stop_machine: dequeue work before signal completion

[Tejun Heo]

On Wed, Feb 06, 2013 at 08:38:43PM +0800, Hillf Danton wrote:
> As handled by the kernel thread, work is dequeued first for further actions.

Ditto as the previous patch.

> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/kernel/stop_machine.c	Wed Feb  6 19:57:12 2013
> +++ b/kernel/stop_machine.c	Wed Feb  6 20:02:12 2013
> @@ -334,23 +334,24 @@ static int __cpuinit cpu_stop_cpu_callba
>  #ifdef CONFIG_HOTPLUG_CPU
>  	case CPU_UP_CANCELED:
>  	case CPU_POST_DEAD:
> -	{
> -		struct cpu_stop_work *work;
> -
>  		sched_set_stop_task(cpu, NULL);
>  		/* kill the stopper */
>  		kthread_stop(stopper->thread);
>  		/* drain remaining works */
>  		spin_lock_irq(&stopper->lock);
> -		list_for_each_entry(work, &stopper->works, list)
> +		while (!list_empty(&stopper->works)) {
> +			struct cpu_stop_work *work;
> +			work = list_first_entry(&stopper->works,
> +					struct cpu_stop_work, list);
> +			list_del_init(&work->list);
>  			cpu_stop_signal_done(work->done, false, 0);
> +		}
>  		stopper->enabled = false;
>  		spin_unlock_irq(&stopper->lock);

Why does this matter?  It's inside spinlock.  What's being made better
by this change?

-- 
tejun

Re: [PATCH 2/3] stop_machine: dequeue work before signal completion

[Namhyung Kim]

Hi Tejun and Hillf,

On Wed, 6 Feb 2013 10:47:49 -0800, Tejun Heo wrote:
> On Wed, Feb 06, 2013 at 08:38:43PM +0800, Hillf Danton wrote:
>> As handled by the kernel thread, work is dequeued first for further actions.
>
> Ditto as the previous patch.
>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>> 
>> --- a/kernel/stop_machine.c	Wed Feb  6 19:57:12 2013
>> +++ b/kernel/stop_machine.c	Wed Feb  6 20:02:12 2013
>> @@ -334,23 +334,24 @@ static int __cpuinit cpu_stop_cpu_callba
>>  #ifdef CONFIG_HOTPLUG_CPU
>>  	case CPU_UP_CANCELED:
>>  	case CPU_POST_DEAD:
>> -	{
>> -		struct cpu_stop_work *work;
>> -
>>  		sched_set_stop_task(cpu, NULL);
>>  		/* kill the stopper */
>>  		kthread_stop(stopper->thread);
>>  		/* drain remaining works */
>>  		spin_lock_irq(&stopper->lock);
>> -		list_for_each_entry(work, &stopper->works, list)
>> +		while (!list_empty(&stopper->works)) {
>> +			struct cpu_stop_work *work;
>> +			work = list_first_entry(&stopper->works,
>> +					struct cpu_stop_work, list);
>> +			list_del_init(&work->list);
>>  			cpu_stop_signal_done(work->done, false, 0);
>> +		}
>>  		stopper->enabled = false;
>>  		spin_unlock_irq(&stopper->lock);
>
> Why does this matter?  It's inside spinlock.  What's being made better
> by this change?

IIUC the work should be deleted from the list, otherwise it'd trigger
BUG_ON when the cpu gets online again.

Thanks,
Namhyung

Re: [PATCH 2/3] stop_machine: dequeue work before signal completion

[Tejun Heo]

Hello,

On Wed, Feb 6, 2013 at 6:21 PM, Namhyung Kim <namhyung@kernel.org> wrote:
>> Why does this matter?  It's inside spinlock.  What's being made better
>> by this change?
>
> IIUC the work should be deleted from the list, otherwise it'd trigger
> BUG_ON when the cpu gets online again.

Ah, okay, the original code was missing list_del_init(), so we end up
with trashy work list if CPU goes down while there are pending work
items which will trigger BUG_ON() later when the CPU comes back on.

Hillf, can you please redo the patch description? I can't tell what
the patch is about from the description at all. If it's a bug fix,
describe the bug and maybe accompany with oops trace if possible, and
then describe how it's fixed.

Thanks.

-- 
tejun

Re: [PATCH 2/3] stop_machine: dequeue work before signal completion

[Hillf Danton]

On Thu, Feb 7, 2013 at 10:29 AM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Feb 6, 2013 at 6:21 PM, Namhyung Kim <namhyung@kernel.org> wrote:
>>> Why does this matter?  It's inside spinlock.  What's being made better
>>> by this change?
>>
>> IIUC the work should be deleted from the list, otherwise it'd trigger
>> BUG_ON when the cpu gets online again.
>
> Ah, okay, the original code was missing list_del_init(), so we end up
> with trashy work list if CPU goes down while there are pending work
> items which will trigger BUG_ON() later when the CPU comes back on.
>
> Hillf, can you please redo the patch description?

Sure, thanks;)
Hillf