Re: [PATCH RFC tip/core/rcu 11/11] rcu: move TREE_RCU from softirq to kthread

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Mon, Feb 28, 2011 at 11:29:44AM +0800, Lai Jiangshan wrote:
> On 02/26/2011 04:32 AM, Paul E. McKenney wrote:
> >>> +/*
> >>> + * Handle cases where the rcu_cpu_kthread() ends up on the wrong CPU.
> >>> + * This can happen while the corresponding CPU is either coming online
> >>> + * or going offline.  We cannot wait until the CPU is fully online
> >>> + * before starting the kthread, because the various notifier functions
> >>> + * can wait for RCU grace periods.  So we park rcu_cpu_kthread() until
> >>> + * the corresponding CPU is online.
> >>> + *
> >>> + * Return 1 if the kthread needs to stop, 0 otherwise.
> >>> + *
> >>> + * Caller must disable bh.  This function can momentarily enable it.
> >>> + */
> >>> +static int rcu_cpu_kthread_should_stop(int cpu)
> >>> +{
> >>> +	while (cpu_is_offline(cpu) || smp_processor_id() != cpu) {
> >>> +		if (kthread_should_stop())
> >>> +			return 1;
> >>> +		local_bh_enable();
> >>> +		schedule_timeout_uninterruptible(1);
> >>> +		if (smp_processor_id() != cpu)
> >>> +			set_cpus_allowed_ptr(current, cpumask_of(cpu));
> >>
> >> The current task is PF_THREAD_BOUND,
> >> Why do "set_cpus_allowed_ptr(current, cpumask_of(cpu));" ?
> > 
> > Because I have seen CPU hotplug operations unbind PF_THREAD_BOUND threads.
> > In addition, I end up having to spawn the kthread at CPU_UP_PREPARE time,
> > at which point the thread must run unbound because its CPU isn't online
> > yet.  I cannot invoke kthread_create() within the stop-machine handler
> > (right?).  I cannot wait until CPU_ONLINE time because that results in
> > hangs when other CPU notifiers wait for grace periods.
> > 
> > Yes, I did find out about the hangs the hard way.  Why do you ask?  ;-)
> 
> The current task is PF_THREAD_BOUND, "set_cpus_allowed_ptr(current, cpumask_of(cpu))"
> will do nothing even it runs on the wrong CPU.

You lost me on this one.

Looking at set_cpus_allowed_ptr()...

The "again" loop won't happen because the task is already running.
The CPU is online, so the cpumask_intersects() check won't kick
us out.  We are working with the current task, so the check for
PF_THREAD_BOUND, current, and cpumask_equal() won't kick us out.
If the old and new cpumasks had been the same, we would not have called
set_cpus_allowed_ptr() in the first place.  So we should get to
the call to migrate_task().

What am I missing here?

> If the task runs on the wrong CPU. We have no API to force/migrate the task
> to the bound CPU when the cpu becomes online. But wake_up_process() has
> a side affect that it will move a slept task to the correct online CPU.
> "schedule_timeout_uninterruptible(1);" will call
> wake_up_process() when timeout, so it will do all thing you need.
> 
> But "set_cpus_allowed_ptr(current, cpumask_of(cpu));" will do nothing.
> 
> The code is a little nasty I think. The proper solution I like:
> set the rcu_cpu_notify a proper priority, and wake up the kthread
> in the notifier.

I will be using both belt and suspenders on this one -- too much can
go wrong given slight adjustments in scheduler, CPU hotplug, and so on.

But speaking of paranoia, I should add a check of smp_processor_id()
vs. the local variable "cpu", shouldn't I?

> Steven, any suggestion? I just known very little about scheduler.
> 
> > 
> > Please feel free to suggest improvements in the header comment above
> > for rcu_cpu_kthread_should_stop(), which is my apparently insufficient
> > attempt to explain this.
> > 
> >>> +		local_bh_disable();
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +/*
> >>> + * Per-CPU kernel thread that invokes RCU callbacks.  This replaces the
> >>> + * earlier RCU softirq.
> >>> + */
> >>> +static int rcu_cpu_kthread(void *arg)
> >>> +{
> >>> +	int cpu = (int)(long)arg;
> >>> +	unsigned long flags;
> >>> +	int spincnt = 0;
> >>> +	wait_queue_head_t *wqp = &per_cpu(rcu_cpu_wq, cpu);
> >>> +	char work;
> >>> +	char *workp = &per_cpu(rcu_cpu_has_work, cpu);
> >>> +
> >>> +	for (;;) {
> >>> +		wait_event_interruptible(*wqp,
> >>> +					 *workp != 0 || kthread_should_stop());
> >>> +		local_bh_disable();
> >>> +		if (rcu_cpu_kthread_should_stop(cpu)) {
> >>> +			local_bh_enable();
> >>> +			break;
> >>> +		}
> >>> +		local_irq_save(flags);
> >>> +		work = *workp;
> >>> +		*workp = 0;
> >>> +		local_irq_restore(flags);
> >>> +		if (work)
> >>> +			rcu_process_callbacks();
> >>> +		local_bh_enable();
> >>> +		if (*workp != 0)
> >>> +			spincnt++;
> >>> +		else
> >>> +			spincnt = 0;
> >>> +		if (spincnt > 10) {
> >>
> >> "10" is a magic number here.
> > 
> > It is indeed.  Suggestions for a cpp macro name to hide it behind?
> > 
> >>> +			rcu_yield(cpu);
> >>> +			spincnt = 0;
> >>> +		}
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +
> >>
> >>
> >>> +/*
> >>> + * Per-rcu_node kthread, which is in charge of waking up the per-CPU
> >>> + * kthreads when needed.
> >>> + */
> >>> +static int rcu_node_kthread(void *arg)
> >>> +{
> >>> +	int cpu;
> >>> +	unsigned long flags;
> >>> +	unsigned long mask;
> >>> +	struct rcu_node *rnp = (struct rcu_node *)arg;
> >>> +	struct sched_param sp;
> >>> +	struct task_struct *t;
> >>> +
> >>> +	for (;;) {
> >>> +		wait_event_interruptible(rnp->node_wq, rnp->wakemask != 0 ||
> >>> +						       kthread_should_stop());
> >>> +		if (kthread_should_stop())
> >>> +			break;
> >>> +		raw_spin_lock_irqsave(&rnp->lock, flags);
> >>> +		mask = rnp->wakemask;
> >>> +		rnp->wakemask = 0;
> >>> +		raw_spin_unlock_irqrestore(&rnp->lock, flags);
> >>> +		for (cpu = rnp->grplo; cpu <= rnp->grphi; cpu++, mask <<= 1) {
> >>> +			if ((mask & 0x1) == 0)
> >>> +				continue;
> >>> +			preempt_disable();
> >>> +			per_cpu(rcu_cpu_has_work, cpu) = 1;
> >>> +			t = per_cpu(rcu_cpu_kthread_task, cpu);
> >>> +			if (t == NULL) {
> >>> +				preempt_enable();
> >>> +				continue;
> >>> +			}
> >>
> >> Obviously preempt_disable() is not for protecting remote percpu data.
> >> Is it for disabling cpu hotplug? I am afraid the @t may leave
> >> and become invalid.
> > 
> > Indeed, acquiring the rnp->lock is safer, except that I don't trust
> > calling sched_setscheduler_nocheck() in that state.  So I need to check
> > for the CPU being online after the preempt_disable().  This means that
> > I ignore requests to do work after CPU_DYING time, but that is OK because
> > force_quiescent_state() will figure out that the CPU is in fact offline.
> > 
> > Make sense?
> 
> Yes.

Good, I will take that approach.

> Another:
> 
> #if CONFIG_HOTPLUG_CPU
> get_task_struct() when set bit in wakemask
> put_task_struct() when clear bit in wakemask
> #endif

Good point, but I will pass on the added #ifdef.  ;-)

							Thanx, Paul