Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/fair: Remove the duplicate check from group_has_capacity() | Date | Tue, 11 Aug 2020 21:16:26 +0100 |
| |
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.
| |