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

[Peter Zijlstra]

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..

> 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.

> +	if (!sched_task_on_rq(tsk))
> +		return;

I'm too tired, but do we really need the task_cpu() load to be before
this?

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