On Thu, 3 Jun 2021 at 15:13, Odin Ugedal [off-list ref] wrote:

Hi,

quoted

Out of curiosity, why did you decide to use
cfs_rq->tg_load_avg_contrib instead of !cfs_rq_is_decayed(cfs_rq)
which is used to delete the cfs_rq from the list when updating blocked
load ?

Well, the main reason was that it is currently (without the other in
flight patches) not safe to just use "cfs_rq_is_decayed" directly,
since that could result in
a situation where tg_load_avg_contrib!=0 while
cfs_rq_is_decayed()==true. I guess we can use cfs_rq_is_decayed() if
you prefer that,
and all the other PELT patches are merged. (This was initially why I
thought a new field was a simpler and more elegant solution to make
sure we book-keep correctly,
but when the PELT stuff is fixed properly, that should be no real
issue as long it works as we expect).

If it's only a matter of waiting other PELT patches to be merged, we
should use cfs_rq_is_decayed().

cfs_rq->tg_load_avg_contrib is used to reduce contention on
tg->load_avg but it is outside the scope of PELT and the update of
blocked load so we should avoid using it there

I was also thinking about the cfs_rq->nr_running part; is there a
chance of a situation where a cfs_rq->nr_running==1 and it has no
load, resulting in it being decayed and
removed from the list in "__update_blocked_fair"? I have not looked
properly at it, but just wondering if that is actually possible..


Also, out of curiosity, are there some implications of a situation
where tg_load_avg_contrib=0 while *_load!=0, or would that not cause

do you mean all cfs_rq->avg.load_avg with *_load ?

fairness issues?

if load_avg!=0, we will update it periodically and sync
tg_load_avg_contrib with the former. So it's not a problem.

The other way was a problem because we stop updating load_avg and
tg_load_avg_contrib when load_avg/load_sum is null so the
tg_load_avg_contrib is stalled with a possibly very old value

Thanks
Odin