Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 30 Mar 2021 09:45:36 +0000
From:   Quentin Perret <qperret@google.com>
To:     Xuewen Yan <xuewen.yan94@gmail.com>
Cc:     mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com, linux-kernel@vger.kernel.org,
        quentin.perret@arm.com, zhang.lyra@gmail.com, xuewyan@foxmail.com
Subject: Re: [PATCH] sched/fair: use signed long when compute energy delta in
 eas
Message-ID: <YGLzQAvVqlrKb8AB@google.com>
References: <20210330052154.26861-1-xuewen.yan94@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210330052154.26861-1-xuewen.yan94@gmail.com>
Precedence: bulk

Hi,

On Tuesday 30 Mar 2021 at 13:21:54 (+0800), Xuewen Yan wrote:
> From: Xuewen Yan <xuewen.yan@unisoc.com>
> 
> now the energy delta compute as follow:
> 
> base_energy_pd = compute_energy(p, -1, pd);
> 	--->Traverse all CPUs in pd
> 	--->em_pd_energy()
> ----------------------------------------------------- \
> search for the max_sapre_cap_cpu                       \
> ---------------------------------                       search time
> cur_delta = compute_energy(p, max_spare_cap_cpu, pd);  /
> 	--->Traverse all CPUs in pd                   /
> ---------------------------------------------------- /
> 	--->em_pd_energy()
> cur_delta -= base_energy_pd;
> 
> During the search_time, or when calculate the cpu_util in
> compute_energy(), there may occurred task dequeue or cpu_util change,
> it may cause the cur_energy < base_energy_pd, so the cur_delta
> would be negative. But the cur_delta is unsigned long, at this time,
> the cur_delta would always bigger than best_delta of last pd.
> 
> Change the vars to signed long.

Is that really helping though?

Yes you will not overflow, but the decision is still 'wrong' if the util
values are not stable for the entire wake-up. I think folks on the Arm
side had patches to try and cache the util values upfront, and then use
them throughout feec() and compute_energy(), which I think would be a
better fix.

Dietmar, wdyt?

Thanks,
Quentin