lkml.org 
[lkml]   [2012]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [Patch 3/7] smpboot: Provide infrastructure for percpu hotplug threads
    On Mon, Jul 16, 2012 at 10:42:36AM -0000, 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 <tglx@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



    \
     
     \ /
      Last update: 2012-07-21 20:01    [W:0.029 / U:31.612 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site