lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 12 Aug 2020 09:49:49 +0800 From: Qi Zheng <arch0.zheng@...il.com> To: Ingo Molnar <mingo@...nel.org>, valentin.schneider@....com Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org, dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de, linux-kernel@...r.kernel.org Subject: Re: [PATCH] sched/core: add unlikely in group_has_capacity() On 2020/8/7 上午10:47, Qi Zheng wrote: > Yeah, because of the following two points, I also think > the probability is 0%: > a) the sd is protected by rcu lock, and load_balance() > func is between rcu_read_lock() and rcu_read_unlock(). > b) the sgs is a local variable. > > So in the group_classify(), the env->sd->imbalance_pct and > the sgs will not be changed. May I remove the duplicate check > from group_has_capacity() and resubmit a patch? > > Yours, > Qi Zheng > > On 2020/8/6 下午10:45, Ingo Molnar wrote: >> >> * Qi Zheng <arch0.zheng@...il.com> wrote: >> >>> 1. The group_has_capacity() function is only called in >>> group_classify(). >>> 2. Before calling the group_has_capacity() function, >>> group_is_overloaded() will first judge the following >>> formula, if it holds, the group_classify() will directly >>> return the group_overloaded. >>> >>> (sgs->group_capacity * imbalance_pct) < >>> (sgs->group_runnable * 100) >>> >>> Therefore, when the group_has_capacity() is called, the >>> probability that the above formalu holds is very small. Hint >>> compilers about that. >>> >>> Signed-off-by: Qi Zheng <arch0.zheng@...il.com> >>> --- >>> kernel/sched/fair.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >>> index 2ba8f230feb9..9074fd5e23b2 100644 >>> --- a/kernel/sched/fair.c >>> +++ b/kernel/sched/fair.c >>> @@ -8234,8 +8234,8 @@ group_has_capacity(unsigned int imbalance_pct, >>> struct sg_lb_stats *sgs) >>> if (sgs->sum_nr_running < sgs->group_weight) >>> return true; >>> - if ((sgs->group_capacity * imbalance_pct) < >>> - (sgs->group_runnable * 100)) >>> + if (unlikely((sgs->group_capacity * imbalance_pct) < >>> + (sgs->group_runnable * 100))) >>> return false; >> >> Isn't the probability that this second check will match around 0%? >> >> I.e. wouldn't the right fix be to remove the duplicate check from >> group_has_capacity(), because it's already been checked in >> group_classify()? Maybe while leaving a comment in place? >> >> Thanks, >> >> Ingo >> Hi, As Valentin and I discussed in the patch below, simply removing the check may not be completely harmless. [PATCH]sched/fair: Remove the duplicate check from group_has_capacity() : - if ((sgs->group_capacity * imbalance_pct) < - (sgs->group_runnable * 100)) - return false; 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(). But in the case of sum_nr_running == group_weight, we can run to this check. Although I also think it is unlikely to cause the significant capacity pressure at the == case, but I'm not sure whether there are some special scenarios. such as some cpus in sg->cpumask are no longer active, or other scenarios? So adding the unlikely() in group_has_capacity() may be the safest way. Add Valentin Schneider <valentin.schneider@....com>. Yours, Qi Zheng
Powered by blists - more mailing lists