Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On Wed, Sep 19, 2012 at 5:47 PM, Sasha Levin wrote: > Hi Thomas, > > On 07/16/2012 12:42 PM, Thomas Gleixner wrote: >> Provide a generic interface for setting up and tearing down percpu >> threads. >> >> On registration the threads for already online cpus are created and >> started. On deregistration (modules) the threads are stoppped. >> >> During hotplug operations the threads are created, started, parked and >> unparked. The datastructure for registration provides a pointer to >> percpu storage space and optional setup, cleanup, park, unpark >> functions. These functions are called when the thread state changes. >> >> Each implementation has to provide a function which is queried and >> returns whether the thread should run and the thread function itself. >> >> The core code handles all state transitions and avoids duplicated code >> in the call sites. >> >> Signed-off-by: Thomas Gleixner >> --- > > This patch seems to cause the following BUG() on KVM guests with large amount > of > VCPUs: > > [0.511760] [ cut here ] > [0.511761] kernel BUG at kernel/smpboot.c:134! > [0.511764] invalid opcode: [#3] PREEMPT SMP DEBUG_PAGEALLOC > [0.511779] CPU 0 > [0.511780] Pid: 70, comm: watchdog/10 Tainted: G D W > 3.6.0-rc6-next-20120919-sasha-1-gb54aafe #365 > [0.511783] RIP: 0010:[] [] > smpboot_thread_fn+0x196/0x2e0 > [0.511785] RSP: 0018:88000cf4bdd0 EFLAGS: 00010206 > [0.511786] RAX: RBX: 88000cf58000 RCX: > > [0.511787] RDX: RSI: 0001 RDI: > 0001 > [0.511788] RBP: 88000cf4be30 R08: R09: > 0001 > [0.511789] R10: R11: R12: > 88000cdb9ff0 > [0.511790] R13: 84c60920 R14: 000a R15: > 88000cf58000 > [0.511792] FS: () GS:88000d20() > knlGS: > [0.511794] CS: 0010 DS: ES: CR0: 8005003b > [0.511795] CR2: CR3: 04c26000 CR4: > 000406f0 > [0.511801] DR0: DR1: DR2: > > [0.511805] DR3: DR6: 0ff0 DR7: > 0400 > [0.511807] Process watchdog/10 (pid: 70, threadinfo 88000cf4a000, task > 88000cf58000) > [0.511808] Stack: > [0.511822] 88000cf4bfd8 88000cf4bfd8 > > [0.511833] 88000cf4be00 839eace5 88000cf4be30 > 88000cdd1c68 > [0.511844] 88000cdb9ff0 811414e0 > > [0.511845] Call Trace: > [0.511852] [] ? schedule+0x55/0x60 > [0.511857] [] ? __smpboot_create_thread+0xf0/0xf0 > [0.511863] [] kthread+0xe3/0xf0 > [0.511867] [] ? wait_for_common+0x143/0x180 > [0.511873] [] kernel_thread_helper+0x4/0x10 > [0.511878] [] ? retint_restore_args+0x13/0x13 > [0.511883] [] ? insert_kthread_work+0x90/0x90 > [0.511888] [] ? gs_change+0x13/0x13 > [0.511916] Code: 24 04 02 00 00 00 0f 1f 80 00 00 00 00 e8 b3 46 ff ff e9 > b6 > fe ff ff 66 0f 1f 44 00 00 45 8b 34 24 e8 ff 72 8a 00 41 39 c6 74 0a <0f> 0b > 0f > 1f 84 00 00 00 00 00 41 8b 44 24 04 85 c0 74 0f 83 f8 > [0.511919] RIP [] smpboot_thread_fn+0x196/0x2e0 > [0.511920] RSP > [0.511922] ---[ end trace 127920ef70923ae1 ]--- > > I'm starting the guest with numa=fake=10, so vcpu 0 ends up on the same (fake) > node as vcpu 10, and while digging into the bug, it seems that the issue is > that > vcpu10's thread gets scheduled on vcpu0. > > Beyond that I don't really understand what's wrong... Ping? Still seeing that with linux-next. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On Wed, Sep 19, 2012 at 5:47 PM, Sasha Levin levinsasha...@gmail.com wrote: Hi Thomas, On 07/16/2012 12:42 PM, Thomas Gleixner wrote: Provide a generic interface for setting up and tearing down percpu threads. On registration the threads for already online cpus are created and started. On deregistration (modules) the threads are stoppped. During hotplug operations the threads are created, started, parked and unparked. The datastructure for registration provides a pointer to percpu storage space and optional setup, cleanup, park, unpark functions. These functions are called when the thread state changes. Each implementation has to provide a function which is queried and returns whether the thread should run and the thread function itself. The core code handles all state transitions and avoids duplicated code in the call sites. Signed-off-by: Thomas Gleixner t...@linutronix.de --- This patch seems to cause the following BUG() on KVM guests with large amount of VCPUs: [0.511760] [ cut here ] [0.511761] kernel BUG at kernel/smpboot.c:134! [0.511764] invalid opcode: [#3] PREEMPT SMP DEBUG_PAGEALLOC [0.511779] CPU 0 [0.511780] Pid: 70, comm: watchdog/10 Tainted: G D W 3.6.0-rc6-next-20120919-sasha-1-gb54aafe #365 [0.511783] RIP: 0010:[81141676] [81141676] smpboot_thread_fn+0x196/0x2e0 [0.511785] RSP: 0018:88000cf4bdd0 EFLAGS: 00010206 [0.511786] RAX: RBX: 88000cf58000 RCX: [0.511787] RDX: RSI: 0001 RDI: 0001 [0.511788] RBP: 88000cf4be30 R08: R09: 0001 [0.511789] R10: R11: R12: 88000cdb9ff0 [0.511790] R13: 84c60920 R14: 000a R15: 88000cf58000 [0.511792] FS: () GS:88000d20() knlGS: [0.511794] CS: 0010 DS: ES: CR0: 8005003b [0.511795] CR2: CR3: 04c26000 CR4: 000406f0 [0.511801] DR0: DR1: DR2: [0.511805] DR3: DR6: 0ff0 DR7: 0400 [0.511807] Process watchdog/10 (pid: 70, threadinfo 88000cf4a000, task 88000cf58000) [0.511808] Stack: [0.511822] 88000cf4bfd8 88000cf4bfd8 [0.511833] 88000cf4be00 839eace5 88000cf4be30 88000cdd1c68 [0.511844] 88000cdb9ff0 811414e0 [0.511845] Call Trace: [0.511852] [839eace5] ? schedule+0x55/0x60 [0.511857] [811414e0] ? __smpboot_create_thread+0xf0/0xf0 [0.511863] [81135c13] kthread+0xe3/0xf0 [0.511867] [839eb463] ? wait_for_common+0x143/0x180 [0.511873] [839ef044] kernel_thread_helper+0x4/0x10 [0.511878] [839ed3b4] ? retint_restore_args+0x13/0x13 [0.511883] [81135b30] ? insert_kthread_work+0x90/0x90 [0.511888] [839ef040] ? gs_change+0x13/0x13 [0.511916] Code: 24 04 02 00 00 00 0f 1f 80 00 00 00 00 e8 b3 46 ff ff e9 b6 fe ff ff 66 0f 1f 44 00 00 45 8b 34 24 e8 ff 72 8a 00 41 39 c6 74 0a 0f 0b 0f 1f 84 00 00 00 00 00 41 8b 44 24 04 85 c0 74 0f 83 f8 [0.511919] RIP [81141676] smpboot_thread_fn+0x196/0x2e0 [0.511920] RSP 88000cf4bdd0 [0.511922] ---[ end trace 127920ef70923ae1 ]--- I'm starting the guest with numa=fake=10, so vcpu 0 ends up on the same (fake) node as vcpu 10, and while digging into the bug, it seems that the issue is that vcpu10's thread gets scheduled on vcpu0. Beyond that I don't really understand what's wrong... Ping? Still seeing that with linux-next. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
Hi Thomas, On 07/16/2012 12:42 PM, Thomas Gleixner wrote: > Provide a generic interface for setting up and tearing down percpu > threads. > > On registration the threads for already online cpus are created and > started. On deregistration (modules) the threads are stoppped. > > During hotplug operations the threads are created, started, parked and > unparked. The datastructure for registration provides a pointer to > percpu storage space and optional setup, cleanup, park, unpark > functions. These functions are called when the thread state changes. > > Each implementation has to provide a function which is queried and > returns whether the thread should run and the thread function itself. > > The core code handles all state transitions and avoids duplicated code > in the call sites. > > Signed-off-by: Thomas Gleixner > --- This patch seems to cause the following BUG() on KVM guests with large amount of VCPUs: [0.511760] [ cut here ] [0.511761] kernel BUG at kernel/smpboot.c:134! [0.511764] invalid opcode: [#3] PREEMPT SMP DEBUG_PAGEALLOC [0.511779] CPU 0 [0.511780] Pid: 70, comm: watchdog/10 Tainted: G D W 3.6.0-rc6-next-20120919-sasha-1-gb54aafe #365 [0.511783] RIP: 0010:[] [] smpboot_thread_fn+0x196/0x2e0 [0.511785] RSP: 0018:88000cf4bdd0 EFLAGS: 00010206 [0.511786] RAX: RBX: 88000cf58000 RCX: [0.511787] RDX: RSI: 0001 RDI: 0001 [0.511788] RBP: 88000cf4be30 R08: R09: 0001 [0.511789] R10: R11: R12: 88000cdb9ff0 [0.511790] R13: 84c60920 R14: 000a R15: 88000cf58000 [0.511792] FS: () GS:88000d20() knlGS: [0.511794] CS: 0010 DS: ES: CR0: 8005003b [0.511795] CR2: CR3: 04c26000 CR4: 000406f0 [0.511801] DR0: DR1: DR2: [0.511805] DR3: DR6: 0ff0 DR7: 0400 [0.511807] Process watchdog/10 (pid: 70, threadinfo 88000cf4a000, task 88000cf58000) [0.511808] Stack: [0.511822] 88000cf4bfd8 88000cf4bfd8 [0.511833] 88000cf4be00 839eace5 88000cf4be30 88000cdd1c68 [0.511844] 88000cdb9ff0 811414e0 [0.511845] Call Trace: [0.511852] [] ? schedule+0x55/0x60 [0.511857] [] ? __smpboot_create_thread+0xf0/0xf0 [0.511863] [] kthread+0xe3/0xf0 [0.511867] [] ? wait_for_common+0x143/0x180 [0.511873] [] kernel_thread_helper+0x4/0x10 [0.511878] [] ? retint_restore_args+0x13/0x13 [0.511883] [] ? insert_kthread_work+0x90/0x90 [0.511888] [] ? gs_change+0x13/0x13 [0.511916] Code: 24 04 02 00 00 00 0f 1f 80 00 00 00 00 e8 b3 46 ff ff e9 b6 fe ff ff 66 0f 1f 44 00 00 45 8b 34 24 e8 ff 72 8a 00 41 39 c6 74 0a <0f> 0b 0f 1f 84 00 00 00 00 00 41 8b 44 24 04 85 c0 74 0f 83 f8 [0.511919] RIP [] smpboot_thread_fn+0x196/0x2e0 [0.511920] RSP [0.511922] ---[ end trace 127920ef70923ae1 ]--- I'm starting the guest with numa=fake=10, so vcpu 0 ends up on the same (fake) node as vcpu 10, and while digging into the bug, it seems that the issue is that vcpu10's thread gets scheduled on vcpu0. Beyond that I don't really understand what's wrong... Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
Hi Thomas, On 07/16/2012 12:42 PM, Thomas Gleixner wrote: Provide a generic interface for setting up and tearing down percpu threads. On registration the threads for already online cpus are created and started. On deregistration (modules) the threads are stoppped. During hotplug operations the threads are created, started, parked and unparked. The datastructure for registration provides a pointer to percpu storage space and optional setup, cleanup, park, unpark functions. These functions are called when the thread state changes. Each implementation has to provide a function which is queried and returns whether the thread should run and the thread function itself. The core code handles all state transitions and avoids duplicated code in the call sites. Signed-off-by: Thomas Gleixner t...@linutronix.de --- This patch seems to cause the following BUG() on KVM guests with large amount of VCPUs: [0.511760] [ cut here ] [0.511761] kernel BUG at kernel/smpboot.c:134! [0.511764] invalid opcode: [#3] PREEMPT SMP DEBUG_PAGEALLOC [0.511779] CPU 0 [0.511780] Pid: 70, comm: watchdog/10 Tainted: G D W 3.6.0-rc6-next-20120919-sasha-1-gb54aafe #365 [0.511783] RIP: 0010:[81141676] [81141676] smpboot_thread_fn+0x196/0x2e0 [0.511785] RSP: 0018:88000cf4bdd0 EFLAGS: 00010206 [0.511786] RAX: RBX: 88000cf58000 RCX: [0.511787] RDX: RSI: 0001 RDI: 0001 [0.511788] RBP: 88000cf4be30 R08: R09: 0001 [0.511789] R10: R11: R12: 88000cdb9ff0 [0.511790] R13: 84c60920 R14: 000a R15: 88000cf58000 [0.511792] FS: () GS:88000d20() knlGS: [0.511794] CS: 0010 DS: ES: CR0: 8005003b [0.511795] CR2: CR3: 04c26000 CR4: 000406f0 [0.511801] DR0: DR1: DR2: [0.511805] DR3: DR6: 0ff0 DR7: 0400 [0.511807] Process watchdog/10 (pid: 70, threadinfo 88000cf4a000, task 88000cf58000) [0.511808] Stack: [0.511822] 88000cf4bfd8 88000cf4bfd8 [0.511833] 88000cf4be00 839eace5 88000cf4be30 88000cdd1c68 [0.511844] 88000cdb9ff0 811414e0 [0.511845] Call Trace: [0.511852] [839eace5] ? schedule+0x55/0x60 [0.511857] [811414e0] ? __smpboot_create_thread+0xf0/0xf0 [0.511863] [81135c13] kthread+0xe3/0xf0 [0.511867] [839eb463] ? wait_for_common+0x143/0x180 [0.511873] [839ef044] kernel_thread_helper+0x4/0x10 [0.511878] [839ed3b4] ? retint_restore_args+0x13/0x13 [0.511883] [81135b30] ? insert_kthread_work+0x90/0x90 [0.511888] [839ef040] ? gs_change+0x13/0x13 [0.511916] Code: 24 04 02 00 00 00 0f 1f 80 00 00 00 00 e8 b3 46 ff ff e9 b6 fe ff ff 66 0f 1f 44 00 00 45 8b 34 24 e8 ff 72 8a 00 41 39 c6 74 0a 0f 0b 0f 1f 84 00 00 00 00 00 41 8b 44 24 04 85 c0 74 0f 83 f8 [0.511919] RIP [81141676] smpboot_thread_fn+0x196/0x2e0 [0.511920] RSP 88000cf4bdd0 [0.511922] ---[ end trace 127920ef70923ae1 ]--- I'm starting the guest with numa=fake=10, so vcpu 0 ends up on the same (fake) node as vcpu 10, and while digging into the bug, it seems that the issue is that vcpu10's thread gets scheduled on vcpu0. Beyond that I don't really understand what's wrong... Thanks, Sasha -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On 07/21/2012 02:56 PM, Srivatsa S. Bhat wrote:
> On 07/16/2012 04:12 PM, Thomas Gleixner wrote:
>> Provide a generic interface for setting up and tearing down percpu
>> threads.
>>
>> On registration the threads for already online cpus are created and
>> started. On deregistration (modules) the threads are stoppped.
>>
>> During hotplug operations the threads are created, started, parked and
>> unparked. The datastructure for registration provides a pointer to
>> percpu storage space and optional setup, cleanup, park, unpark
>> functions. These functions are called when the thread state changes.
>>
>> Each implementation has to provide a function which is queried and
>> returns whether the thread should run and the thread function itself.
>>
>> The core code handles all state transitions and avoids duplicated code
>> in the call sites.
>>
>> Signed-off-by: Thomas Gleixner
>
> Elegant design and very beautiful code!
> It was such a pleasure to read and review it :-)
>
> Reviewed-by: Srivatsa S. Bhat
>
Of course, the preempt imbalance needs to get resolved, as pointed out
by Paul... Your patchset + his fix worked fine, without throwing any
scheduling-while-atomic splats.
Regards,
Srivatsa S. Bhat
> [ A minor nit below ]
>
>> ---
>> include/linux/smpboot.h | 43 +
>> kernel/cpu.c| 10 +-
>> kernel/smpboot.c| 229
>>
>> kernel/smpboot.h|4
>> 4 files changed, 285 insertions(+), 1 deletion(-)
>>
>> +
>> +/**
>> + * smpboot_thread_fn - percpu hotplug thread loop function
>> + * @void: thread data pointer
>
> s/void/data
>
>> + *
>> + * Checks for thread stop and park conditions. Calls the necessary
>> + * setup, cleanup, park and unpark functions for the registered
>> + * thread.
>> + *
>> + * Returns 1 when the thread should exit, 0 otherwise.
>> + */
>> +static int smpboot_thread_fn(void *data)
>> +{
>> +struct smpboot_thread_data *td = data;
>> +struct smp_hotplug_thread *ht = td->ht;
>> +
>
> Regards,
> Srivatsa S. Bhat
>
--
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On Mon, Jul 16, 2012 at 10:42:36AM -, Thomas Gleixner wrote:
> Provide a generic interface for setting up and tearing down percpu
> threads.
>
> On registration the threads for already online cpus are created and
> started. On deregistration (modules) the threads are stoppped.
>
> During hotplug operations the threads are created, started, parked and
> unparked. The datastructure for registration provides a pointer to
> percpu storage space and optional setup, cleanup, park, unpark
> functions. These functions are called when the thread state changes.
>
> Each implementation has to provide a function which is queried and
> returns whether the thread should run and the thread function itself.
>
> The core code handles all state transitions and avoids duplicated code
> in the call sites.
>
> Signed-off-by: Thomas Gleixner
This approach certainly results in much nicer code!
One issue noted below.
Thanx, Paul
> +static int smpboot_thread_fn(void *data)
> +{
> + struct smpboot_thread_data *td = data;
> + struct smp_hotplug_thread *ht = td->ht;
> +
> + while (1) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + preempt_disable();
> + if (kthread_should_stop()) {
> + set_current_state(TASK_RUNNING);
> + preempt_enable();
> + if (ht->cleanup)
> + ht->cleanup(td->cpu, cpu_online(td->cpu));
> + kfree(td);
> + return 0;
> + }
> +
> + if (kthread_should_park()) {
> + __set_current_state(TASK_RUNNING);
> + preempt_enable();
> + if (ht->park && td->status == HP_THREAD_ACTIVE) {
> + BUG_ON(td->cpu != smp_processor_id());
> + ht->park(td->cpu);
> + td->status = HP_THREAD_PARKED;
> + }
> + kthread_parkme();
> + /* We might have been woken for stop */
> + continue;
> + }
> +
> + BUG_ON(td->cpu != smp_processor_id());
> +
> + /* Check for state change setup */
> + switch (td->status) {
> + case HP_THREAD_NONE:
> + preempt_enable();
> + if (ht->setup)
> + ht->setup(td->cpu);
> + td->status = HP_THREAD_ACTIVE;
> + preempt_disable();
> + break;
> + case HP_THREAD_PARKED:
> + preempt_enable();
> + if (ht->unpark)
> + ht->unpark(td->cpu);
> + td->status = HP_THREAD_ACTIVE;
> + preempt_disable();
> + break;
> + }
> +
> + if (!ht->thread_should_run(td->cpu)) {
> + schedule_preempt_disabled();
> + } else {
> + set_current_state(TASK_RUNNING);
> + preempt_enable();
> + ht->thread_fn(td->cpu);
> + preempt_disable();
> + }
At this point, preemption is disabled, but the code at the beginning
of the loop will disable it again. This results in "scheduling while
atomic" splats. I did the following to clear them up:
+ if (!ht->thread_should_run(td->cpu)) {
+ preempt_enable();
+ schedule();
+ } else {
+ set_current_state(TASK_RUNNING);
+ preempt_enable();
+ ht->thread_fn(td->cpu);
+ }
I also placed the updated series on -rcu at branch rcu/smp/hotplug
(git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git),
based on tip/smp/hotplug, for Linaro testing purposes.
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On 07/16/2012 04:12 PM, Thomas Gleixner wrote:
> Provide a generic interface for setting up and tearing down percpu
> threads.
>
> On registration the threads for already online cpus are created and
> started. On deregistration (modules) the threads are stoppped.
>
> During hotplug operations the threads are created, started, parked and
> unparked. The datastructure for registration provides a pointer to
> percpu storage space and optional setup, cleanup, park, unpark
> functions. These functions are called when the thread state changes.
>
> Each implementation has to provide a function which is queried and
> returns whether the thread should run and the thread function itself.
>
> The core code handles all state transitions and avoids duplicated code
> in the call sites.
>
> Signed-off-by: Thomas Gleixner
Elegant design and very beautiful code!
It was such a pleasure to read and review it :-)
Reviewed-by: Srivatsa S. Bhat
[ A minor nit below ]
> ---
> include/linux/smpboot.h | 43 +
> kernel/cpu.c| 10 +-
> kernel/smpboot.c| 229
>
> kernel/smpboot.h|4
> 4 files changed, 285 insertions(+), 1 deletion(-)
>
> +
> +/**
> + * smpboot_thread_fn - percpu hotplug thread loop function
> + * @void:thread data pointer
s/void/data
> + *
> + * Checks for thread stop and park conditions. Calls the necessary
> + * setup, cleanup, park and unpark functions for the registered
> + * thread.
> + *
> + * Returns 1 when the thread should exit, 0 otherwise.
> + */
> +static int smpboot_thread_fn(void *data)
> +{
> + struct smpboot_thread_data *td = data;
> + struct smp_hotplug_thread *ht = td->ht;
> +
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On 07/16/2012 04:12 PM, Thomas Gleixner wrote:
Provide a generic interface for setting up and tearing down percpu
threads.
On registration the threads for already online cpus are created and
started. On deregistration (modules) the threads are stoppped.
During hotplug operations the threads are created, started, parked and
unparked. The datastructure for registration provides a pointer to
percpu storage space and optional setup, cleanup, park, unpark
functions. These functions are called when the thread state changes.
Each implementation has to provide a function which is queried and
returns whether the thread should run and the thread function itself.
The core code handles all state transitions and avoids duplicated code
in the call sites.
Signed-off-by: Thomas Gleixner t...@linutronix.de
Elegant design and very beautiful code!
It was such a pleasure to read and review it :-)
Reviewed-by: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
[ A minor nit below ]
---
include/linux/smpboot.h | 43 +
kernel/cpu.c| 10 +-
kernel/smpboot.c| 229
kernel/smpboot.h|4
4 files changed, 285 insertions(+), 1 deletion(-)
+
+/**
+ * smpboot_thread_fn - percpu hotplug thread loop function
+ * @void:thread data pointer
s/void/data
+ *
+ * Checks for thread stop and park conditions. Calls the necessary
+ * setup, cleanup, park and unpark functions for the registered
+ * thread.
+ *
+ * Returns 1 when the thread should exit, 0 otherwise.
+ */
+static int smpboot_thread_fn(void *data)
+{
+ struct smpboot_thread_data *td = data;
+ struct smp_hotplug_thread *ht = td-ht;
+
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On Mon, Jul 16, 2012 at 10:42:36AM -, Thomas Gleixner wrote:
Provide a generic interface for setting up and tearing down percpu
threads.
On registration the threads for already online cpus are created and
started. On deregistration (modules) the threads are stoppped.
During hotplug operations the threads are created, started, parked and
unparked. The datastructure for registration provides a pointer to
percpu storage space and optional setup, cleanup, park, unpark
functions. These functions are called when the thread state changes.
Each implementation has to provide a function which is queried and
returns whether the thread should run and the thread function itself.
The core code handles all state transitions and avoids duplicated code
in the call sites.
Signed-off-by: Thomas Gleixner t...@linutronix.de
This approach certainly results in much nicer code!
One issue noted below.
Thanx, Paul
+static int smpboot_thread_fn(void *data)
+{
+ struct smpboot_thread_data *td = data;
+ struct smp_hotplug_thread *ht = td-ht;
+
+ while (1) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ preempt_disable();
+ if (kthread_should_stop()) {
+ set_current_state(TASK_RUNNING);
+ preempt_enable();
+ if (ht-cleanup)
+ ht-cleanup(td-cpu, cpu_online(td-cpu));
+ kfree(td);
+ return 0;
+ }
+
+ if (kthread_should_park()) {
+ __set_current_state(TASK_RUNNING);
+ preempt_enable();
+ if (ht-park td-status == HP_THREAD_ACTIVE) {
+ BUG_ON(td-cpu != smp_processor_id());
+ ht-park(td-cpu);
+ td-status = HP_THREAD_PARKED;
+ }
+ kthread_parkme();
+ /* We might have been woken for stop */
+ continue;
+ }
+
+ BUG_ON(td-cpu != smp_processor_id());
+
+ /* Check for state change setup */
+ switch (td-status) {
+ case HP_THREAD_NONE:
+ preempt_enable();
+ if (ht-setup)
+ ht-setup(td-cpu);
+ td-status = HP_THREAD_ACTIVE;
+ preempt_disable();
+ break;
+ case HP_THREAD_PARKED:
+ preempt_enable();
+ if (ht-unpark)
+ ht-unpark(td-cpu);
+ td-status = HP_THREAD_ACTIVE;
+ preempt_disable();
+ break;
+ }
+
+ if (!ht-thread_should_run(td-cpu)) {
+ schedule_preempt_disabled();
+ } else {
+ set_current_state(TASK_RUNNING);
+ preempt_enable();
+ ht-thread_fn(td-cpu);
+ preempt_disable();
+ }
At this point, preemption is disabled, but the code at the beginning
of the loop will disable it again. This results in scheduling while
atomic splats. I did the following to clear them up:
+ if (!ht-thread_should_run(td-cpu)) {
+ preempt_enable();
+ schedule();
+ } else {
+ set_current_state(TASK_RUNNING);
+ preempt_enable();
+ ht-thread_fn(td-cpu);
+ }
I also placed the updated series on -rcu at branch rcu/smp/hotplug
(git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git),
based on tip/smp/hotplug, for Linaro testing purposes.
Thanx, Paul
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
On 07/21/2012 02:56 PM, Srivatsa S. Bhat wrote:
On 07/16/2012 04:12 PM, Thomas Gleixner wrote:
Provide a generic interface for setting up and tearing down percpu
threads.
On registration the threads for already online cpus are created and
started. On deregistration (modules) the threads are stoppped.
During hotplug operations the threads are created, started, parked and
unparked. The datastructure for registration provides a pointer to
percpu storage space and optional setup, cleanup, park, unpark
functions. These functions are called when the thread state changes.
Each implementation has to provide a function which is queried and
returns whether the thread should run and the thread function itself.
The core code handles all state transitions and avoids duplicated code
in the call sites.
Signed-off-by: Thomas Gleixner t...@linutronix.de
Elegant design and very beautiful code!
It was such a pleasure to read and review it :-)
Reviewed-by: Srivatsa S. Bhat srivatsa.b...@linux.vnet.ibm.com
Of course, the preempt imbalance needs to get resolved, as pointed out
by Paul... Your patchset + his fix worked fine, without throwing any
scheduling-while-atomic splats.
Regards,
Srivatsa S. Bhat
[ A minor nit below ]
---
include/linux/smpboot.h | 43 +
kernel/cpu.c| 10 +-
kernel/smpboot.c| 229
kernel/smpboot.h|4
4 files changed, 285 insertions(+), 1 deletion(-)
+
+/**
+ * smpboot_thread_fn - percpu hotplug thread loop function
+ * @void: thread data pointer
s/void/data
+ *
+ * Checks for thread stop and park conditions. Calls the necessary
+ * setup, cleanup, park and unpark functions for the registered
+ * thread.
+ *
+ * Returns 1 when the thread should exit, 0 otherwise.
+ */
+static int smpboot_thread_fn(void *data)
+{
+struct smpboot_thread_data *td = data;
+struct smp_hotplug_thread *ht = td-ht;
+
Regards,
Srivatsa S. Bhat
--
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
Provide a generic interface for setting up and tearing down percpu
threads.
On registration the threads for already online cpus are created and
started. On deregistration (modules) the threads are stoppped.
During hotplug operations the threads are created, started, parked and
unparked. The datastructure for registration provides a pointer to
percpu storage space and optional setup, cleanup, park, unpark
functions. These functions are called when the thread state changes.
Each implementation has to provide a function which is queried and
returns whether the thread should run and the thread function itself.
The core code handles all state transitions and avoids duplicated code
in the call sites.
Signed-off-by: Thomas Gleixner
---
include/linux/smpboot.h | 43 +
kernel/cpu.c| 10 +-
kernel/smpboot.c| 229
kernel/smpboot.h|4
4 files changed, 285 insertions(+), 1 deletion(-)
Index: tip/include/linux/smpboot.h
===
--- /dev/null
+++ tip/include/linux/smpboot.h
@@ -0,0 +1,43 @@
+#ifndef _LINUX_SMPBOOT_H
+#define _LINUX_SMPBOOT_H
+
+#include
+
+struct task_struct;
+/* Cookie handed to the thread_fn*/
+struct smpboot_thread_data;
+
+/**
+ * struct smp_hotplug_thread - CPU hotplug related thread descriptor
+ * @store: Pointer to per cpu storage for the task pointers
+ * @list: List head for core management
+ * @thread_should_run: Check whether the thread should run or not. Called with
+ * preemption disabled.
+ * @thread_fn: The associated thread function
+ * @setup: Optional setup function, called when the thread gets
+ * operational the first time
+ * @cleanup: Optional cleanup function, called when the thread
+ * should stop (module exit)
+ * @park: Optional park function, called when the thread is
+ * parked (cpu offline)
+ * @unpark:Optional unpark function, called when the thread is
+ * unparked (cpu online)
+ * @thread_comm: The base name of the thread
+ */
+struct smp_hotplug_thread {
+ struct task_struct __percpu **store;
+ struct list_headlist;
+ int (*thread_should_run)(unsigned int cpu);
+ void(*thread_fn)(unsigned int cpu);
+ void(*setup)(unsigned int cpu);
+ void(*cleanup)(unsigned int cpu, bool
online);
+ void(*park)(unsigned int cpu);
+ void(*unpark)(unsigned int cpu);
+ const char *thread_comm;
+};
+
+int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread);
+void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
+int smpboot_thread_schedule(void);
+
+#endif
Index: tip/kernel/cpu.c
===
--- tip.orig/kernel/cpu.c
+++ tip/kernel/cpu.c
@@ -280,12 +280,13 @@ static int __ref _cpu_down(unsigned int
__func__, cpu);
goto out_release;
}
+ smpboot_park_threads(cpu);
err = __stop_machine(take_cpu_down, _param, cpumask_of(cpu));
if (err) {
/* CPU didn't die: tell everyone. Can't complain. */
+ smpboot_unpark_threads(cpu);
cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
-
goto out_release;
}
BUG_ON(cpu_online(cpu));
@@ -354,6 +355,10 @@ static int __cpuinit _cpu_up(unsigned in
goto out;
}
+ ret = smpboot_create_threads(cpu);
+ if (ret)
+ goto out;
+
ret = __cpu_notify(CPU_UP_PREPARE | mod, hcpu, -1, _calls);
if (ret) {
nr_calls--;
@@ -368,6 +373,9 @@ static int __cpuinit _cpu_up(unsigned in
goto out_notify;
BUG_ON(!cpu_online(cpu));
+ /* Wake the per cpu threads */
+ smpboot_unpark_threads(cpu);
+
/* Now call notifier in preparation. */
cpu_notify(CPU_ONLINE | mod, hcpu);
Index: tip/kernel/smpboot.c
===
--- tip.orig/kernel/smpboot.c
+++ tip/kernel/smpboot.c
@@ -1,11 +1,17 @@
/*
* Common SMP CPU bringup/teardown functions
*/
+#include
#include
#include
#include
+#include
+#include
#include
+#include
#include
+#include
+#include
#include "smpboot.h"
@@ -65,3 +71,226 @@ void __init idle_threads_init(void)
}
}
#endif
+
+static LIST_HEAD(hotplug_threads);
+static DEFINE_MUTEX(smpboot_threads_lock);
+
+struct smpboot_thread_data {
+ unsigned intcpu;
+ unsigned int
[Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
Provide a generic interface for setting up and tearing down percpu
threads.
On registration the threads for already online cpus are created and
started. On deregistration (modules) the threads are stoppped.
During hotplug operations the threads are created, started, parked and
unparked. The datastructure for registration provides a pointer to
percpu storage space and optional setup, cleanup, park, unpark
functions. These functions are called when the thread state changes.
Each implementation has to provide a function which is queried and
returns whether the thread should run and the thread function itself.
The core code handles all state transitions and avoids duplicated code
in the call sites.
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
include/linux/smpboot.h | 43 +
kernel/cpu.c| 10 +-
kernel/smpboot.c| 229
kernel/smpboot.h|4
4 files changed, 285 insertions(+), 1 deletion(-)
Index: tip/include/linux/smpboot.h
===
--- /dev/null
+++ tip/include/linux/smpboot.h
@@ -0,0 +1,43 @@
+#ifndef _LINUX_SMPBOOT_H
+#define _LINUX_SMPBOOT_H
+
+#include linux/types.h
+
+struct task_struct;
+/* Cookie handed to the thread_fn*/
+struct smpboot_thread_data;
+
+/**
+ * struct smp_hotplug_thread - CPU hotplug related thread descriptor
+ * @store: Pointer to per cpu storage for the task pointers
+ * @list: List head for core management
+ * @thread_should_run: Check whether the thread should run or not. Called with
+ * preemption disabled.
+ * @thread_fn: The associated thread function
+ * @setup: Optional setup function, called when the thread gets
+ * operational the first time
+ * @cleanup: Optional cleanup function, called when the thread
+ * should stop (module exit)
+ * @park: Optional park function, called when the thread is
+ * parked (cpu offline)
+ * @unpark:Optional unpark function, called when the thread is
+ * unparked (cpu online)
+ * @thread_comm: The base name of the thread
+ */
+struct smp_hotplug_thread {
+ struct task_struct __percpu **store;
+ struct list_headlist;
+ int (*thread_should_run)(unsigned int cpu);
+ void(*thread_fn)(unsigned int cpu);
+ void(*setup)(unsigned int cpu);
+ void(*cleanup)(unsigned int cpu, bool
online);
+ void(*park)(unsigned int cpu);
+ void(*unpark)(unsigned int cpu);
+ const char *thread_comm;
+};
+
+int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread);
+void smpboot_unregister_percpu_thread(struct smp_hotplug_thread *plug_thread);
+int smpboot_thread_schedule(void);
+
+#endif
Index: tip/kernel/cpu.c
===
--- tip.orig/kernel/cpu.c
+++ tip/kernel/cpu.c
@@ -280,12 +280,13 @@ static int __ref _cpu_down(unsigned int
__func__, cpu);
goto out_release;
}
+ smpboot_park_threads(cpu);
err = __stop_machine(take_cpu_down, tcd_param, cpumask_of(cpu));
if (err) {
/* CPU didn't die: tell everyone. Can't complain. */
+ smpboot_unpark_threads(cpu);
cpu_notify_nofail(CPU_DOWN_FAILED | mod, hcpu);
-
goto out_release;
}
BUG_ON(cpu_online(cpu));
@@ -354,6 +355,10 @@ static int __cpuinit _cpu_up(unsigned in
goto out;
}
+ ret = smpboot_create_threads(cpu);
+ if (ret)
+ goto out;
+
ret = __cpu_notify(CPU_UP_PREPARE | mod, hcpu, -1, nr_calls);
if (ret) {
nr_calls--;
@@ -368,6 +373,9 @@ static int __cpuinit _cpu_up(unsigned in
goto out_notify;
BUG_ON(!cpu_online(cpu));
+ /* Wake the per cpu threads */
+ smpboot_unpark_threads(cpu);
+
/* Now call notifier in preparation. */
cpu_notify(CPU_ONLINE | mod, hcpu);
Index: tip/kernel/smpboot.c
===
--- tip.orig/kernel/smpboot.c
+++ tip/kernel/smpboot.c
@@ -1,11 +1,17 @@
/*
* Common SMP CPU bringup/teardown functions
*/
+#include linux/cpu.h
#include linux/err.h
#include linux/smp.h
#include linux/init.h
+#include linux/list.h
+#include linux/slab.h
#include linux/sched.h
+#include linux/export.h
#include linux/percpu.h
+#include linux/kthread.h
+#include linux/smpboot.h
#include smpboot.h
@@ -65,3 +71,226 @@ void __init idle_threads_init(void)
}
}
#endif
+
+static
