Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1627771 > unrolled thread

[PATCH 0/4] sched/topology: fix overlap group capacity and balance cpu

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] sched/topology: fix overlap group capacity and balance cpu Lauro Ramos Venancio <lvenanci@redhat.com> - 2017-04-20 22:00 +0200
    [PATCH 1/4] sched/topology: optimize build_group_mask() Lauro Ramos Venancio <lvenanci@redhat.com> - 2017-04-20 22:00 +0200
    [PATCH 3/4] sched/topology: move comment about asymmetric node setups Lauro Ramos Venancio <lvenanci@redhat.com> - 2017-04-20 22:00 +0200
      Re: [PATCH 3/4] sched/topology: move comment about asymmetric node  setups Peter Zijlstra <peterz@infradead.org> - 2017-04-21 19:50 +0200
    [PATCH 2/4] sched/topology: all instances of a sched group must use the same sched_group_capacity Lauro Ramos Venancio <lvenanci@redhat.com> - 2017-04-20 22:00 +0200
    [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed Lauro Ramos Venancio <lvenanci@redhat.com> - 2017-04-20 22:00 +0200
      Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu  where the group is installed Peter Zijlstra <peterz@infradead.org> - 2017-04-24 15:10 +0200
        Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu  where the group is installed Peter Zijlstra <peterz@infradead.org> - 2017-04-24 16:30 +0200
          Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu  where the group is installed Peter Zijlstra <peterz@infradead.org> - 2017-04-24 16:30 +0200
            Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu  where the group is installed Lauro Venancio <lvenanci@redhat.com> - 2017-04-24 17:30 +0200
        Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu  where the group is installed Lauro Venancio <lvenanci@redhat.com> - 2017-04-24 17:20 +0200

#1627771 — [PATCH 0/4] sched/topology: fix overlap group capacity and balance cpu

This patchset is on top of Peter Zijlstra's sched/core tree[1]. It is equivalent
to the patch 3 from my previous patchset[2].

This patchset ensures:

 1) different instances of the same sched group share the same
    sched_group_capacity instance.
 2) instances of different groups don't share the same sched_group_capacity
    instance.
 3) the group balance cpu must be one of the cpus where the group is installed.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/log/?h=sched/core
[2] https://lkml.org/lkml/2017/4/13/355

Lauro Ramos Venancio (4):
  sched/topology: optimize build_group_mask()
  sched/topology: all instances of a sched group must use the same
    sched_group_capacity
  sched/topology: move comment about asymmetric node setups
  sched/topology: the group balance cpu must be a cpu where the group is
    installed

 kernel/sched/topology.c | 83 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1627772 — [PATCH 1/4] sched/topology: optimize build_group_mask()

The group mask is always used in intersection with the group cpus. So,
when building the group mask, we don't have to care about cpus that are
not part of the group.

