* [PATCH 2/3] stop_machine: dequeue work before signal completion
@ 2013-02-06 12:38 Hillf Danton
2013-02-06 18:47 ` Tejun Heo
0 siblings, 1 reply; 5+ messages in thread
From: Hillf Danton @ 2013-02-06 12:38 UTC (permalink / raw)
To: Rusty Russell, Tejun Heo; +Cc: Andrew Morton, Ingo Molnar, LKML, 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
}
--
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] stop_machine: dequeue work before signal completion
2013-02-06 12:38 [PATCH 2/3] stop_machine: dequeue work before signal completion Hillf Danton
@ 2013-02-06 18:47 ` Tejun Heo
2013-02-07 2:21 ` Namhyung Kim
0 siblings, 1 reply; 5+ messages in thread
From: Tejun Heo @ 2013-02-06 18:47 UTC (permalink / raw)
To: Hillf Danton; +Cc: Rusty Russell, Andrew Morton, Ingo Molnar, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] stop_machine: dequeue work before signal completion
2013-02-06 18:47 ` Tejun Heo
@ 2013-02-07 2:21 ` Namhyung Kim
2013-02-07 2:29 ` Tejun Heo
0 siblings, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2013-02-07 2:21 UTC (permalink / raw)
To: Tejun Heo; +Cc: Hillf Danton, Rusty Russell, Andrew Morton, Ingo Molnar, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] stop_machine: dequeue work before signal completion
2013-02-07 2:21 ` Namhyung Kim
@ 2013-02-07 2:29 ` Tejun Heo
2013-02-07 11:53 ` Hillf Danton
0 siblings, 1 reply; 5+ messages in thread
From: Tejun Heo @ 2013-02-07 2:29 UTC (permalink / raw)
To: Namhyung Kim
Cc: Hillf Danton, Rusty Russell, Andrew Morton, Ingo Molnar, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] stop_machine: dequeue work before signal completion
2013-02-07 2:29 ` Tejun Heo
@ 2013-02-07 11:53 ` Hillf Danton
0 siblings, 0 replies; 5+ messages in thread
From: Hillf Danton @ 2013-02-07 11:53 UTC (permalink / raw)
To: Tejun Heo; +Cc: Namhyung Kim, Rusty Russell, Andrew Morton, Ingo Molnar, LKML
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-02-07 11:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-06 12:38 [PATCH 2/3] stop_machine: dequeue work before signal completion Hillf Danton
2013-02-06 18:47 ` Tejun Heo
2013-02-07 2:21 ` Namhyung Kim
2013-02-07 2:29 ` Tejun Heo
2013-02-07 11:53 ` Hillf Danton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.