From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D3AEDC433ED for ; Mon, 12 Apr 2021 10:26:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B0AA36134F for ; Mon, 12 Apr 2021 10:26:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238629AbhDLK1L (ORCPT ); Mon, 12 Apr 2021 06:27:11 -0400 Received: from foss.arm.com ([217.140.110.172]:45682 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237753AbhDLK1J (ORCPT ); Mon, 12 Apr 2021 06:27:09 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8A15B1FB; Mon, 12 Apr 2021 03:26:51 -0700 (PDT) Received: from [10.57.59.170] (unknown [10.57.59.170]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 433C83F694; Mon, 12 Apr 2021 03:26:48 -0700 (PDT) From: Pierre Gondois Subject: Re: [PATCH] sched/fair: use signed long when compute energy delta in eas To: Xuewen Yan Cc: Dietmar Eggemann , Quentin Perret , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Steven Rostedt , Benjamin Segall , Mel Gorman , Daniel Bristot de Oliveira , linux-kernel , Chunyan Zhang , Ryan Y References: <20210330052154.26861-1-xuewen.yan94@gmail.com> <34ce11ad-9c20-7ba7-90d8-4830725bf38a@arm.com> <1ebddd33-4666-1e6e-7788-a3fe28c9e99c@arm.com> Message-ID: Date: Mon, 12 Apr 2021 11:26:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, > > > > Hi, > > > > > I test the patch, but the overflow still exists. > > > > > In the "sched/fair: Use pd_cache to speed up > > > find_energy_efficient_cpu()" > > > > > I wonder why recompute the cpu util when cpu==dst_cpu in > > > compute_energy(), > > > > > when the dst_cpu's util change, it also would cause the overflow. > > > > > > > > The patches aim to cache the energy values for the CPUs whose > > > > utilization is not modified (so we don't have to compute it multiple > > > > times). The values cached are the 'base values' of the CPUs, > i.e. when > > > > the task is not placed on the CPU. When (cpu==dst_cpu) in > > > > compute_energy(), it means the energy values need to be updated > instead > > > > of using the cached ones. > > > > > > > well, is it better to use the task_util(p) + cache values ? but in > > > this case, the cache > > > values may need more parameters. > > > > This patch-set is not significantly improving the execution time of > > feec(). The results we have so far are an improvement of 5-10% in > > execution time, with feec() being executed in < 10us. So the gain is not > > spectacular. > > well， I meaned to cache all util value and compute energy with caches, > when > (cpu==dst_cpu), use caches instead of updating util, and do not get > util with function: >  "effective_cpu_util()", to compute util with cache. > I add more parameters into pd_cache: > struct pd_cache { >         unsigned long util; >         unsigned long util_est; >         unsigned long util_cfs; >         unsigned long util_irq; >         unsigned long util_rt; >         unsigned long util_dl; >         unsigned long bw_dl; >         unsigned long freq_util; >         unsigned long nrg_util; > }; > In this way, it can avoid util update while feec. I tested with it, > and the negative delta disappeared. > Maybe this is not a good method, but it does work. If I understand correctly, you put all the fields used by core.c:effective_cpu_util() in the caches, allowing to have values not subject to updates. core.c:effective_cpu_util() isn't only called from fair.c:compute_energy(). It is used in the cpufreq_schedutil.c and cpufreq_cooling.c (through core.c:sched_cpu_util()). Did you have to duplicate core.c:effective_cpu_util() to have a second version using the caches ? If yes, I think the function was meant to be unique so that all the utilization estimations go through the same path. If your concern is to avoid negative delta, I think just bailing out when this happens should be sufficient. As shown in the last message, having a wrong placement should not happen that often, plus the prev_cpu should be used which should be ok. If you want to cache the values, I think a stronger justification will be asked: this seems to be a big modification compared to the initial issue, knowing that another simpler solution is available (i.e. bailing out). I was not able to prove there was a significant gain in the find_energy_efficient_cpu() execution time, but I would be happy if you can, or if you find other arguments. > > > > > > > > > You are right, there is still a possibility to have a negative delta > > > > with the patches at: > > > > > > > > https://gitlab.arm.com/linux-arm/linux-power/-/commits/eas/next/integration-20210129 > > > > > > > > > > Adding a check before subtracting the values, and bailing out in > such > > > > case would avoid this, such as at: > > > > > https://gitlab.arm.com/linux-arm/linux-pg/-/commits/feec_bail_out/ > > > > > > > > > > > > > In your patch, you bail out the case by "go to fail", that means you > > > don't use eas in such > > > case. However, in the actual scene, the case often occurr when select > > > cpu for small task. > > > As a result, the small task would not select cpu according to the eas, > > > it may affect > > > power consumption? > > With this patch (bailing out), the percentage of feec() returning due to > > a negative delta I get are: > > on a Juno-r2, with 2 big CPUs and 4 CPUs (capacity of 383), with a > > workload running during 5s with task having a period of 16 ms and: > >   - 50 tasks at 1%:   0.14% > >   - 30 tasks at 1%:   0.54% > >   - 10 tasks at 1%: < 0.1% > >   - 30 tasks at 5%: < 0.1% > >   - 10 tasks at 5%: < 0.1% > > It doesn't happen so often to me.If we bail out of feec(), the task will > > still have another opportunity in the next call. However I agree this > > can lead to a bad placement when this happens. > > > > > > > I think a similar modification should be done in your patch. > Even though > > > > this is a good idea to group the calls to compute_energy() to > reduce the > > > > chances of having updates of utilization values in between the > > > > compute_energy() calls, > > > > there is still a chance to have updates. I think it happened when I > > > > applied your patch. > > > > > > > > About changing the delta(s) from 'unsigned long' to 'long', I am not > > > > sure of the meaning of having a negative delta. I thing it would be > > > > better to check and fail before it happens instead. > > > > Regards