Messages in this thread | | | Date | Mon, 30 Jul 2018 10:14:14 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH RFC tip/core/rcu] Avoid resched_cpu() when rescheduling the current CPU |
| |
On Mon, Jul 30, 2018 at 06:42:47PM +0200, Peter Zijlstra wrote: > On Mon, Jul 30, 2018 at 07:59:33AM -0700, Paul E. McKenney wrote: > > > > Something that might be more in line with > > > resched_curr(smp_processor_id()) would be: > > > > > > preempt_disable(); > > > if (!test_tsk_need_resched(current)) { > > > set_tsk_need_resched(current); > > > set_preempt_need_resched(); > > > } > > > preempt_enable(); > > > > > > Where the preempt_enable() could of course instantly trigger the > > > reschedule if it was the outer most one. > > > > Ah. So should I use resched_curr() from rcu_check_callbacks(), which > > is invoked from the scheduling-clock interrupt? Right now I have calls > > to set_tsk_need_resched() and set_preempt_need_resched(). > > > > > > @@ -2674,10 +2675,12 @@ static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused > > > > > > > - resched_cpu(rdp->cpu); /* Provoke future context switch. */ > > > > > > > + set_tsk_need_resched(current); > > > > + set_preempt_need_resched(); > > > > > > That's not obviously correct. rdp->cpu had better be smp_processor_id(). > > > > At the beginning of the function, we have: > > > > struct rcu_data *rdp = raw_cpu_ptr(&rcu_data); > > > > And this is in a softirq handler, so we are OK. > > Agreed. > > > > > @@ -672,7 +672,8 @@ static void sync_rcu_exp_handler(void *unused) > > > > rcu_report_exp_rdp(rdp); > > > > } else { > > > > rdp->deferred_qs = true; > > > > - resched_cpu(rdp->cpu); > > > > + set_tsk_need_resched(t); > > > > + set_preempt_need_resched(); > > > > > > That only works if @t == current. > > > > At the beginning of the function, we have: > > > > struct task_struct *t = current; > > > > So we should be OK. > > Ah, the scheduler and locking code typically use to call that curr, to > be more explicit that it is the current task.
I cargo-culted the "t" from somewhere a very long time ago, and of course I have no idea from where. Now I have hundreds of them in RCU. :-/
Then again, if I am to change, doing it now when I have other full-source changes makes sense...
> > Should I be instead using resched_curr() on some or all of these? > > If, as it seems is the case, they are all targeting the current cpu and > have (soft) interrupts disabled, then what you propose is indeed fine.
Very good, I will leave them as is, then. Thank you for the review! May I add your Reviewed-by, Acked-by, or some such?
Thanx, Paul
| |