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

[Tim Chen <tim.c.chen@linux.intel.com>]

On Wed, 2026-11-04 at 22:13 +0800, Jianyong Wu wrote:
> sched_balance_find_src_group() only considers group_llc_balance when
> sds.prefer_sibling is set:
> 
> 	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;
> 
> sds.prefer_sibling mirrors SD_PREFER_SIBLING of the busiest group's child
> sched domain, and sd_init() clears that flag on every SD_NUMA domain.
> So on any domain whose child is itself a NUMA domain, i.e. on systems
> with more than one NUMA level such as multiple nodes per socket, groups
> tagged with group_llc_balance by llc_balance() can never take the
> force_balance path, and the group_llc_balance test there is dead code.
> 
> Such a group falls into the !group_overloaded branch below and in the
> common case ends up in out_balanced, so a task that was placed beyond
> the first NUMA level is never pulled into its preferred LLC.
> 
> On a 2-socket Hygon system (8 nodes, 32 LLCs, 4 cores per LLC), with
> "spin 3"/"spin 4" (3/4 busy threads of one process), all threads were
> in one LLC in only 41/140 and 0/142 samples; every failing case had a
> straggler on the other socket. With this patch: 195/196 and 180/196.
> 
> Only require sds.prefer_sibling for the sibling_imbalance() case, which
> is what it was originally introduced for.


Hi Jianyong,

Here's a suggested reorg of the patch to make the code logic clearer.  Also
incorporate some suggestions from Peter on your previous proposal
https://lore.kernel.org/all/20260827122816.756234-15-wujianyong@hygon.cn/ .
So we do not do the llc_balance check for non cache aware scheduling case.

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@hygon.cn>
> Signed-off-by: Jianyong Wu <wujianyong@hygon.cn>
> ---
>  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) {