Re: [PATCH 0/6] sched: TTWU, IPI, and assorted stuff

[Paul E. McKenney]

On Fri, Jun 19, 2020 at 10:20:47AM -0700, Paul E. McKenney wrote:
> On Fri, Jun 19, 2020 at 03:44:23PM +0200, Peter Zijlstra wrote:
> > On Tue, Jun 16, 2020 at 07:17:21PM +0200, Peter Zijlstra wrote:

[ . . . ]

> > If not, I'm, once again, defeated by this...
> 
> Here is hoping that this patch cures things!
> 
> 							Thanx, Paul
> 
> > ---
> >  kernel/sched/core.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 8298b2c240ce..5534eb1ab79a 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -2378,6 +2378,9 @@ static inline bool ttwu_queue_cond(int cpu, int wake_flags)
> >  static bool ttwu_queue_wakelist(struct task_struct *p, int cpu, int wake_flags)
> >  {
> >  	if (sched_feat(TTWU_QUEUE) && ttwu_queue_cond(cpu, wake_flags)) {
> > +		if (WARN_ON(cpu == smp_processor_id()))
> > +			return false;
> > +
> >  		sched_clock_cpu(cpu); /* Sync clocks across CPUs */
> >  		__ttwu_queue_wakelist(p, cpu, wake_flags);
> >  		return true;
> > @@ -2550,7 +2553,6 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> >  
> >  	/* We're going to change ->state: */
> >  	success = 1;
> > -	cpu = task_cpu(p);
> >  
> >  	/*
> >  	 * Ensure we load p->on_rq _after_ p->state, otherwise it would
> > @@ -2615,7 +2617,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> >  	 * let the waker make forward progress. This is safe because IRQs are
> >  	 * disabled and the IPI will deliver after on_cpu is cleared.
> >  	 */
> > -	if (READ_ONCE(p->on_cpu) && ttwu_queue_wakelist(p, cpu, wake_flags | WF_ON_RQ))
> > +	if (smp_load_acquire(&p->on_cpu) &&

Given the x86 memory model, this only protects against the compiler
reordering accesses in ttwu_queue_wakelist() against the fetch of
p->on_cpu, correct?

Don't get me wrong, I do see some potential compiler misorderings,
including with cpu_rq(cpu)->nr_running.  Just curious.

						Thanx, Paul

> > +	    ttwu_queue_wakelist(p, task_cpu(p), wake_flags | WF_ON_RQ))
> >  		goto unlock;
> >  
> >  	/*
> > @@ -2635,6 +2638,8 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
> >  		psi_ttwu_dequeue(p);
> >  		set_task_cpu(p, cpu);
> >  	}
> > +#else
> > +	cpu = task_cpu(p);
> >  #endif /* CONFIG_SMP */
> >  
> >  	ttwu_queue(p, cpu, wake_flags);
> >