Re: [PATCH 6/8] tick/nohz: Only wakeup a single target cpu when kicking a task

From: Peter Zijlstra
Date: Wed May 05 2021 - 09:43:46 EST


On Thu, Apr 22, 2021 at 02:01:56PM +0200, Frederic Weisbecker wrote:
> When adding a tick dependency to a task, its necessary to
> wakeup the CPU where the task resides to reevaluate tick
> dependencies on that CPU.
>
> However the current code wakes up all nohz_full CPUs, which
> is unnecessary.
>
> Switch to waking up a single CPU, by using ordering of writes
> to task->cpu and task->tick_dep_mask.
>
> Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Frederic Weisbecker <frederic@xxxxxxxxxx>
> Cc: Yunfeng Ye <yeyunfeng@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> ---
> kernel/time/tick-sched.c | 40 +++++++++++++++++++++++++++-------------
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index ffc13b9dfbe3..45d9a4ea3ee0 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -322,6 +322,31 @@ void tick_nohz_full_kick_cpu(int cpu)
> irq_work_queue_on(&per_cpu(nohz_full_kick_work, cpu), 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
> + * schedule.
> + *
> + *
> + * set_task_cpu(p, cpu);
> + * STORE p->cpu = @cpu
> + * __schedule() (switch to task 'p')
> + * LOCK rq->lock
> + * smp_mb__after_spin_lock() STORE p->tick_dep_mask
> + * tick_nohz_task_switch() smp_mb() (atomic_fetch_or())
> + * LOAD p->tick_dep_mask LOAD p->cpu
> + */
> +
> + preempt_disable();
> + if (cpu_online(cpu))
> + tick_nohz_full_kick_cpu(cpu);
> + preempt_enable();
> +}


That had me looking at tick_nohz_task_switch(), does we want the below?


diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9143163fa678..ff45fc513ba7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4207,6 +4207,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
vtime_task_switch(prev);
perf_event_task_sched_in(prev, current);
finish_task(prev);
+ tick_nohz_task_switch();
finish_lock_switch(rq);
finish_arch_post_lock_switch();
kcov_finish_switch(current);
@@ -4252,7 +4253,6 @@ static struct rq *finish_task_switch(struct task_struct *prev)
put_task_struct_rcu_user(prev);
}

- tick_nohz_task_switch();
return rq;
}

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 828b091501ca..ea079be9097f 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -447,13 +447,10 @@ void tick_nohz_dep_clear_signal(struct signal_struct *sig, enum tick_dep_bits bi
*/
void __tick_nohz_task_switch(void)
{
- unsigned long flags;
struct tick_sched *ts;

- local_irq_save(flags);
-
if (!tick_nohz_full_cpu(smp_processor_id()))
- goto out;
+ return;

ts = this_cpu_ptr(&tick_cpu_sched);

@@ -462,8 +459,6 @@ void __tick_nohz_task_switch(void)
atomic_read(&current->signal->tick_dep_mask))
tick_nohz_full_kick();
}
-out:
- local_irq_restore(flags);
}

/* Get the boot-time nohz CPU list from the kernel parameters. */