Re: [PATCH 8/8] tick/nohz: Kick only _queued_ task whose tick dependency is updated

[Frederic Weisbecker]

On Wed, May 05, 2021 at 03:57:08PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 22, 2021 at 02:01:58PM +0200, Frederic Weisbecker wrote:
> 
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 98191218d891..08526227d200 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -1580,6 +1580,11 @@ static inline void uclamp_post_fork(struct task_struct *p) { }
> >  static inline void init_uclamp(void) { }
> >  #endif /* CONFIG_UCLAMP_TASK */
> >  
> > +bool sched_task_on_rq(struct task_struct *p)
> > +{
> > +	return task_on_rq_queued(p);
> > +}
> > +
> >  static inline void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
> >  {
> >  	if (!(flags & ENQUEUE_NOCLOCK))
> 
> That's a wee bit sad..

I know... But I couldn't find a better way.

> 
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index ad5c3905196a..faba7881048f 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -324,8 +324,6 @@ void tick_nohz_full_kick_cpu(int cpu)
> >  
> >  static void tick_nohz_kick_task(struct task_struct *tsk)
> >  {
> > -	int cpu = task_cpu(tsk);
> > -
> >  	/*
> >  	 * If the task concurrently migrates to another cpu,
> >  	 * we guarantee it sees the new tick dependency upon
> > @@ -340,6 +338,23 @@ static void tick_nohz_kick_task(struct task_struct *tsk)
> >  	 *   tick_nohz_task_switch()            smp_mb() (atomic_fetch_or())
> >  	 *      LOAD p->tick_dep_mask           LOAD p->cpu
> >  	 */
> > +	int cpu = task_cpu(tsk);
> > +
> > +	/*
> > +	 * If the task is not running, run_posix_cpu_timers
> > +	 * has nothing to elapsed, can spare IPI in that
> > +	 * case.
> > +	 *
> > +	 * activate_task()                      STORE p->tick_dep_mask
> > +	 * STORE p->on_rq
> > +	 * __schedule() (switch to task 'p')    smp_mb() (atomic_fetch_or())
> > +	 * LOCK rq->lock                        LOAD p->on_rq
> > +	 * smp_mb__after_spin_lock()
> > +	 * tick_nohz_task_switch()
> > +	 *	LOAD p->tick_dep_mask
> > +	 */
> 
> That needs indenting, the style is distinctly different from the comment
> right above it.

Ok, I'll fix that.

> 
> > +	if (!sched_task_on_rq(tsk))
> > +		return;
> 
> I'm too tired, but do we really need the task_cpu() load to be before
> this?

Nope, it should be fine to put it after.

Thanks!

> 
> >  
> >  	preempt_disable();
> >  	if (cpu_online(cpu))
> > -- 
> > 2.25.1
> >