Re: [PATCH] nohz: nohz idle balancing per node

From: Peter Zijlstra <peterz@infradead.org>
To: Nicholas Piggin <npiggin@gmail.com>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Vaidyanathan Srinivasan <svaidy@linux.ibm.com>,
	Srikar Dronamraju <srikar@linux.vnet.ibm.com>,
	Frederic Weisbecker <fweisbec@gmail.com>
Subject: Re: [PATCH] nohz: nohz idle balancing per node
Date: Thu, 1 Jul 2021 12:18:18 +0200	[thread overview]
Message-ID: <YN2Wav1CSVq+6cS+@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20210701055323.2199175-1-npiggin@gmail.com>

On Thu, Jul 01, 2021 at 03:53:23PM +1000, Nicholas Piggin wrote:
> Currently a single nohz idle CPU is designated to perform balancing on
> behalf of all other nohz idle CPUs in the system. Implement a per node
> nohz balancer to minimize cross-node memory accesses and runqueue lock
> acquisitions.
> 
> On a 4 node system, this improves performance by 9.3% on a 'pgbench -N'
> with 32 clients/jobs (which is about where throughput maxes out due to
> IO and contention in postgres).

Hmm, Suresh tried something like this around 2010 and then we ran into
trouble that when once node went completely idle and another node was
fully busy, the completely idle node would not run ILB and the node
would forever stay idle.

I've not gone through the code to see if that could still happen -- lots
has changed since, but it is something to consider.

Some further nits below..

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index fb469b26b00a..832f8673bba1 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5722,13 +5722,27 @@ DEFINE_PER_CPU(cpumask_var_t, select_idle_mask);
>  
>  #ifdef CONFIG_NO_HZ_COMMON
>  
> -static struct {
> +struct nohz {
>  	cpumask_var_t idle_cpus_mask;
>  	atomic_t nr_cpus;
>  	int has_blocked;		/* Idle CPUS has blocked load */
>  	unsigned long next_balance;     /* in jiffy units */
>  	unsigned long next_blocked;	/* Next update of blocked load in jiffies */
> -} nohz ____cacheline_aligned;
> +} ____cacheline_aligned;
> +
> +static struct nohz **nohz_nodes __ro_after_init;
> +
> +static struct nohz *get_nohz(void)
> +{
> +#ifdef CONFIG_CPU_ISOLATION
> +	/*
> +	 * May not have a house keeping CPU per node, do global idle balancing.
> +	 */
> +	if (static_branch_unlikely(&housekeeping_overridden))
> +		return nohz_nodes[0];
> +#endif
> +	return nohz_nodes[numa_node_id()];
> +}

*urgh* I'm not a fan of that isolation/hk behaviour. Even when the HK
mask allows for a CPU per node, this code won't DTRT. Do we want a
KERN_INFO message that performance will suffer here?

>  #endif /* CONFIG_NO_HZ_COMMON */
>  
> @@ -10291,14 +10305,14 @@ static void nohz_balancer_kick(struct rq *rq)
>  	 * None are in tickless mode and hence no need for NOHZ idle load
>  	 * balancing.
>  	 */
> -	if (likely(!atomic_read(&nohz.nr_cpus)))
> +	if (likely(!atomic_read(&get_nohz()->nr_cpus)))
>  		return;
>  
> -	if (READ_ONCE(nohz.has_blocked) &&
> -	    time_after(now, READ_ONCE(nohz.next_blocked)))
> +	if (READ_ONCE(get_nohz()->has_blocked) &&
> +	    time_after(now, READ_ONCE(get_nohz()->next_blocked)))
>  		flags = NOHZ_STATS_KICK;
>  
> -	if (time_before(now, nohz.next_balance))
> +	if (time_before(now, get_nohz()->next_balance))
>  		goto out;
>  
>  	if (rq->nr_running >= 2) {

Also, stuff like the above, will result in horrific code-gen, because
it cannot CSE get_nohz().