Re: [PATCH v2] sched/fair: fix sgc->{min,max}_capacity miscalculate

[Peng Liu]

On Mon, Jan 06, 2020 at 10:25:49AM +0100, Dietmar Eggemann wrote:
> On 04/01/2020 14:08, Peng Liu wrote:
> 
> Could you add a hint that this is about the SD_OVERLAP path? Something
> like 'Fix sgc->{min,max}_capacity calculation for SD_OVERLAP'
> 
> > commit bf475ce0a3dd ("sched/fair: Add per-CPU min capacity to
> > sched_group_capacity") introduced per-cpu min_capacity.
> > 
> > commit e3d6d0cb66f2 ("sched/fair: Add sched_group per-CPU max capacity")
> > introduced per-cpu max_capacity.
> > 
> 
> Could we improve the description of the issue and the change a little
> bit? Something like:
> 
> In the SD_OVERLAP case, the local variable 'capacity' represents the sum
> of CPU capacity of all CPUs in the first sched group (sg) of the sched
> domain (sd).
> 
> It is erroneously used to calculate sg's min and max CPU capacity.
> To fix this use capacity_of(cpu) instead of 'capacity'.
> 
> The code which achieves this via cpu_rq(cpu)->sd->groups->sgc->capacity
> (for rq->sd != NULL) can be removed since it delivers the same value as
> capacity_of(cpu) which is currently only used for the (!rq->sd) case
> (see update_cpu_capacity()).
> A sg of the lowest sd (rq->sd or sd->child == NULL) represents a single
> CPU (and hence sg->sgc->capacity == capacity_of(cpu)).

Dietmar, thanks for your time. Indeed, it's better with a detailed description.

> 
> 
> Nit: Why not
> 
> +                       capacity += cpu_cap;
> +                       min_capacity = min(cpu_cap, min_capacity);
> +                       max_capacity = max(cpu_cap, max_capacity);
> 
> like in the !SD_OVERLAP path?
> 
> >  		}
> >  	} else  {
> >  		/*
> >