Re: [PATCH] sched, fair: Allow a small degree of load imbalance between SD_NUMA domains v2

[Mel Gorman <mgorman@techsingularity.net>]

On Fri, Dec 20, 2019 at 12:40:02PM +0000, Valentin Schneider wrote:
> > +		/* Consider allowing a small imbalance between NUMA groups */
> > +		if (env->sd->flags & SD_NUMA) {
> > +			unsigned int imbalance_adj, imbalance_max;
> > +
> > +			/*
> > +			 * imbalance_adj is the allowable degree of imbalance
> > +			 * to exist between two NUMA domains. It's calculated
> > +			 * relative to imbalance_pct with a minimum of two
> > +			 * tasks or idle CPUs. The choice of two is due to
> > +			 * the most basic case of two communicating tasks
> > +			 * that should remain on the same NUMA node after
> > +			 * wakeup.
> > +			 */
> > +			imbalance_adj = max(2U, (busiest->group_weight *
> > +				(env->sd->imbalance_pct - 100) / 100) >> 1);
> > +
> > +			/*
> > +			 * Ignore small imbalances unless the busiest sd has
> > +			 * almost half as many busy CPUs as there are
> > +			 * available CPUs in the busiest group. Note that
> 
> This is all on the busiest group, so this should be more like:
> 
> 			 * Ignore small imbalances unless almost half of the
> 			 * busiest sg's CPUs are busy.
> 

Updated.

> > +			 * it is not exactly half as imbalance_adj must be
> > +			 * accounted for or the two domains do not converge
> > +			 * as equally balanced if the number of busy tasks is
> > +			 * roughly the size of one NUMA domain.
> > +			 */
> > +			imbalance_max = (busiest->group_weight >> 1) + imbalance_adj;
> > +			if (env->imbalance <= imbalance_adj &&
> 
> I'm confused now, have we set env->imbalance to anything at this point?

No, it's a no-op in this context and I meant to take the check out.

> AIUI
> Vincent's suggestion was to hinge this purely on the weight vs idle_cpus /
> nr_running, IOW not use imbalance:
> 
> if (sd->flags & SD_NUMA) {                                                                         
> 	imbalance_adj = max(2U, (busiest->group_weight *                                           
> 				 (env->sd->imbalance_pct - 100) / 100) >> 1);                      
> 	imbalance_max = (busiest->group_weight >> 1) + imbalance_adj;                              
>                                                                                                      
> 	if (busiest->idle_cpus >= imbalance_max) {                                                 
> 		env->imbalance = 0;                                                                
> 		return;                                                                            
> 	}                                                                                          
> }                                                                                                  
>        

And that's what it is now. Functionally it's equivalent similar other
than imbalance could be potentially anything (but should be zero).

> Now, I have to say I'm not sold on the idle_cpus thing, I'd much rather use
> the number of runnable tasks. We are setting up a threshold for how far we
> are willing to ignore imbalances; if we have overloaded CPUs we *really*
> should try to solve this. Number of tasks is the safer option IMO: when we
> do have one task per CPU, it'll be the same as if we had used idle_cpus, and
> when we don't have one task per CPU we'll load-balance more often that we
> would have with idle_cpus.
> 

I couldn't convince myself to really push back hard on the sum_nr_runnable
versus idle_cpus.  If the local group has spare capacity and the busiest
group has multiple tasks stacked on CPUs then it's most likely due to
CPU affinity. In that case, there is no guarantee tasks can move to the
local group either. In that case, the difference between sum_nr_running
and idle_cpus is almost moot.  There may be common use cases where the
distinction really matters but right now, I'm at the point where I think
such a change could be a separate patch with the use case included and
supporting data on why it must be sum_nr_running.  Right now, I feel it's
mostly a cosmetic issue given the context and intent of the patch.

-- 
Mel Gorman
SUSE Labs