Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 5 May 2021 15:57:08 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Marcelo Tosatti <mtosatti@redhat.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Yunfeng Ye <yeyunfeng@huawei.com>
Subject: Re: [PATCH 8/8] tick/nohz: Kick only _queued_ task whose tick
 dependency is updated
Message-ID: <YJKkNJTQFtmQPv1u@hirez.programming.kicks-ass.net>
References: <20210422120158.33629-1-frederic@kernel.org>
 <20210422120158.33629-9-frederic@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210422120158.33629-9-frederic@kernel.org>
Precedence: bulk

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
>