lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 2 Apr 2024 14:14:31 +0530
From: Vishal Chourasia <vishalc@...ux.ibm.com>
To: Dawei Li <daweilics@...il.com>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>,
        Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira
 <bristot@...hat.com>,
        Valentin Schneider <vschneid@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] sched/fair: fix initial util_avg calculation

On 02/04/24 11:17 am, Dawei Li wrote:
> Hi Vishal
> 
> Thanks for the comment!
> Do you suggest using scale_load_down() in place of se_weight()?
scale_load_down should be better.
> It's a soft bug we should fix one way or another before what the
> comment mentions really happens.
IIUC, We should be moving towards using full load resolution
for all the calculations. In that case, we need not worry about scaling load at
all. Maybe someone could provide context here.

> I am actually confused that we have both se_weight() and
> scale_load_down(), and they do the same thing.
> 
> Best regards,
> Dawei
> 
> On Mon, Apr 1, 2024 at 3:36 AM Vishal Chourasia <vishalc@...ux.ibm.com> wrote:
>>
>> On Thu, Mar 14, 2024 at 06:59:16PM -0700, Dawei Li wrote:
>>> Change se->load.weight to se_weight(se) in the calculation for the
>>> initial util_avg to avoid unnecessarily inflating the util_avg by 1024
>>> times.
>>>
>>> The reason is that se->load.weight has the unit/scale as the scaled-up
>>> load, while cfs_rg->avg.load_avg has the unit/scale as the true task
>>> weight (as mapped directly from the task's nice/priority value). With
>>> CONFIG_32BIT, the scaled-up load is equal to the true task weight. With
>>> CONFIG_64BIT, the scaled-up load is 1024 times the true task weight.
>>> Thus, the current code may inflate the util_avg by 1024 times. The
>>> follow-up capping will not allow the util_avg value to go wild. But the
>>> calculation should have the correct logic.
>>>
>>> Signed-off-by: Dawei Li <daweilics@...il.com>
>>> ---
>>> Changes in v2:
>>> - update the commit message
>>> ---
>>>  kernel/sched/fair.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index a19ea290b790..5f98f639bdb9 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -1031,7 +1031,8 @@ void init_entity_runnable_average(struct sched_entity *se)
>>>   * With new tasks being created, their initial util_avgs are extrapolated
>>>   * based on the cfs_rq's current util_avg:
>>>   *
>>> - *   util_avg = cfs_rq->util_avg / (cfs_rq->load_avg + 1) * se.load.weight
>>> + *   util_avg = cfs_rq->avg.util_avg / (cfs_rq->avg.load_avg + 1)
>>> + *           * se_weight(se)
>>>   *
>>>   * However, in many cases, the above util_avg does not give a desired
>>>   * value. Moreover, the sum of the util_avgs may be divergent, such
>>> @@ -1078,7 +1079,7 @@ void post_init_entity_util_avg(struct task_struct *p)
>>>
>>>       if (cap > 0) {
>>>               if (cfs_rq->avg.util_avg != 0) {
>>> -                     sa->util_avg  = cfs_rq->avg.util_avg * se->load.weight;
>>> +                     sa->util_avg  = cfs_rq->avg.util_avg * se_weight(se);
>> Hi,
>>
>> The comment above the declaration of se_weight function says we should be
>> using full load resolution and get rid of this helper.
>>
>> Should we be adding new user of the helper?
>>
>> /*
>>  * XXX we want to get rid of these helpers and use the full load resolution.
>>  */
>> static inline long se_weight(struct sched_entity *se)
>> {
>>         return scale_load_down(se->load.weight);
>> }
>>
>>
>>>                       sa->util_avg /= (cfs_rq->avg.load_avg + 1);
>>>
>>>                       if (sa->util_avg > cap)
>>> --
>>> 2.40.1
>>>


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