Signed-off-by: Lauro Ramos Venancio <lvenanci@redhat.com>
---
 kernel/sched/topology.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 9d4566c..f8b53b3 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -506,12 +506,12 @@ enum s_alloc {
  */
 static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
 {
-	const struct cpumask *span = sched_domain_span(sd);
+	const struct cpumask *sg_span = sched_group_cpus(sg);
 	struct sd_data *sdd = sd->private;
 	struct sched_domain *sibling;
 	int i;
 
-	for_each_cpu(i, span) {
+	for_each_cpu(i, sg_span) {
 		sibling = *per_cpu_ptr(sdd->sd, i);
 		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
 			continue;
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1627774 — [PATCH 3/4] sched/topology: move comment about asymmetric node setups

Signed-off-by: Lauro Ramos Venancio <lvenanci@redhat.com>
---
 kernel/sched/topology.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index 55bbaf7..e77c93a 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -495,14 +495,6 @@ enum s_alloc {
 /*
  * Build an iteration mask that can exclude certain CPUs from the upwards
  * domain traversal.
- *
- * Asymmetric node setups can result in situations where the domain tree is of
- * unequal depth, make sure to skip domains that already cover the entire
- * range.
- *
- * In that case build_sched_domains() will have terminated the iteration early
- * and our sibling sd spans will be empty. Domains should always include the
- * CPU they're built on, so check that.
  */
 static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
 {
@@ -612,7 +604,16 @@ static void init_overlap_sched_group(struct sched_domain *sd,
 
 		sibling = *per_cpu_ptr(sdd->sd, i);
 
-		/* See the comment near build_group_mask(). */
+		/*
+		 * Asymmetric node setups can result in situations where the
+		 * domain tree is of unequal depth, make sure to skip domains
+		 * that already cover the entire range.
+		 *
+		 * In that case build_sched_domains() will have terminated the
+		 * iteration early and our sibling sd spans will be empty.
+		 * Domains should always include the CPU they're built on, so
+		 * check that.
+		 */
 		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
 			continue;
 
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1628402 — Re: [PATCH 3/4] sched/topology: move comment about asymmetric node setups

On Thu, Apr 20, 2017 at 04:51:42PM -0300, Lauro Ramos Venancio wrote:
> @@ -612,7 +604,16 @@ static void init_overlap_sched_group(struct sched_domain *sd,
>  
>  		sibling = *per_cpu_ptr(sdd->sd, i);
>  
> -		/* See the comment near build_group_mask(). */
> +		/*
> +		 * Asymmetric node setups can result in situations where the
> +		 * domain tree is of unequal depth, make sure to skip domains
> +		 * that already cover the entire range.
> +		 *
> +		 * In that case build_sched_domains() will have terminated the
> +		 * iteration early and our sibling sd spans will be empty.
> +		 * Domains should always include the CPU they're built on, so
> +		 * check that.
> +		 */
>  		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
>  			continue;
>  

FWIW, the topology that spawned all that is:

  10,20,20,30
  20,10,20,20
  20,20,10,20
  30,20,20,10

[toc] | [prev] | [next] | [standalone]

#1627775 — [PATCH 2/4] sched/topology: all instances of a sched group must use the same sched_group_capacity

Use the group balance cpu to select the same sched_group_capacity
instance for all instances of a sched group.

As the group mask is stored in the struct sched_group_capacity and the
function group_balance_cpu() cannot be used when the group mask is not
available, this patch creates a function to find the group balance cpu
when the mask is not available.

Signed-off-by: Lauro Ramos Venancio <lvenanci@redhat.com>
---
 kernel/sched/topology.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index f8b53b3..55bbaf7 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -529,6 +529,27 @@ int group_balance_cpu(struct sched_group *sg)
 	return cpumask_first_and(sched_group_cpus(sg), sched_group_mask(sg));
 }
 
+/*
+ * Find the group balance cpu when the group mask is not available yet.
+ */
+static int find_group_balance_cpu(struct sched_domain *sd,
+				  struct sched_group *sg)
+{
+	const struct cpumask *sg_span = sched_group_cpus(sg);
+	struct sd_data *sdd = sd->private;
+	struct sched_domain *sibling;
+	int i;
+
+	for_each_cpu(i, sg_span) {
+		sibling = *per_cpu_ptr(sdd->sd, i);
+		if (cpumask_test_cpu(i, sched_domain_span(sibling)))
+			return i;
+	}
+
+	WARN(1, "group balance cpu not found.");
+	return 0;
+}
+
 static struct sched_group *
 build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
 {
@@ -551,10 +572,11 @@ int group_balance_cpu(struct sched_group *sg)
 }
 
 static void init_overlap_sched_group(struct sched_domain *sd,
-				     struct sched_group *sg, int cpu)
+				     struct sched_group *sg)
 {
 	struct sd_data *sdd = sd->private;
 	struct cpumask *sg_span;
+	int cpu = find_group_balance_cpu(sd, sg);
 
 	sg->sgc = *per_cpu_ptr(sdd->sgc, cpu);
 	if (atomic_inc_return(&sg->sgc->ref) == 1)
@@ -601,7 +623,7 @@ static void init_overlap_sched_group(struct sched_domain *sd,
 		sg_span = sched_group_cpus(sg);
 		cpumask_or(covered, covered, sg_span);
 
-		init_overlap_sched_group(sd, sg, i);
+		init_overlap_sched_group(sd, sg);
 
 		if (!first)
 			first = sg;
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1627776 — [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed

An overlap sched group may not be installed in all cpus that compose the
group. Currently, the group balance cpu may be a cpu where the group is
not installed, causing two problems:

1) Two groups may have the same group balance cpu and, as consequence,
share the sched_group_capacity.
2) should_we_balance() in fair.c may never return true.

This patch changes the group mask meaning to mark all the cpus where a
group is installed.

Signed-off-by: Lauro Ramos Venancio <lvenanci@redhat.com>
---
 kernel/sched/topology.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index e77c93a..694e799 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -493,8 +493,10 @@ enum s_alloc {
 };
 
 /*
- * Build an iteration mask that can exclude certain CPUs from the upwards
- * domain traversal.
+ * An overlap sched group may not be installed in all CPUs that compose the
+ * group. So build the mask, marking all the CPUs where the group is installed.
+ *
+ * This function can only be used when all the groups are already built.
  */
 static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
 {
@@ -505,7 +507,11 @@ static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
 
 	for_each_cpu(i, sg_span) {
 		sibling = *per_cpu_ptr(sdd->sd, i);
-		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
+
+		if (!sibling->groups)
+			continue;
+
+		if (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
 			continue;
 
 		cpumask_set_cpu(i, sched_group_mask(sg));
@@ -523,6 +529,7 @@ int group_balance_cpu(struct sched_group *sg)
 
 /*
  * Find the group balance cpu when the group mask is not available yet.
+ * This function can only be used when all the groups are already built.
  */
 static int find_group_balance_cpu(struct sched_domain *sd,
 				  struct sched_group *sg)
@@ -534,7 +541,11 @@ static int find_group_balance_cpu(struct sched_domain *sd,
 
 	for_each_cpu(i, sg_span) {
 		sibling = *per_cpu_ptr(sdd->sd, i);
-		if (cpumask_test_cpu(i, sched_domain_span(sibling)))
+
+		if (!sibling->groups)
+			continue;
+
+		if (cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
 			return i;
 	}
 
@@ -584,6 +595,17 @@ static void init_overlap_sched_group(struct sched_domain *sd,
 	sg->sgc->min_capacity = SCHED_CAPACITY_SCALE;
 }
 
+static void init_overlap_sched_groups(struct sched_domain *sd)
+{
+	struct sched_group *sg = sd->groups;
+
+	do {
+		init_overlap_sched_group(sd, sg);
+
+		sg = sg->next;
+	} while (sg != sd->groups);
+}
+
 static int
 build_overlap_sched_groups(struct sched_domain *sd, int cpu)
 {
@@ -624,8 +646,6 @@ static void init_overlap_sched_group(struct sched_domain *sd,
 		sg_span = sched_group_cpus(sg);
 		cpumask_or(covered, covered, sg_span);
 
-		init_overlap_sched_group(sd, sg);
-
 		if (!first)
 			first = sg;
 		if (last)
@@ -1482,6 +1502,14 @@ struct sched_domain *build_sched_domain(struct sched_domain_topology_level *tl,
 		}
 	}
 
+	/* Init overlap groups */
+	for_each_cpu(i, cpu_map) {
+		for (sd = *per_cpu_ptr(d.sd, i); sd; sd = sd->parent) {
+			if (sd->flags & SD_OVERLAP)
+				init_overlap_sched_groups(sd);
+		}
+	}
+
 	/* Calculate CPU capacity for physical packages and nodes */
 	for (i = nr_cpumask_bits-1; i >= 0; i--) {
 		if (!cpumask_test_cpu(i, cpu_map))
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1629529 — Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed

On Thu, Apr 20, 2017 at 04:51:43PM -0300, Lauro Ramos Venancio wrote:

> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index e77c93a..694e799 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c

> @@ -505,7 +507,11 @@ static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
>  
>  	for_each_cpu(i, sg_span) {
>  		sibling = *per_cpu_ptr(sdd->sd, i);
> -		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
> +
> +		if (!sibling->groups)
> +			continue;

How can this happen?

> +
> +		if (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
>  			continue;
>  
>  		cpumask_set_cpu(i, sched_group_mask(sg));


> @@ -1482,6 +1502,14 @@ struct sched_domain *build_sched_domain(struct sched_domain_topology_level *tl,
>  		}
>  	}
>  
> +	/* Init overlap groups */
> +	for_each_cpu(i, cpu_map) {
> +		for (sd = *per_cpu_ptr(d.sd, i); sd; sd = sd->parent) {
> +			if (sd->flags & SD_OVERLAP)
> +				init_overlap_sched_groups(sd);
> +		}
> +	}

Why does this have to be a whole new loop? This is because in
build_group_mask() we could encounter @sibling that were not constructed
yet?

So this is the primary fix?

> +
>  	/* Calculate CPU capacity for physical packages and nodes */
>  	for (i = nr_cpumask_bits-1; i >= 0; i--) {
>  		if (!cpumask_test_cpu(i, cpu_map))


Also, would it not make sense to re-order patch 2 to come after this,
such that we _do_ have the group_mask available and don't have to jump
through hoops in order to link up the sgc? Afaict we don't actually use
the sgc until the above (reverse) loop computing the CPU capacities.

[toc] | [prev] | [next] | [standalone]

#1629618 — Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed

On Mon, Apr 24, 2017 at 03:03:26PM +0200, Peter Zijlstra wrote:

> Also, would it not make sense to re-order patch 2 to come after this,
> such that we _do_ have the group_mask available and don't have to jump
> through hoops in order to link up the sgc? Afaict we don't actually use
> the sgc until the above (reverse) loop computing the CPU capacities.

That is, if I force 4 on without 2, then doesn't something like the
below also do the right thing? (without duplicating part of the magic
already contained in build_group_mask)

---
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -498,13 +498,16 @@ enum s_alloc {
  *
  * This function can only be used when all the groups are already built.
  */
-static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
+static void
+build_group_mask(struct sched_domain *sd, struct sched_group *sg, struct cpumask *mask)
 {
 	const struct cpumask *sg_span = sched_group_cpus(sg);
 	struct sd_data *sdd = sd->private;
 	struct sched_domain *sibling;
 	int i;
 
+	cpumask_clear(mask);
+
 	for_each_cpu(i, sg_span) {
 		sibling = *per_cpu_ptr(sdd->sd, i);
 
@@ -514,7 +517,7 @@ static void build_group_mask(struct sche
 		if (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
 			continue;
 
-		cpumask_set_cpu(i, sched_group_mask(sg));
+		cpumask_set_cpu(i, mask);
 	}
 }
 
@@ -549,14 +552,19 @@ build_group_from_child_sched_domain(stru
 }
 
 static void init_overlap_sched_group(struct sched_domain *sd,
-				     struct sched_group *sg, int cpu)
+				     struct sched_group *sg)
 {
+	struct cpumask *mask = sched_domains_tmpmask;
 	struct sd_data *sdd = sd->private;
 	struct cpumask *sg_span;
+	int cpu;
+
+	build_group_mask(sd, sg, mask);
+	cpu = cpumask_first_and(sched_group_mask(sg), mask); /* balance cpu */
 
 	sg->sgc = *per_cpu_ptr(sdd->sgc, cpu);
 	if (atomic_inc_return(&sg->sgc->ref) == 1)
-		build_group_mask(sd, sg);
+		cpumask_copy(sched_group_mask(sg), mask);
 
 	/*
 	 * Initialize sgc->capacity such that even if we mess up the

[toc] | [prev] | [next] | [standalone]

#1629631 — Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed

On Mon, Apr 24, 2017 at 04:19:44PM +0200, Peter Zijlstra wrote:
> On Mon, Apr 24, 2017 at 03:03:26PM +0200, Peter Zijlstra wrote:
> 
> > Also, would it not make sense to re-order patch 2 to come after this,
> > such that we _do_ have the group_mask available and don't have to jump
> > through hoops in order to link up the sgc? Afaict we don't actually use
> > the sgc until the above (reverse) loop computing the CPU capacities.
> 
> That is, if I force 4 on without 2, then doesn't something like the
> below also do the right thing? (without duplicating part of the magic
> already contained in build_group_mask)
> 
> ---
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -498,13 +498,16 @@ enum s_alloc {
>   *
>   * This function can only be used when all the groups are already built.
>   */
> -static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
> +static void
> +build_group_mask(struct sched_domain *sd, struct sched_group *sg, struct cpumask *mask)
>  {
>  	const struct cpumask *sg_span = sched_group_cpus(sg);
>  	struct sd_data *sdd = sd->private;
>  	struct sched_domain *sibling;
>  	int i;
>  
> +	cpumask_clear(mask);
> +
>  	for_each_cpu(i, sg_span) {
>  		sibling = *per_cpu_ptr(sdd->sd, i);
>  
> @@ -514,7 +517,7 @@ static void build_group_mask(struct sche
>  		if (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
>  			continue;
>  
> -		cpumask_set_cpu(i, sched_group_mask(sg));
> +		cpumask_set_cpu(i, mask);
>  	}
>  }
>  
> @@ -549,14 +552,19 @@ build_group_from_child_sched_domain(stru
>  }
>  
>  static void init_overlap_sched_group(struct sched_domain *sd,
> -				     struct sched_group *sg, int cpu)
> +				     struct sched_group *sg)
>  {
> +	struct cpumask *mask = sched_domains_tmpmask;
>  	struct sd_data *sdd = sd->private;
>  	struct cpumask *sg_span;
> +	int cpu;
> +
> +	build_group_mask(sd, sg, mask);
> +	cpu = cpumask_first_and(sched_group_mask(sg), mask); /* balance cpu */

s/group_mask/group_span/

>  
>  	sg->sgc = *per_cpu_ptr(sdd->sgc, cpu);
>  	if (atomic_inc_return(&sg->sgc->ref) == 1)
> -		build_group_mask(sd, sg);
> +		cpumask_copy(sched_group_mask(sg), mask);
>  
>  	/*
>  	 * Initialize sgc->capacity such that even if we mess up the

[toc] | [prev] | [next] | [standalone]

#1629671 — Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed

On 04/24/2017 11:27 AM, Peter Zijlstra wrote:
> On Mon, Apr 24, 2017 at 04:19:44PM +0200, Peter Zijlstra wrote:
>> On Mon, Apr 24, 2017 at 03:03:26PM +0200, Peter Zijlstra wrote:
>>
>>> Also, would it not make sense to re-order patch 2 to come after this,
>>> such that we _do_ have the group_mask available and don't have to jump
>>> through hoops in order to link up the sgc? Afaict we don't actually use
>>> the sgc until the above (reverse) loop computing the CPU capacities.
>> That is, if I force 4 on without 2, then doesn't something like the
>> below also do the right thing? (without duplicating part of the magic
>> already contained in build_group_mask)
Yes, it has the same result. I duplicated the build_group_mask magic to
avoid building the complete mask for all instances of a group.
Currently, the mask is built just once per group.
>>
>> ---
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -498,13 +498,16 @@ enum s_alloc {
>>   *
>>   * This function can only be used when all the groups are already built.
>>   */
>> -static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
>> +static void
>> +build_group_mask(struct sched_domain *sd, struct sched_group *sg, struct cpumask *mask)
>>  {
>>  	const struct cpumask *sg_span = sched_group_cpus(sg);
>>  	struct sd_data *sdd = sd->private;
>>  	struct sched_domain *sibling;
>>  	int i;
>>  
>> +	cpumask_clear(mask);
>> +
>>  	for_each_cpu(i, sg_span) {
>>  		sibling = *per_cpu_ptr(sdd->sd, i);
>>  
>> @@ -514,7 +517,7 @@ static void build_group_mask(struct sche
>>  		if (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
>>  			continue;
>>  
>> -		cpumask_set_cpu(i, sched_group_mask(sg));
>> +		cpumask_set_cpu(i, mask);
>>  	}
>>  }
>>  
>> @@ -549,14 +552,19 @@ build_group_from_child_sched_domain(stru
>>  }
>>  
>>  static void init_overlap_sched_group(struct sched_domain *sd,
>> -				     struct sched_group *sg, int cpu)
>> +				     struct sched_group *sg)
>>  {
>> +	struct cpumask *mask = sched_domains_tmpmask;
>>  	struct sd_data *sdd = sd->private;
>>  	struct cpumask *sg_span;
>> +	int cpu;
>> +
>> +	build_group_mask(sd, sg, mask);
>> +	cpu = cpumask_first_and(sched_group_mask(sg), mask); /* balance cpu */
> s/group_mask/group_span/
>
>>  
>>  	sg->sgc = *per_cpu_ptr(sdd->sgc, cpu);
>>  	if (atomic_inc_return(&sg->sgc->ref) == 1)
>> -		build_group_mask(sd, sg);
>> +		cpumask_copy(sched_group_mask(sg), mask);
>>  
>>  	/*
>>  	 * Initialize sgc->capacity such that even if we mess up the

[toc] | [prev] | [next] | [standalone]

#1629661 — Re: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed

On 04/24/2017 10:03 AM, Peter Zijlstra wrote:
> On Thu, Apr 20, 2017 at 04:51:43PM -0300, Lauro Ramos Venancio wrote:
>
>> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
>> index e77c93a..694e799 100644
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -505,7 +507,11 @@ static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
>>  
>>  	for_each_cpu(i, sg_span) {
>>  		sibling = *per_cpu_ptr(sdd->sd, i);
>> -		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
>> +
>> +		if (!sibling->groups)
>> +			continue;
> How can this happen?
This happens on machines with asymmetric topologies. For more details see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c1174876874dcf8986806e4dad3d7d07af20b439
>
>> +
>> +		if (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
>>  			continue;
>>  
>>  		cpumask_set_cpu(i, sched_group_mask(sg));
>
>> @@ -1482,6 +1502,14 @@ struct sched_domain *build_sched_domain(struct sched_domain_topology_level *tl,
>>  		}
>>  	}
>>  
>> +	/* Init overlap groups */
>> +	for_each_cpu(i, cpu_map) {
>> +		for (sd = *per_cpu_ptr(d.sd, i); sd; sd = sd->parent) {
>> +			if (sd->flags & SD_OVERLAP)
>> +				init_overlap_sched_groups(sd);
>> +		}
>> +	}
> Why does this have to be a whole new loop? This is because in
> build_group_mask() we could encounter @sibling that were not constructed
> yet?
That is right. We can only build the group mask when all siblings are
constructed.

>
> So this is the primary fix?
Yes.
>
>> +
>>  	/* Calculate CPU capacity for physical packages and nodes */
>>  	for (i = nr_cpumask_bits-1; i >= 0; i--) {
>>  		if (!cpumask_test_cpu(i, cpu_map))
>
> Also, would it not make sense to re-order patch 2 to come after this,
> such that we _do_ have the group_mask available and don't have to jump
> through hoops in order to link up the sgc? Afaict we don't actually use
> the sgc until the above (reverse) loop computing the CPU capacities.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web