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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 10 Aug 2020 19:33:20 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Qi Zheng <arch0.zheng@...il.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/fair: Remove the duplicate check from group_has_capacity()


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.


For the

    sgs->sum_nr_running > sgs->group_weight

case I agree with your patch, there just is that oddity at the == case.

> So just remove the duplicate check from group_has_capacity().
>
> Signed-off-by: Qi Zheng <arch0.zheng@...il.com>
> ---
>  kernel/sched/fair.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 2ba8f230feb9..a41903fb327a 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8234,10 +8234,6 @@ 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))
> -		return false;
> -
>       if ((sgs->group_capacity * 100) >
>                       (sgs->group_util * imbalance_pct))
>               return true;

Powered by blists - more mailing lists