Messages in this thread | | | Date | Wed, 13 Mar 2024 11:21:26 +0100 | Subject | Re: [PATCH] sched/fair: simplify __calc_delta() | From | 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@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. >
| |