Messages in this thread | | | Subject | Re: [PATCH] sched/fair: Remove the duplicate check from group_has_capacity() | From | Qi Zheng <> | Date | Wed, 12 Aug 2020 08:46:16 +0800 |
| |
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
| |