RE: [PATCH] sched/cache: Fix group_llc_balance being unreachable above the first NUMA level

[Jianyong Wu]

Hi Tim,


> So we do not do the llc_balance check for non cache aware scheduling
> case.
>

Thanks, this is clearer than my version, and cheaper too: when cache aware
scheduling is disabled the group type check is either compiled out or skipped
by the static branch.

This patch comes from my earlier series that scales cache aware scheduling at
LLC granularity.  Yibin found that without it mainline does not pull a task
back into its preferred LLC once the task sits beyond the first NUMA level, so
I picked it out and sent it as a standalone fix.

I'll include your version in v2.

Thanks
Jianyong


> Tim
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index cfc982205b98..8a7442189a3b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -12192,6 +12192,16 @@ static bool update_llc_busiest(struct lb_env
> *env,
>          */
>         return sgs->nr_pref_dst_llc > busiest->nr_pref_dst_llc;
>  }
> +
> +/*
> + * The busiest group has tasks that prefer the destination LLC and
> + * cache aware scheduling is enabled.
> + */
> +static inline bool busiest_llc_balance(struct sg_lb_stats *busiest)
> +{
> +       return sched_cache_enabled() &&
> +              busiest->group_type == group_llc_balance;
> +}
>  #else
>  static inline void record_sg_llc_stats(struct lb_env *env, struct sg_lb_stats
> *sgs,
>                                        struct sched_group *group)
> @@ -12210,6 +12220,11 @@ static bool update_llc_busiest(struct lb_env
> *env,
>  {
>         return false;
>  }
> +
> +static inline bool busiest_llc_balance(struct sg_lb_stats *busiest)
> +{
> +       return false;
> +}
>  #endif
> 
>  /**
> @@ -13303,14 +13318,25 @@ static struct sched_group
> *sched_balance_find_src_group(struct lb_env *env)
>                         goto out_balanced;
>         }
> 
> -       /*
> -        * Try to move all excess tasks to a sibling domain of the busiest
> -        * group's child domain.
> -        */
> -       if (sds.prefer_sibling && local->group_type == group_has_spare
> &&
> -           (busiest->group_type == group_llc_balance ||
> -           sibling_imbalance(env, &sds, busiest, local) > 1))
> -               goto force_balance;
> +       if (local->group_type == group_has_spare) {
> +
> +               if (busiest_llc_balance(busiest))
> +                       goto force_balance;
> +
> +               /*
> +                * Try to move all excess tasks to a sibling domain of
> the
> +                * busiest group's child domain.
> +                */
> +               if (sds.prefer_sibling &&
> +                   sibling_imbalance(env, &sds, busiest, local) > 1)
> +                       goto force_balance;
> +       }
> 
> 
> >
> > Fixes: f38cc2f0d8a3 ("sched/cache: Prioritize tasks preferring destination
> LLC during balancing")
> > Reported-by: Yibin Liu <liuyibin@xxxxxxxx>
> > Signed-off-by: Jianyong Wu <wujianyong@xxxxxxxx>
> > ---
> >  kernel/sched/fair.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 57360f5cdde4..205a0891b0a0 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -13294,9 +13294,10 @@ static struct sched_group
> *sched_balance_find_src_group(struct lb_env *env)
> >  	 * Try to move all excess tasks to a sibling domain of the busiest
> >  	 * group's child domain.
> >  	 */
> > -	if (sds.prefer_sibling && local->group_type == group_has_spare &&
> > +	if (local->group_type == group_has_spare &&
> >  	    (busiest->group_type == group_llc_balance ||
> > -	    sibling_imbalance(env, &sds, busiest, local) > 1))
> > +	     (sds.prefer_sibling &&
> > +	      sibling_imbalance(env, &sds, busiest, local) > 1)))
> >  		goto force_balance;
> >
> >  	if (busiest->group_type != group_overloaded) {