linux-kernel - Re: [PATCH RFC tip/core/rcu] Avoid resched_cpu() when rescheduling the current CPU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 30 Jul 2018 10:14:14 -0700
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org
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