Re: [PATCH] sched/fair: Remove the duplicate check from group_has_capacity()
From: Qi Zheng
Date: Tue Aug 11 2020 - 20:46:27 EST
On 2020/8/12 上午4:16, Valentin Schneider wrote:
On 11/08/20 14:12, Qi Zheng wrote:
On 2020/8/11 下午8:48, Valentin Schneider wrote:
On 11/08/20 12:44, Qi Zheng wrote:
In fact, at the beginning, I added unlikely() here to hint the compiler:
- if ((sgs->group_capacity * imbalance_pct) <
- (sgs->group_runnable * 100))
+ if (unlikely((sgs->group_capacity * imbalance_pct) <
+ (sgs->group_runnable * 100)))
The corresponding patch is as follows:
[PATCH]sched/core: add unlikely in group_has_capacity()
Do you think it is necessary?
The "unlikely" approach has the benefit of keeping all corner cases in
place. I was tempted to say it could still make sense to get rid of the
extra check entirely, given that it has an impact only when:
- sum_nr_running == group_weight
- group capacity has been noticeably reduced
If sum_nr_running < group_weight, we won't evaluate it.
If sum_nr_running > group_weight, we either won't call into
group_has_capacity() or we'll have checked it already in
group_overloaded().
That said, it does make very much sense to check it in that ==
case. Vincent might have a different take on this, but right now I'd say
the unlikely approach is the safest one of the two.
So what should I do next? Do I resubmit a patch with unlikely() or
add your email to the old patch([PATCH]sched/core: add unlikely in
group_has_capacity())? Or continue to wait for suggestions from
other maintainers?
I guess you can add a reply to the original thread where you had the
unlikely() to point out *removing* the check isn't 100% harmless.
Vincent might want to have a look at it, but AFAIA he's on holidays ATM.
Okay, I will reply to the old patch and add your email to it.
Thanks for your comments.
Yours,
Qi Zheng