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
