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

[Pierre Gondois]

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@xxxxxxx> 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.