Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 6 Jan 2020 22:49:41 +0800
From:   Peng Liu <iwtbavbm@gmail.com>
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, rostedt@goodmis.org,
        bsegall@google.com, mgorman@suse.de, qais.yousef@arm.com,
        morten.rasmussen@arm.com, valentin.schneider@arm.com
Subject: Re: [PATCH v2] sched/fair: fix sgc->{min,max}_capacity miscalculate
Message-ID: <20200106144941.GA15532@iZj6chx1xj0e0buvshuecpZ>
References: <20200104130828.GA7718@iZj6chx1xj0e0buvshuecpZ>
 <e034b806-bb3d-98c0-5d60-53610bfe6ca0@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e034b806-bb3d-98c0-5d60-53610bfe6ca0@arm.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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  {
> >  		/*
> >