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

[Nicholas Piggin]

Excerpts from Peter Zijlstra's message of July 1, 2021 8:18 pm:
> 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 only changed which CPU runs ilb on behalf of others, not how the 
balancing is done.

If one node is idle, then one of the CPUs in that node will be the nohz
balancer for the others in that node. In any other respect, the 
balancing will be done the same as if the nohz balancer was on a 
different node. So I don't see how it could get into that situation 
unless I have a bug. Maybe Suresh's patch was doing something different.

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

IIRC it turned out to be non-trivial to work out the housekeeping vs 
online CPUs but it might be doable later.

I'm not sure about a printk, that would just fire every time. Maybe a 
generic printk or something in the docs to say housekeeping can affect
performance of non-isolated CPUs so make sure you measure.

> 
>>  #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().
> 

It was somewhat mechanical conversion. I could use a local variable for 
a couple of these cases.

Thanks,
Nick