diff mbox series

[RESEND,v5,2/2] sched/fair: Scan cluster before scanning LLC in wake-up path

Message ID 20220720081150.22167-3-yangyicong@hisilicon.com (mailing list archive)
State New, archived
Headers show
Series sched/fair: Scan cluster before scanning LLC in wake-up path | expand

Commit Message

Yicong Yang July 20, 2022, 8:11 a.m. UTC 
From: Barry Song <song.bao.hua@hisilicon.com>

For platforms having clusters like Kunpeng920, CPUs within the same cluster
have lower latency when synchronizing and accessing shared resources like
cache. Thus, this patch tries to find an idle cpu within the cluster of the
target CPU before scanning the whole LLC to gain lower latency.

Note neither Kunpeng920 nor x86 Jacobsville supports SMT, so this patch
doesn't consider SMT for this moment.

Testing has been done on Kunpeng920 by pinning tasks to one numa and two
numa. On Kunpeng920, Each numa has 8 clusters and each cluster has 4 CPUs.

With this patch, We noticed enhancement on tbench within one numa or cross
two numa.

On numa 0:
                           tip/core                 patched
Hmean     1        345.89 (   0.00%)      393.96 *  13.90%*
Hmean     2        697.77 (   0.00%)      786.04 *  12.65%*
Hmean     4       1392.51 (   0.00%)     1570.26 *  12.76%*
Hmean     8       2800.61 (   0.00%)     3083.98 *  10.12%*
Hmean     16      5514.27 (   0.00%)     6116.00 *  10.91%*
Hmean     32     10869.81 (   0.00%)    10782.98 *  -0.80%*
Hmean     64      8315.22 (   0.00%)     8519.84 *   2.46%*
Hmean     128     6324.47 (   0.00%)     7159.35 *  13.20%*

On numa 0-1:
                           tip/core                 patched
Hmean     1        348.68 (   0.00%)      387.91 *  11.25%*
Hmean     2        693.57 (   0.00%)      774.91 *  11.73%*
Hmean     4       1369.26 (   0.00%)     1475.48 *   7.76%*
Hmean     8       2772.99 (   0.00%)     2984.61 *   7.63%*
Hmean     16      4825.83 (   0.00%)     5873.13 *  21.70%*
Hmean     32     10250.32 (   0.00%)    11688.06 *  14.03%*
Hmean     64     16309.51 (   0.00%)    19889.48 *  21.95%*
Hmean     128    13022.32 (   0.00%)    16005.64 *  22.91%*
Hmean     256    11335.79 (   0.00%)    13821.74 *  21.93%*

Tested-by: Yicong Yang <yangyicong@hisilicon.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 kernel/sched/fair.c | 44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

Comments

