Messages in this thread | | | Subject | Re: [PATCH] sched/fair: Remove the duplicate check from group_has_capacity() | From | Qi Zheng <> | Date | Tue, 11 Aug 2020 21:12:17 +0800 |
| |
On 2020/8/11 下午8:48, Valentin Schneider wrote: > > On 11/08/20 12:44, Qi Zheng wrote: >> On 2020/8/11 下午6:38, Valentin Schneider wrote: >>> >>> On 11/08/20 04:39, Qi Zheng wrote: >>>> On 2020/8/11 上午2:33, Valentin Schneider wrote: >>>>> >>>>> On 10/08/20 02:00, Qi Zheng wrote: >>>>>> 1. The group_has_capacity() function is only called in >>>>>> group_classify(). >>>>>> 2. The following inequality has already been checked in >>>>>> group_is_overloaded() which was also called in >>>>>> group_classify(). >>>>>> >>>>>> (sgs->group_capacity * imbalance_pct) < >>>>>> (sgs->group_runnable * 100) >>>>>> >>>>> >>>>> Consider group_is_overloaded() returns false because of the first >>>>> condition: >>>>> >>>>> if (sgs->sum_nr_running <= sgs->group_weight) >>>>> return false; >>>>> >>>>> then group_has_capacity() would be the first place where the group_runnable >>>>> vs group_capacity comparison would be done. >>>>> >>>>> Now in that specific case we'll actually only check it if >>>>> >>>>> sgs->sum_nr_running == sgs->group_weight >>>>> >>>>> and the only case where the runnable vs capacity check can fail here is if >>>>> there's significant capacity pressure going on. TBH this capacity pressure >>>>> could be happening even when there are fewer tasks than CPUs, so I'm not >>>>> sure how intentional that corner case is. >>>> >>>> Maybe some cpus in sg->cpumask are no longer active at the == case, >>>> which causes the significant capacity pressure? >>>> >>> >>> That can only happen in that short window between deactivating a CPU and >>> not having rebuilt the sched_domains yet, which sounds quite elusive. >>> >> >> 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?
| |