Re: [PATCH] sched/fair: simplify __calc_delta()

[Pierre Gondois <pierre.gondois@arm.com>]

Hello Dawei,

On 3/13/24 00:25, Dawei Li wrote:
> Hi Pierre,
> Thank you for the review!
> 
> On Tue, Mar 12, 2024 at 6:18 AM Pierre Gondois <pierre.gondois@arm.com> wrote:
>>
>> Hello Dawei,
>>
>> On 3/6/24 23:28, Dawei Li wrote:
>>> Based on how __calc_delta() is called now, the input parameter, weight
>>> is always NICE_0_LOAD. I think we don't need it as an input parameter
>>> now?
>>
>> Maybe
>>     5e963f2bd4654a202a8a05aa3a86cb0300b10e6c ("sched/fair: Commit to EEVDF")
>> should be referenced to explain that the case where (weight =< lw.weight)
>> doesn't exist anymore and that NICE_0_LOAD could be incorporated in
>> __calc_delta() directly.
>>
>>
>> Also I think indirect forms are preferred in general:
>> "I think we don't need it as an input parameter now ?" ->
>> "The 'weight' parameter doesn't seem to be required anymore"
>> (same note for the whole commit message)
>>
>>>
>>> Also, when weight is always NICE_0_LOAD, the initial fact value is
>>> always 2^10, and the first fact_hi will always be 0. Thus, we can get
>>> rid of the first if bock.
>>>
>>> The previous comment "(delta_exec * (weight * lw->inv_weight)) >>
>>> WMULT_SHIFT" seems to be assuming that lw->weight * lw->inv_weight is
>>> always (approximately) equal to 2^WMULT_SHIFT. However, when
>>> CONFIG_64BIT is set, lw->weight * lw->inv_weight is (approximately)
>>> equal to 2^WMULT_SHIFT * 2^10. What remains true for both CONFIG_32BIT
>>> and CONFIG_64BIT is: scale_load_down(lw->weight) * lw->inv_weight is
>>> (approximately) equal to 2^WMULT_SHIFT. (Correct me if I am wrong.)
>>
>> I think the comment is more about explaining that:
>>     X * lw.weight
>> equals:
>>     X * lw->inv_weight >> WMULT_SHIFT
>>
> I assume you mean
> X / lw->weight
> equals:
> X * lw->inv_weight >> WMULT_SHIFT

Yes right indeed.

> However, this is not always true, and that's why I'd like to revise
> it. It is true for
> CONFIG_32BIT. However, For CONFIG_64BIT, we have lw->weight * lw->inv_weight =
> 2**WMULT_SHIFT * 2**10. Thus,
> X / lw->weight
> equals:
> X * lw->inv_weight >> (WMULT_SHIFT + 10)

Ok yes, you're correct indeed.
The equality is always correct when scale_load_down() is used,

Regards,
Pierre

> 
> 
>> Also, if CONFIG_64BIT is set, we should have:
>>     weight / lw.weight == scale_load_down(lw->weight) * 2**10 * lw->inv_weight
>>
> 
> weight / lw->weight should be equal to scale_load_down(weight) /
> scale_load_down(lw->weight)
> = scale_load_down(weight) * lw->inv_weight / (2**WMULT_SHIFT)
> Right?
> 
>> So IIUC, either both lines should be update, either none.
>> (meaning that:
>>     delta_exec * NICE_0_LOAD / lw->weight
>> should be changed to
>>     delta_exec * scale_load_down(NICE_0_LOAD) / lw->weight
> 
> I think this is not correct? scale_load_down(NICE_0_LOAD) is the true
> weight, as mapped
> directly from the task's nice/priority value, while lw->weight is the
> scaled_up load.
> Their units/scales don't match.
>