Peter Zijlstra July 20, 2022, 11:14 a.m. UTC | #1 
On Wed, Jul 20, 2022 at 04:11:50PM +0800, Yicong Yang wrote:
> +	/* TODO: Support SMT system with cluster topology */
> +	if (!sched_smt_active() && sd) {
> +		for_each_cpu_and(cpu, cpus, sched_domain_span(sd)) {

So that's no SMT and no wrap iteration..

Does something like this work?

---
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6437,6 +6437,30 @@ static int select_idle_cpu(struct task_s
 		}
 	}
 
+	if (IS_ENABLED(CONFIG_SCHED_CLUSTER) &&
+	    static_branch_unlikely(&sched_cluster_active)) {
+		struct sched_domain *sdc = rcu_dereference(per_cpu(sd_cluster, target));
+		if (sdc) {
+			for_each_cpu_wrap(cpu, sched_domain_span(sdc), target + 1) {
+				if (!cpumask_test_cpu(cpu, cpus))
+					continue;
+
+				if (has_idle_core) {
+					i = select_idle_core(p, cpu, cpus, &idle_cpu);
+					if ((unsigned int)i < nr_cpumask_bits)
+						return i;
+				} else {
+					if (--nr <= 0)
+						return -1;
+					idle_cpu = __select_idle_cpu(cpu, p);
+					if ((unsigned int)idle_cpu < nr_cpumask_bits)
+						break;
+				}
+			}
+			cpumask_andnot(cpus, cpus, sched_domain_span(sdc));
+		}
+	}
+
 	for_each_cpu_wrap(cpu, cpus, target + 1) {
 		if (has_idle_core) {
 			i = select_idle_core(p, cpu, cpus, &idle_cpu);
@@ -6444,7 +6468,7 @@ static int select_idle_cpu(struct task_s
 				return i;
 
 		} else {
-			if (!--nr)
+			if (--nr <= 0)
 				return -1;
 			idle_cpu = __select_idle_cpu(cpu, p);
 			if ((unsigned int)idle_cpu < nr_cpumask_bits)
@@ -6543,7 +6567,7 @@ static int select_idle_sibling(struct ta
 	/*
 	 * If the previous CPU is cache affine and idle, don't be stupid:
 	 */
-	if (prev != target && cpus_share_cache(prev, target) &&
+	if (prev != target && cpus_share_lowest_cache(prev, target) &&
 	    (available_idle_cpu(prev) || sched_idle_cpu(prev)) &&
 	    asym_fits_capacity(task_util, prev))
 		return prev;
@@ -6569,7 +6593,7 @@ static int select_idle_sibling(struct ta
 	p->recent_used_cpu = prev;
 	if (recent_used_cpu != prev &&
 	    recent_used_cpu != target &&
-	    cpus_share_cache(recent_used_cpu, target) &&
+	    cpus_share_lowest_cache(recent_used_cpu, target) &&
 	    (available_idle_cpu(recent_used_cpu) || sched_idle_cpu(recent_used_cpu)) &&
 	    cpumask_test_cpu(p->recent_used_cpu, p->cpus_ptr) &&
 	    asym_fits_capacity(task_util, recent_used_cpu)) {
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1813,7 +1813,9 @@ DECLARE_PER_CPU(struct sched_domain __rc
 DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
 DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
 DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
+
 extern struct static_key_false sched_asym_cpucapacity;
+extern struct static_key_false sched_cluster_active;
 
 struct sched_group_capacity {
 	atomic_t		ref;
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -670,7 +670,9 @@ DEFINE_PER_CPU(struct sched_domain_share
 DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
 DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
 DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
+
 DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
+DEFINE_STATIC_KEY_FALSE(sched_cluster_active);
 
 static void update_top_cache_domain(int cpu)
 {
@@ -2268,6 +2270,7 @@ build_sched_domains(const struct cpumask
 	struct rq *rq = NULL;
 	int i, ret = -ENOMEM;
 	bool has_asym = false;
+	bool has_cluster = false;
 
 	if (WARN_ON(cpumask_empty(cpu_map)))
 		goto error;
@@ -2289,6 +2292,7 @@ build_sched_domains(const struct cpumask
 			sd = build_sched_domain(tl, cpu_map, attr, sd, i);
 
 			has_asym |= sd->flags & SD_ASYM_CPUCAPACITY;
+			has_cluster |= sd->flags & SD_CLUSTER;
 
 			if (tl == sched_domain_topology)
 				*per_cpu_ptr(d.sd, i) = sd;
@@ -2399,6 +2403,9 @@ build_sched_domains(const struct cpumask
 	if (has_asym)
 		static_branch_inc_cpuslocked(&sched_asym_cpucapacity);
 
+	if (has_cluster)
+		static_branch_inc_cpuslocked(&sched_cluster_active);
+
 	if (rq && sched_debug_verbose) {
 		pr_info("root domain span: %*pbl (max cpu_capacity = %lu)\n",
 			cpumask_pr_args(cpu_map), rq->rd->max_cpu_capacity);
@@ -2498,6 +2505,9 @@ static void detach_destroy_domains(const
 	if (rcu_access_pointer(per_cpu(sd_asym_cpucapacity, cpu)))
 		static_branch_dec_cpuslocked(&sched_asym_cpucapacity);
 
+	if (rcu_access_pointer(per_cpu(sd_cluster, cpu)))
+		static_branch_dec_cpuslocked(&sched_cluster_active);
+
 	rcu_read_lock();
 	for_each_cpu(i, cpu_map)
 		cpu_attach_domain(NULL, &def_root_domain, i);
Barry Song July 20, 2022, 11:33 a.m. UTC | #2 
On Wed, Jul 20, 2022 at 11:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Jul 20, 2022 at 04:11:50PM +0800, Yicong Yang wrote:
> > +     /* TODO: Support SMT system with cluster topology */
> > +     if (!sched_smt_active() && sd) {
> > +             for_each_cpu_and(cpu, cpus, sched_domain_span(sd)) {
>
> So that's no SMT and no wrap iteration..
>
> Does something like this work?

I guess it is ok. The main reason for me to disable smt from the first beginning
is that we don't really have a machine with both cluster and smt to
reach the code
for smt, aka has_idle_core=true. so I thought I was going to bring up
that branch
when I really have a machine. so, it is marked as TODO.

till now,  neither kunpeng920 nor Jacobsville is smt.

But I don't mind if it is enabled now.

>
> ---
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6437,6 +6437,30 @@ static int select_idle_cpu(struct task_s
>                 }
>         }
>
> +       if (IS_ENABLED(CONFIG_SCHED_CLUSTER) &&
> +           static_branch_unlikely(&sched_cluster_active)) {
> +               struct sched_domain *sdc = rcu_dereference(per_cpu(sd_cluster, target));
> +               if (sdc) {
> +                       for_each_cpu_wrap(cpu, sched_domain_span(sdc), target + 1) {
> +                               if (!cpumask_test_cpu(cpu, cpus))
> +                                       continue;
> +
> +                               if (has_idle_core) {
> +                                       i = select_idle_core(p, cpu, cpus, &idle_cpu);
> +                                       if ((unsigned int)i < nr_cpumask_bits)
> +                                               return i;
> +                               } else {
> +                                       if (--nr <= 0)
> +                                               return -1;
> +                                       idle_cpu = __select_idle_cpu(cpu, p);
> +                                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
> +                                               break;
> +                               }
> +                       }
> +                       cpumask_andnot(cpus, cpus, sched_domain_span(sdc));
> +               }
> +       }
> +
>         for_each_cpu_wrap(cpu, cpus, target + 1) {
>                 if (has_idle_core) {
>                         i = select_idle_core(p, cpu, cpus, &idle_cpu);
> @@ -6444,7 +6468,7 @@ static int select_idle_cpu(struct task_s
>                                 return i;
>
>                 } else {
> -                       if (!--nr)
> +                       if (--nr <= 0)
>                                 return -1;
>                         idle_cpu = __select_idle_cpu(cpu, p);
>                         if ((unsigned int)idle_cpu < nr_cpumask_bits)
> @@ -6543,7 +6567,7 @@ static int select_idle_sibling(struct ta
>         /*
>          * If the previous CPU is cache affine and idle, don't be stupid:
>          */
> -       if (prev != target && cpus_share_cache(prev, target) &&
> +       if (prev != target && cpus_share_lowest_cache(prev, target) &&
>             (available_idle_cpu(prev) || sched_idle_cpu(prev)) &&
>             asym_fits_capacity(task_util, prev))
>                 return prev;
> @@ -6569,7 +6593,7 @@ static int select_idle_sibling(struct ta
>         p->recent_used_cpu = prev;
>         if (recent_used_cpu != prev &&
>             recent_used_cpu != target &&
> -           cpus_share_cache(recent_used_cpu, target) &&
> +           cpus_share_lowest_cache(recent_used_cpu, target) &&
>             (available_idle_cpu(recent_used_cpu) || sched_idle_cpu(recent_used_cpu)) &&
>             cpumask_test_cpu(p->recent_used_cpu, p->cpus_ptr) &&
>             asym_fits_capacity(task_util, recent_used_cpu)) {
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1813,7 +1813,9 @@ DECLARE_PER_CPU(struct sched_domain __rc
>  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
>  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
>  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> +
>  extern struct static_key_false sched_asym_cpucapacity;
> +extern struct static_key_false sched_cluster_active;
>
>  struct sched_group_capacity {
>         atomic_t                ref;
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -670,7 +670,9 @@ DEFINE_PER_CPU(struct sched_domain_share
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> +
>  DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
> +DEFINE_STATIC_KEY_FALSE(sched_cluster_active);
>
>  static void update_top_cache_domain(int cpu)
>  {
> @@ -2268,6 +2270,7 @@ build_sched_domains(const struct cpumask
>         struct rq *rq = NULL;
>         int i, ret = -ENOMEM;
>         bool has_asym = false;
> +       bool has_cluster = false;
>
>         if (WARN_ON(cpumask_empty(cpu_map)))
>                 goto error;
> @@ -2289,6 +2292,7 @@ build_sched_domains(const struct cpumask
>                         sd = build_sched_domain(tl, cpu_map, attr, sd, i);
>
>                         has_asym |= sd->flags & SD_ASYM_CPUCAPACITY;
> +                       has_cluster |= sd->flags & SD_CLUSTER;
>
>                         if (tl == sched_domain_topology)
>                                 *per_cpu_ptr(d.sd, i) = sd;
> @@ -2399,6 +2403,9 @@ build_sched_domains(const struct cpumask
>         if (has_asym)
>                 static_branch_inc_cpuslocked(&sched_asym_cpucapacity);
>
> +       if (has_cluster)
> +               static_branch_inc_cpuslocked(&sched_cluster_active);
> +
>         if (rq && sched_debug_verbose) {
>                 pr_info("root domain span: %*pbl (max cpu_capacity = %lu)\n",
>                         cpumask_pr_args(cpu_map), rq->rd->max_cpu_capacity);
> @@ -2498,6 +2505,9 @@ static void detach_destroy_domains(const
>         if (rcu_access_pointer(per_cpu(sd_asym_cpucapacity, cpu)))
>                 static_branch_dec_cpuslocked(&sched_asym_cpucapacity);
>
> +       if (rcu_access_pointer(per_cpu(sd_cluster, cpu)))
> +               static_branch_dec_cpuslocked(&sched_cluster_active);
> +
>         rcu_read_lock();
>         for_each_cpu(i, cpu_map)
>                 cpu_attach_domain(NULL, &def_root_domain, i);

Thanks
Barry
Barry Song July 21, 2022, 9:38 a.m. UTC | #3 
On Wed, Jul 20, 2022 at 11:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Jul 20, 2022 at 04:11:50PM +0800, Yicong Yang wrote:
> > +     /* TODO: Support SMT system with cluster topology */
> > +     if (!sched_smt_active() && sd) {
> > +             for_each_cpu_and(cpu, cpus, sched_domain_span(sd)) {
>
> So that's no SMT and no wrap iteration..
>
> Does something like this work?
>
> ---
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6437,6 +6437,30 @@ static int select_idle_cpu(struct task_s
>                 }
>         }
>
> +       if (IS_ENABLED(CONFIG_SCHED_CLUSTER) &&
> +           static_branch_unlikely(&sched_cluster_active)) {
> +               struct sched_domain *sdc = rcu_dereference(per_cpu(sd_cluster, target));
> +               if (sdc) {
> +                       for_each_cpu_wrap(cpu, sched_domain_span(sdc), target + 1) {
> +                               if (!cpumask_test_cpu(cpu, cpus))
> +                                       continue;
> +
> +                               if (has_idle_core) {
> +                                       i = select_idle_core(p, cpu, cpus, &idle_cpu);
> +                                       if ((unsigned int)i < nr_cpumask_bits)
> +                                               return i;
> +                               } else {
> +                                       if (--nr <= 0)
> +                                               return -1;
> +                                       idle_cpu = __select_idle_cpu(cpu, p);
> +                                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
> +                                               break;

Guess here it should be "return idle_cpu", but not "break". as "break"
will make us scan more
other cpus outside the cluster if we have found idle_cpu within the cluster.

Yicong,
Please test Peter's code with the above change.


> +                               }
> +                       }
> +                       cpumask_andnot(cpus, cpus, sched_domain_span(sdc));
> +               }
> +       }
> +
>         for_each_cpu_wrap(cpu, cpus, target + 1) {
>                 if (has_idle_core) {
>                         i = select_idle_core(p, cpu, cpus, &idle_cpu);
> @@ -6444,7 +6468,7 @@ static int select_idle_cpu(struct task_s
>                                 return i;
>
>                 } else {
> -                       if (!--nr)
> +                       if (--nr <= 0)
>                                 return -1;
>                         idle_cpu = __select_idle_cpu(cpu, p);
>                         if ((unsigned int)idle_cpu < nr_cpumask_bits)
> @@ -6543,7 +6567,7 @@ static int select_idle_sibling(struct ta
>         /*
>          * If the previous CPU is cache affine and idle, don't be stupid:
>          */
> -       if (prev != target && cpus_share_cache(prev, target) &&
> +       if (prev != target && cpus_share_lowest_cache(prev, target) &&
>             (available_idle_cpu(prev) || sched_idle_cpu(prev)) &&
>             asym_fits_capacity(task_util, prev))
>                 return prev;
> @@ -6569,7 +6593,7 @@ static int select_idle_sibling(struct ta
>         p->recent_used_cpu = prev;
>         if (recent_used_cpu != prev &&
>             recent_used_cpu != target &&
> -           cpus_share_cache(recent_used_cpu, target) &&
> +           cpus_share_lowest_cache(recent_used_cpu, target) &&
>             (available_idle_cpu(recent_used_cpu) || sched_idle_cpu(recent_used_cpu)) &&
>             cpumask_test_cpu(p->recent_used_cpu, p->cpus_ptr) &&
>             asym_fits_capacity(task_util, recent_used_cpu)) {
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1813,7 +1813,9 @@ DECLARE_PER_CPU(struct sched_domain __rc
>  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_numa);
>  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
>  DECLARE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> +
>  extern struct static_key_false sched_asym_cpucapacity;
> +extern struct static_key_false sched_cluster_active;
>
>  struct sched_group_capacity {
>         atomic_t                ref;
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -670,7 +670,9 @@ DEFINE_PER_CPU(struct sched_domain_share
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_numa);
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_packing);
>  DEFINE_PER_CPU(struct sched_domain __rcu *, sd_asym_cpucapacity);
> +
>  DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity);
> +DEFINE_STATIC_KEY_FALSE(sched_cluster_active);
>
>  static void update_top_cache_domain(int cpu)
>  {
> @@ -2268,6 +2270,7 @@ build_sched_domains(const struct cpumask
>         struct rq *rq = NULL;
>         int i, ret = -ENOMEM;
>         bool has_asym = false;
> +       bool has_cluster = false;
>
>         if (WARN_ON(cpumask_empty(cpu_map)))
>                 goto error;
> @@ -2289,6 +2292,7 @@ build_sched_domains(const struct cpumask
>                         sd = build_sched_domain(tl, cpu_map, attr, sd, i);
>
>                         has_asym |= sd->flags & SD_ASYM_CPUCAPACITY;
> +                       has_cluster |= sd->flags & SD_CLUSTER;
>
>                         if (tl == sched_domain_topology)
>                                 *per_cpu_ptr(d.sd, i) = sd;
> @@ -2399,6 +2403,9 @@ build_sched_domains(const struct cpumask
>         if (has_asym)
>                 static_branch_inc_cpuslocked(&sched_asym_cpucapacity);
>
> +       if (has_cluster)
> +               static_branch_inc_cpuslocked(&sched_cluster_active);
> +
>         if (rq && sched_debug_verbose) {
>                 pr_info("root domain span: %*pbl (max cpu_capacity = %lu)\n",
>                         cpumask_pr_args(cpu_map), rq->rd->max_cpu_capacity);
> @@ -2498,6 +2505,9 @@ static void detach_destroy_domains(const
>         if (rcu_access_pointer(per_cpu(sd_asym_cpucapacity, cpu)))
>                 static_branch_dec_cpuslocked(&sched_asym_cpucapacity);
>
> +       if (rcu_access_pointer(per_cpu(sd_cluster, cpu)))
> +               static_branch_dec_cpuslocked(&sched_cluster_active);
> +
>         rcu_read_lock();
>         for_each_cpu(i, cpu_map)
>                 cpu_attach_domain(NULL, &def_root_domain, i);

Thanks
Barry
Peter Zijlstra July 21, 2022, 10:33 a.m. UTC | #4 
On Thu, Jul 21, 2022 at 09:38:04PM +1200, Barry Song wrote:
> On Wed, Jul 20, 2022 at 11:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Jul 20, 2022 at 04:11:50PM +0800, Yicong Yang wrote:
> > > +     /* TODO: Support SMT system with cluster topology */
> > > +     if (!sched_smt_active() && sd) {
> > > +             for_each_cpu_and(cpu, cpus, sched_domain_span(sd)) {
> >
> > So that's no SMT and no wrap iteration..
> >
> > Does something like this work?
> >
> > ---
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6437,6 +6437,30 @@ static int select_idle_cpu(struct task_s
> >                 }
> >         }
> >
> > +       if (IS_ENABLED(CONFIG_SCHED_CLUSTER) &&
> > +           static_branch_unlikely(&sched_cluster_active)) {
> > +               struct sched_domain *sdc = rcu_dereference(per_cpu(sd_cluster, target));
> > +               if (sdc) {
> > +                       for_each_cpu_wrap(cpu, sched_domain_span(sdc), target + 1) {
> > +                               if (!cpumask_test_cpu(cpu, cpus))
> > +                                       continue;
> > +
> > +                               if (has_idle_core) {
> > +                                       i = select_idle_core(p, cpu, cpus, &idle_cpu);
> > +                                       if ((unsigned int)i < nr_cpumask_bits)
> > +                                               return i;
> > +                               } else {
> > +                                       if (--nr <= 0)
> > +                                               return -1;
> > +                                       idle_cpu = __select_idle_cpu(cpu, p);
> > +                                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
> > +                                               break;
> 
> Guess here it should be "return idle_cpu", but not "break". as "break"
> will make us scan more
> other cpus outside the cluster if we have found idle_cpu within the cluster.
> 
> Yicong,
> Please test Peter's code with the above change.

Indeed. Sorry for that.
Yicong Yang July 21, 2022, 12:42 p.m. UTC | #5 
On 2022/7/21 18:33, Peter Zijlstra wrote:
> On Thu, Jul 21, 2022 at 09:38:04PM +1200, Barry Song wrote:
>> On Wed, Jul 20, 2022 at 11:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
>>>
>>> On Wed, Jul 20, 2022 at 04:11:50PM +0800, Yicong Yang wrote:
>>>> +     /* TODO: Support SMT system with cluster topology */
>>>> +     if (!sched_smt_active() && sd) {
>>>> +             for_each_cpu_and(cpu, cpus, sched_domain_span(sd)) {
>>>
>>> So that's no SMT and no wrap iteration..
>>>
>>> Does something like this work?
>>>
>>> ---
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -6437,6 +6437,30 @@ static int select_idle_cpu(struct task_s
>>>                 }
>>>         }
>>>
>>> +       if (IS_ENABLED(CONFIG_SCHED_CLUSTER) &&
>>> +           static_branch_unlikely(&sched_cluster_active)) {
>>> +               struct sched_domain *sdc = rcu_dereference(per_cpu(sd_cluster, target));
>>> +               if (sdc) {
>>> +                       for_each_cpu_wrap(cpu, sched_domain_span(sdc), target + 1) {
>>> +                               if (!cpumask_test_cpu(cpu, cpus))
>>> +                                       continue;
>>> +
>>> +                               if (has_idle_core) {
>>> +                                       i = select_idle_core(p, cpu, cpus, &idle_cpu);
>>> +                                       if ((unsigned int)i < nr_cpumask_bits)
>>> +                                               return i;
>>> +                               } else {
>>> +                                       if (--nr <= 0)
>>> +                                               return -1;
>>> +                                       idle_cpu = __select_idle_cpu(cpu, p);
>>> +                                       if ((unsigned int)idle_cpu < nr_cpumask_bits)
>>> +                                               break;
>>
>> Guess here it should be "return idle_cpu", but not "break". as "break"
>> will make us scan more
>> other cpus outside the cluster if we have found idle_cpu within the cluster.
>>

That can explain why the performance regress when underload.

>> Yicong,
>> Please test Peter's code with the above change.
> 
> Indeed. Sorry for that.
> 

The performance's still positive based on the tip/sched/core used in this patch's commit.
70fb5ccf2ebb ("sched/fair: Introduce SIS_UTIL to search idle CPU based on sum of util_avg").

On numa 0:
                           tip/core                 patched
Hmean     1        345.89 (   0.00%)      398.43 *  15.19%*
Hmean     2        697.77 (   0.00%)      794.40 *  13.85%*
Hmean     4       1392.51 (   0.00%)     1577.60 *  13.29%*
Hmean     8       2800.61 (   0.00%)     3118.38 *  11.35%*
Hmean     16      5514.27 (   0.00%)     6124.51 *  11.07%*
Hmean     32     10869.81 (   0.00%)    10690.97 *  -1.65%*
Hmean     64      8315.22 (   0.00%)     8520.73 *   2.47%*
Hmean     128     6324.47 (   0.00%)     7253.65 *  14.69%*

On numa 0-1:
                           tip/core                 patched
Hmean     1        348.68 (   0.00%)      397.74 *  14.07%*
Hmean     2        693.57 (   0.00%)      795.54 *  14.70%*
Hmean     4       1369.26 (   0.00%)     1548.72 *  13.11%*
Hmean     8       2772.99 (   0.00%)     3055.54 *  10.19%*
Hmean     16      4825.83 (   0.00%)     5936.64 *  23.02%*
Hmean     32     10250.32 (   0.00%)    11780.59 *  14.93%*
Hmean     64     16309.51 (   0.00%)    19864.38 *  21.80%*
Hmean     128    13022.32 (   0.00%)    16365.43 *  25.67%*
Hmean     256    11335.79 (   0.00%)    13991.33 *  23.43%*

Hi Peter,

Do you want me to respin a v6 based on your change?

Thanks,
Yicong
diff mbox series

Patch

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 914096c5b1ae..25d2900ae221 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6377,6 +6377,40 @@  static inline int select_idle_smt(struct task_struct *p, struct sched_domain *sd
 
 #endif /* CONFIG_SCHED_SMT */
 
+#ifdef CONFIG_SCHED_CLUSTER
+/*
+ * Scan the cluster domain for idle CPUs and clear cluster cpumask after scanning
+ */
+static inline int scan_cluster(struct task_struct *p, struct cpumask *cpus,
+			       int target, int *nr)
+{
+	struct sched_domain *sd = rcu_dereference(per_cpu(sd_cluster, target));
+	int cpu, idle_cpu;
+
+	/* TODO: Support SMT system with cluster topology */
+	if (!sched_smt_active() && sd) {
+		for_each_cpu_and(cpu, cpus, sched_domain_span(sd)) {
+			if (!--*nr)
+				return -1;
+
+			idle_cpu = __select_idle_cpu(cpu, p);
+			if ((unsigned int)idle_cpu < nr_cpumask_bits)
+				return idle_cpu;
+		}
+
+		cpumask_andnot(cpus, cpus, sched_domain_span(sd));
+	}
+
+	return -1;
+}
+#else
+static inline int scan_cluster(struct task_struct *p, struct cpumask *cpus,
+			       int target, int *nr)
+{
+	return -1;
+}
+#endif
+
 /*
  * Scan the LLC domain for idle CPUs; this is dynamically regulated by
  * comparing the average scan cost (tracked in sd->avg_scan_cost) against the
@@ -6437,6 +6471,10 @@  static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
 		}
 	}
 
+	idle_cpu = scan_cluster(p, cpus, target, &nr);
+	if ((unsigned int)idle_cpu < nr_cpumask_bits)
+		return idle_cpu;
+
 	for_each_cpu_wrap(cpu, cpus, target + 1) {
 		if (has_idle_core) {
 			i = select_idle_core(p, cpu, cpus, &idle_cpu);
@@ -6444,7 +6482,7 @@  static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
 				return i;
 
 		} else {
-			if (!--nr)
+			if (--nr <= 0)
 				return -1;
 			idle_cpu = __select_idle_cpu(cpu, p);
 			if ((unsigned int)idle_cpu < nr_cpumask_bits)
@@ -6543,7 +6581,7 @@  static int select_idle_sibling(struct task_struct *p, int prev, int target)
 	/*
 	 * If the previous CPU is cache affine and idle, don't be stupid:
 	 */
-	if (prev != target && cpus_share_cache(prev, target) &&
+	if (prev != target && cpus_share_lowest_cache(prev, target) &&
 	    (available_idle_cpu(prev) || sched_idle_cpu(prev)) &&
 	    asym_fits_capacity(task_util, prev))
 		return prev;
@@ -6569,7 +6607,7 @@  static int select_idle_sibling(struct task_struct *p, int prev, int target)
 	p->recent_used_cpu = prev;
 	if (recent_used_cpu != prev &&
 	    recent_used_cpu != target &&
-	    cpus_share_cache(recent_used_cpu, target) &&
+	    cpus_share_lowest_cache(recent_used_cpu, target) &&
 	    (available_idle_cpu(recent_used_cpu) || sched_idle_cpu(recent_used_cpu)) &&
 	    cpumask_test_cpu(p->recent_used_cpu, p->cpus_ptr) &&
 	    asym_fits_capacity(task_util, recent_used_cpu)) {