Re: [RFC patch 5/5] infiniband: ehca: Use hotplug thread infrastructure

Thomas Gleixner
2012-06-18T08:08:34Z

On Mon, 18 Jun 2012, Rusty Russell wrote:

> On Wed, 13 Jun 2012 11:00:56 -0000, Thomas Gleixner <tglx@linutronix.de> wrote:
> > @@ -662,10 +663,15 @@ static inline int find_next_online_cpu(s
> > ehca_dmp(cpu_online_mask, cpumask_size(), "");
> >
> > spin_lock_irqsave(&pool->last_cpu_lock, flags);
> > - cpu = cpumask_next(pool->last_cpu, cpu_online_mask);
> > - if (cpu >= nr_cpu_ids)
> > - cpu = cpumask_first(cpu_online_mask);
> > - pool->last_cpu = cpu;
> > + while (1) {
> > + cpu = cpumask_next(pool->last_cpu, cpu_online_mask);
> > + if (cpu >= nr_cpu_ids)
> > + cpu = cpumask_first(cpu_online_mask);
> > + pool->last_cpu = cpu;
> > + /* Might be on the way out */
> > + if (per_cpu_ptr(pool->cpu_comp_tasks, cpu)->active)
> > + break;
> > + }
>
> Heh, isn't this what we used to call a "do while" loop? :)

Yep :)

> Your infrastructure is a really weird mix. On the one hand, it's a set
> of callbacks: setup, cleanup, park, unpark. Cool.
>
> On the other hand, instead of a 'run' callback, you've got a thread_fn,
> which has to loop and call smpboot_thread_check_parking().
>
> If you just had the thread_fn, it'd be trivial to follow program flow.
> If you just had the callbacks, it'd still be pretty easy, though it
> seems like a little too much help.

The reason why I moved stuff into the callbacks and have the state
machine inside of smpboot_check_kthread_parking() is that every user
has to do that. i.e. keep track of the state so you wont
setup/teardown stuff twice.

Now look at that function and copy it into all users of the park
infrastructure. Not pretty. I had it that way and it was fugly as
hell. That's why I came up with the callbacks and the generic state
machine.

> As it is, we have Paul doing setup stuff inside his thread_fn:
>
> + trace_rcu_utilization("Start CPU kthread@unpark");
> + sp.sched_priority = RCU_KTHREAD_PRIO;
> + sched_setscheduler_nocheck(current, SCHED_FIFO, &sp);

I fixed that already :)

Thanks,

tglx