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

Groups > linux.kernel > #1630445 > unrolled thread

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

Started byPeter Zijlstra <peterz@infradead.org>
First post2017-04-25 14:20 +0200
Last post2017-04-25 17:30 +0200
Articles 9 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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-25 14:20 +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-25 16:40 +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-25 17: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-25 17:50 +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-25 18:00 +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-25 18: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-25 18: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-25 17: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-25 17:30 +0200

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

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 14:20 +0200
SubjectRe: [PATCH 4/4] sched/topology: the group balance cpu must be a cpu where the group is installed
Message-ID<tA7BM-4B0-19@gated-at.bofh.it>
On Mon, Apr 24, 2017 at 12:11:59PM -0300, Lauro Venancio wrote:
> 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 (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
> >>  			continue;

Hmm _this_ is what requires us to move the thing to a whole separate
iteration. Because when we build the groups, the domains are already
constructed, so that was right.

So the moving crud around wasn't the primary fix, this is.

With the fact that sched_group_cpus(sd->groups) ==
sched_domain_span(sibling->child) (if child exists) established in the
previous patches, could we not simplify this like the below?

---
Subject: sched/topology: Fix overlapping sched_group_mask
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Apr 25 14:00:49 CEST 2017

The point of sched_group_mask is to select those CPUs from
sched_group_cpus that can actually arrive at this balance domain.

The current code gets it wrong, as can be readily demonstrated with a
topology like:

  node   0   1   2   3
    0:  10  20  30  20
    1:  20  10  20  30
    2:  30  20  10  20
    3:  20  30  20  10

Where (for example) domain 1 on CPU1 ends up with a mask that includes
CPU0:

  [] CPU1 attaching sched-domain:
  []  domain 0: span 0-2 level NUMA
  []   groups: 1 (mask: 1), 2, 0
  []   domain 1: span 0-3 level NUMA
  []    groups: 0-2 (mask: 0-2) (cpu_capacity: 3072), 0,2-3 (cpu_capacity: 3072)

This causes sched_balance_cpu() to compute the wrong CPU and
consequently should_we_balance() will terminate early resulting in
missed load-balance opportunities.

The fixed topology looks like:

  [] CPU1 attaching sched-domain:
  []  domain 0: span 0-2 level NUMA
  []   groups: 1 (mask: 1), 2, 0
  []   domain 1: span 0-3 level NUMA
  []    groups: 0-2 (mask: 1) (cpu_capacity: 3072), 0,2-3 (cpu_capacity: 3072)

Debugged-by: Lauro Ramos Venancio <lvenanci@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/sched/topology.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -495,6 +495,9 @@ enum s_alloc {
 /*
  * Build an iteration mask that can exclude certain CPUs from the upwards
  * domain traversal.
+ *
+ * Only CPUs that can arrive at this group should be considered to continue
+ * balancing.
  */
 static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
 {
@@ -505,7 +508,13 @@ static void build_group_mask(struct sche
 
 	for_each_cpu(i, sg_span) {
 		sibling = *per_cpu_ptr(sdd->sd, i);
-		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
+
+		/* overlap should have children; except for FORCE_SD_OVERLAP */
+		if (WARN_ON_ONCE(!sibling->child))
+			continue;
+
+		/* If we would not end up here, we can't continue from here */
+		if (!cpumask_equal(sg_span, sched_domain_span(sibling->child)))
 			continue;
 
 		cpumask_set_cpu(i, sched_group_mask(sg));

[toc] | [next] | [standalone]

#1630559

FromLauro Venancio <lvenanci@redhat.com>
Date2017-04-25 16:40 +0200
Message-ID<tA9Nf-5S4-1@gated-at.bofh.it>
In reply to#1630445
On 04/25/2017 09:17 AM, Peter Zijlstra wrote:
> On Mon, Apr 24, 2017 at 12:11:59PM -0300, Lauro Venancio wrote:
>> 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 (!cpumask_equal(sg_span, sched_group_cpus(sibling->groups)))
>>>>  			continue;
> Hmm _this_ is what requires us to move the thing to a whole separate
> iteration. Because when we build the groups, the domains are already
> constructed, so that was right.
>
> So the moving crud around wasn't the primary fix, this is.
>
> With the fact that sched_group_cpus(sd->groups) ==
> sched_domain_span(sibling->child) (if child exists) established in the
> previous patches, could we not simplify this like the below?
We can. We just need to better handle the case when there is no child or
we will have empty masks.
We have to replicate the build_group_from_child_sched_domain() behavior:

if (sd->child)
    cpumask_copy(sg_span, sched_domain_span(sd->child));
else
    cpumask_copy(sg_span, sched_domain_span(sd));


So we need something like:


if (sibling->child)
    gsd = sibling->child;
else
    gsd = sibling;

if (!cpumask_equal(sg_span, sched_domain_span(gsd)))

	continue;


>
> ---
> Subject: sched/topology: Fix overlapping sched_group_mask
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Tue Apr 25 14:00:49 CEST 2017
>
> The point of sched_group_mask is to select those CPUs from
> sched_group_cpus that can actually arrive at this balance domain.
>
> The current code gets it wrong, as can be readily demonstrated with a
> topology like:
>
>   node   0   1   2   3
>     0:  10  20  30  20
>     1:  20  10  20  30
>     2:  30  20  10  20
>     3:  20  30  20  10
>
> Where (for example) domain 1 on CPU1 ends up with a mask that includes
> CPU0:
>
>   [] CPU1 attaching sched-domain:
>   []  domain 0: span 0-2 level NUMA
>   []   groups: 1 (mask: 1), 2, 0
>   []   domain 1: span 0-3 level NUMA
>   []    groups: 0-2 (mask: 0-2) (cpu_capacity: 3072), 0,2-3 (cpu_capacity: 3072)
>
> This causes sched_balance_cpu() to compute the wrong CPU and
> consequently should_we_balance() will terminate early resulting in
> missed load-balance opportunities.
>
> The fixed topology looks like:
>
>   [] CPU1 attaching sched-domain:
>   []  domain 0: span 0-2 level NUMA
>   []   groups: 1 (mask: 1), 2, 0
>   []   domain 1: span 0-3 level NUMA
>   []    groups: 0-2 (mask: 1) (cpu_capacity: 3072), 0,2-3 (cpu_capacity: 3072)
>
> Debugged-by: Lauro Ramos Venancio <lvenanci@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/sched/topology.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -495,6 +495,9 @@ enum s_alloc {
>  /*
>   * Build an iteration mask that can exclude certain CPUs from the upwards
>   * domain traversal.
> + *
> + * Only CPUs that can arrive at this group should be considered to continue
> + * balancing.
>   */
>  static void build_group_mask(struct sched_domain *sd, struct sched_group *sg)
>  {
> @@ -505,7 +508,13 @@ static void build_group_mask(struct sche
>  
>  	for_each_cpu(i, sg_span) {
>  		sibling = *per_cpu_ptr(sdd->sd, i);
> -		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
> +
> +		/* overlap should have children; except for FORCE_SD_OVERLAP */
> +		if (WARN_ON_ONCE(!sibling->child))
> +			continue;
> +
> +		/* If we would not end up here, we can't continue from here */
> +		if (!cpumask_equal(sg_span, sched_domain_span(sibling->child)))
>  			continue;
>  
>  		cpumask_set_cpu(i, sched_group_mask(sg));
>

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

#1630645

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 17:30 +0200
Message-ID<tAazE-6sV-13@gated-at.bofh.it>
In reply to#1630559
On Tue, Apr 25, 2017 at 05:22:36PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 25, 2017 at 05:12:00PM +0200, Peter Zijlstra wrote:
> > But I'll first try and figure out why I'm not having empty masks.
> 
> Ah, so this is before all the degenerate stuff, so there's a bunch of
> redundant domains below that make it work -- and there always will be,
> unless FORCE_SD_OVERLAP.
> 
> Now I wonder what triggered it.. let me put it back.

Ah! the asymmetric setup, where @sibling is entirely uninitialized for
the top domain.

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

#1630702

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 17:50 +0200
Message-ID<tAaT0-6BR-15@gated-at.bofh.it>
In reply to#1630645
On Tue, Apr 25, 2017 at 05:27:03PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 25, 2017 at 05:22:36PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 25, 2017 at 05:12:00PM +0200, Peter Zijlstra wrote:
> > > But I'll first try and figure out why I'm not having empty masks.
> > 
> > Ah, so this is before all the degenerate stuff, so there's a bunch of
> > redundant domains below that make it work -- and there always will be,
> > unless FORCE_SD_OVERLAP.
> > 
> > Now I wonder what triggered it.. let me put it back.
> 
> Ah! the asymmetric setup, where @sibling is entirely uninitialized for
> the top domain.
> 
And it still works correctly too:


[    0.078756] XXX 1 NUMA 
[    0.079005] XXX 2 NUMA 
[    0.080003] XXY 0-2:0
[    0.081007] XXX 1 NUMA 
[    0.082005] XXX 2 NUMA 
[    0.083003] XXY 1-3:3
[    0.084032] XXX 1 NUMA 
[    0.085003] XXX 2 NUMA 
[    0.086003] XXY 1-3:3
[    0.087015] XXX 1 NUMA 
[    0.088003] XXX 2 NUMA 
[    0.089002] XXY 0-2:0


[    0.090007] CPU0 attaching sched-domain:
[    0.091002]  domain 0: span 0-2 level NUMA
[    0.092002]   groups: 0 (mask: 0), 1, 2
[    0.093002]   domain 1: span 0-3 level NUMA
[    0.094002]    groups: 0-2 (mask: 0) (cpu_capacity: 3072), 1-3 (cpu_capacity: 3072)
[    0.095005] CPU1 attaching sched-domain:
[    0.096003]  domain 0: span 0-3 level NUMA
[    0.097002]   groups: 1 (mask: 1), 2, 3, 0
[    0.098004] CPU2 attaching sched-domain:
[    0.099002]  domain 0: span 0-3 level NUMA
[    0.100002]   groups: 2 (mask: 2), 3, 0, 1
[    0.101004] CPU3 attaching sched-domain:
[    0.102002]  domain 0: span 1-3 level NUMA
[    0.103002]   groups: 3 (mask: 3), 1, 2
[    0.104002]   domain 1: span 0-3 level NUMA
[    0.105002]    groups: 1-3 (mask: 3) (cpu_capacity: 3072), 0-2 (cpu_capacity: 3072)


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, funny = 0;

        cpumask_clear(mask);

        for_each_cpu(i, sg_span) {
                sibling = *per_cpu_ptr(sdd->sd, i);

                if (!sibling->child) {
                        funny = 1;
                        printk("XXX %d %s %*pbl\n", i, sd->name, cpumask_pr_args(sched_domain_span(sibling)));
                        continue;
                }

                /* If we would not end up here, we can't continue from here */
                if (!cpumask_equal(sg_span, sched_domain_span(sibling->child)))
                        continue;

                cpumask_set_cpu(i, mask);
        }

        if (funny) {
                printk("XXY %*pbl:%*pbl\n",
                                cpumask_pr_args(sg_span),
                                cpumask_pr_args(mask));
        }
}


So that will still get the right balance cpu and thus sgc.

Another thing I've been thinking about; I think we can do away with the
kzalloc() in build_group_from_child_sched_domain() and use the sdd->sg
storage.

I just didn't want to move too much code around again, and ideally put
more assertions in place to catch bad stuff; I just haven't had a good
time thinking of good assertions :/

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

#1630719

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 18:00 +0200
Message-ID<tAb2F-6Ff-19@gated-at.bofh.it>
In reply to#1630702
On Tue, Apr 25, 2017 at 05:39:37PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 25, 2017 at 05:27:03PM +0200, Peter Zijlstra wrote:

> > Ah! the asymmetric setup, where @sibling is entirely uninitialized for
> > the top domain.

Like so then...

--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -509,6 +509,11 @@ static void build_group_mask(struct sche
 	for_each_cpu(i, sg_span) {
 		sibling = *per_cpu_ptr(sdd->sd, i);
 
+		/*
+		 * Can happen in the asymmetric case, where these siblings are
+		 * unused. The mask will not be empty because those CPUs that
+		 * do have the top domain _should_ span the domain.
+		 */
 		if (!sibling->child)
 			continue;
 
@@ -518,6 +523,9 @@ static void build_group_mask(struct sche
 
 		cpumask_set_cpu(i, sched_group_mask(sg));
 	}
+
+	/* We must not have empty masks here */
+	WARN_ON_ONCE(cpumask_empty(sched_group_mask(sg)));
 }
 
 /*

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

#1630728

FromLauro Venancio <lvenanci@redhat.com>
Date2017-04-25 18:00 +0200
Message-ID<tAb2G-6Ff-47@gated-at.bofh.it>
In reply to#1630702
On 04/25/2017 12:39 PM, Peter Zijlstra wrote:
> On Tue, Apr 25, 2017 at 05:27:03PM +0200, Peter Zijlstra wrote:
>> On Tue, Apr 25, 2017 at 05:22:36PM +0200, Peter Zijlstra wrote:
>>> On Tue, Apr 25, 2017 at 05:12:00PM +0200, Peter Zijlstra wrote:
>>>> But I'll first try and figure out why I'm not having empty masks.
>>> Ah, so this is before all the degenerate stuff, so there's a bunch of
>>> redundant domains below that make it work -- and there always will be,
>>> unless FORCE_SD_OVERLAP.
>>>
>>> Now I wonder what triggered it.. let me put it back.
>> Ah! the asymmetric setup, where @sibling is entirely uninitialized for
>> the top domain.
>>
> And it still works correctly too:
>
>
> [    0.078756] XXX 1 NUMA 
> [    0.079005] XXX 2 NUMA 
> [    0.080003] XXY 0-2:0
> [    0.081007] XXX 1 NUMA 
> [    0.082005] XXX 2 NUMA 
> [    0.083003] XXY 1-3:3
> [    0.084032] XXX 1 NUMA 
> [    0.085003] XXX 2 NUMA 
> [    0.086003] XXY 1-3:3
> [    0.087015] XXX 1 NUMA 
> [    0.088003] XXX 2 NUMA 
> [    0.089002] XXY 0-2:0
>
>
> [    0.090007] CPU0 attaching sched-domain:
> [    0.091002]  domain 0: span 0-2 level NUMA
> [    0.092002]   groups: 0 (mask: 0), 1, 2
> [    0.093002]   domain 1: span 0-3 level NUMA
> [    0.094002]    groups: 0-2 (mask: 0) (cpu_capacity: 3072), 1-3 (cpu_capacity: 3072)
> [    0.095005] CPU1 attaching sched-domain:
> [    0.096003]  domain 0: span 0-3 level NUMA
> [    0.097002]   groups: 1 (mask: 1), 2, 3, 0
> [    0.098004] CPU2 attaching sched-domain:
> [    0.099002]  domain 0: span 0-3 level NUMA
> [    0.100002]   groups: 2 (mask: 2), 3, 0, 1
> [    0.101004] CPU3 attaching sched-domain:
> [    0.102002]  domain 0: span 1-3 level NUMA
> [    0.103002]   groups: 3 (mask: 3), 1, 2
> [    0.104002]   domain 1: span 0-3 level NUMA
> [    0.105002]    groups: 1-3 (mask: 3) (cpu_capacity: 3072), 0-2 (cpu_capacity: 3072)
>
>
> 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, funny = 0;
>
>         cpumask_clear(mask);
>
>         for_each_cpu(i, sg_span) {
>                 sibling = *per_cpu_ptr(sdd->sd, i);
>
>                 if (!sibling->child) {
>                         funny = 1;
>                         printk("XXX %d %s %*pbl\n", i, sd->name, cpumask_pr_args(sched_domain_span(sibling)));
>                         continue;
>                 }
>
>                 /* If we would not end up here, we can't continue from here */
>                 if (!cpumask_equal(sg_span, sched_domain_span(sibling->child)))
>                         continue;
>
>                 cpumask_set_cpu(i, mask);
>         }
>
>         if (funny) {
>                 printk("XXY %*pbl:%*pbl\n",
>                                 cpumask_pr_args(sg_span),
>                                 cpumask_pr_args(mask));
>         }
> }
>
>
> So that will still get the right balance cpu and thus sgc.
>
> Another thing I've been thinking about; I think we can do away with the
> kzalloc() in build_group_from_child_sched_domain() and use the sdd->sg
> storage.
I considered this too. I decided to do not change this because I was not
sure if the kzalloc() was there for performance reasons. Currently, all
groups are allocated in the NUMA node they are used.
If we use sdd->sg storage, we may have groups allocated in one NUMA node
being used in another node.
>
> I just didn't want to move too much code around again, and ideally put
> more assertions in place to catch bad stuff; I just haven't had a good
> time thinking of good assertions :/

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

#1630756

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 18:30 +0200
Message-ID<tAbvI-74c-35@gated-at.bofh.it>
In reply to#1630728
On Tue, Apr 25, 2017 at 12:56:23PM -0300, Lauro Venancio wrote:

> > Another thing I've been thinking about; I think we can do away with the
> > kzalloc() in build_group_from_child_sched_domain() and use the sdd->sg
> > storage.
> I considered this too. I decided to do not change this because I was not
> sure if the kzalloc() was there for performance reasons. Currently, all
> groups are allocated in the NUMA node they are used.
> If we use sdd->sg storage, we may have groups allocated in one NUMA node
> being used in another node.

Right.. I cannot remember :/

/me once again kicks himself for not writing more comments

It does save a few lines.. and I suspect that if we do this, we could
actually completely get rid of sched_group_capacity, since its now
always the same as the group (again), which should removes more lines
still.

But I'll shelf this patch for now.. we've got enough changes as is.

I still need to write a changelog for the new #2, which has become ugly
again, because its needs a second sched_domains_tmpmask.

(compile tested only)

---
 kernel/sched/topology.c |   76 ++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 47 deletions(-)

--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -501,10 +501,8 @@ enum s_alloc {
  * balancing.
  */
 static void
-build_group_mask(struct sched_domain *sd, struct sched_group *sg, struct cpumask *mask)
+build_group_mask(struct sd_data *sdd, struct cpumask *sg_span, struct cpumask *mask)
 {
-	const struct cpumask *sg_span = sched_group_cpus(sg);
-	struct sd_data *sdd = sd->private;
 	struct sched_domain *sibling;
 	int i;
 
@@ -542,49 +540,34 @@ int group_balance_cpu(struct sched_group
 }
 
 static struct sched_group *
-build_group_from_child_sched_domain(struct sched_domain *sd, int cpu)
+get_overlap_group(struct sd_data *sdd, int cpu)
 {
-	struct sched_group *sg;
-	struct cpumask *sg_span;
+	struct sched_domain *sd = *per_cpu_ptr(sdd->sd, cpu);
+	struct sched_domain *child = sd->child;
+	struct sched_group *group;
+	struct cpumask *mask = sched_domains_tmpmask2;
 
-	sg = kzalloc_node(sizeof(struct sched_group) + cpumask_size(),
-			GFP_KERNEL, cpu_to_node(cpu));
+	/*
+	 * Overlap must have !overlap children.
+	 * This is before degenerate throws them out.
+	 */
+	BUG_ON(!sd->child);
 
-	if (!sg)
-		return NULL;
+	build_group_mask(sdd, sched_domain_span(child), mask);
+	cpu = cpumask_first_and(sched_domain_span(child), mask);
 
-	sg_span = sched_group_cpus(sg);
-	if (sd->child)
-		cpumask_copy(sg_span, sched_domain_span(sd->child));
-	else
-		cpumask_copy(sg_span, sched_domain_span(sd));
+	BUG_ON(cpu >= nr_cpu_ids);
 
-	return sg;
-}
+	group = *per_cpu_ptr(sdd->sg, cpu);
+	group->sgc = *per_cpu_ptr(sdd->sgc, cpu);
 
-static void init_overlap_sched_group(struct sched_domain *sd,
-				     struct sched_group *sg)
-{
-	struct cpumask *mask = sched_domains_tmpmask2;
-	struct sd_data *sdd = sd->private;
-	struct cpumask *sg_span;
-	int cpu;
+	atomic_inc(&group->ref);
+	atomic_inc(&group->sgc->ref);
 
-	build_group_mask(sd, sg, mask);
-	cpu = cpumask_first_and(sched_group_cpus(sg), mask);
+	cpumask_copy(sched_group_cpus(group), sched_domain_span(child));
+	cpumask_copy(sched_group_mask(group), mask);
 
-	sg->sgc = *per_cpu_ptr(sdd->sgc, cpu);
-	if (atomic_inc_return(&sg->sgc->ref) == 1)
-		cpumask_copy(sched_group_mask(sg), mask);
-
-	/*
-	 * Initialize sgc->capacity such that even if we mess up the
-	 * domains and no possible iteration will get us here, we won't
-	 * die on a /0 trap.
-	 */
-	sg_span = sched_group_cpus(sg);
-	sg->sgc->capacity = SCHED_CAPACITY_SCALE * cpumask_weight(sg_span);
-	sg->sgc->min_capacity = SCHED_CAPACITY_SCALE;
+	return group;
 }
 
 static int
@@ -620,14 +603,18 @@ build_overlap_sched_groups(struct sched_
 		if (!cpumask_test_cpu(i, sched_domain_span(sibling)))
 			continue;
 
-		sg = build_group_from_child_sched_domain(sibling, cpu);
-		if (!sg)
-			goto fail;
+		sg = get_overlap_group(sdd, i);
 
 		sg_span = sched_group_cpus(sg);
 		cpumask_or(covered, covered, sg_span);
 
-		init_overlap_sched_group(sd, sg);
+		/*
+		 * Initialize sgc->capacity such that even if we mess up the
+		 * domains and no possible iteration will get us here, we won't
+		 * die on a /0 trap.
+		 */
+		sg->sgc->capacity = SCHED_CAPACITY_SCALE * cpumask_weight(sg_span);
+		sg->sgc->min_capacity = SCHED_CAPACITY_SCALE;
 
 		if (!first)
 			first = sg;
@@ -639,11 +626,6 @@ build_overlap_sched_groups(struct sched_
 	sd->groups = first;
 
 	return 0;
-
-fail:
-	free_sched_groups(first, 0);
-
-	return -ENOMEM;
 }
 
 static int get_group(int cpu, struct sd_data *sdd, struct sched_group **sg)

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

#1630667

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 17:30 +0200
Message-ID<tAazE-6sV-17@gated-at.bofh.it>
In reply to#1630559
On Tue, Apr 25, 2017 at 05:12:00PM +0200, Peter Zijlstra wrote:
> But I'll first try and figure out why I'm not having empty masks.

Ah, so this is before all the degenerate stuff, so there's a bunch of
redundant domains below that make it work -- and there always will be,
unless FORCE_SD_OVERLAP.

Now I wonder what triggered it.. let me put it back.

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

#1630669

FromPeter Zijlstra <peterz@infradead.org>
Date2017-04-25 17:30 +0200
Message-ID<tAazE-6sV-15@gated-at.bofh.it>
In reply to#1630559
On Tue, Apr 25, 2017 at 11:33:51AM -0300, Lauro Venancio wrote:
> On 04/25/2017 09:17 AM, Peter Zijlstra wrote:

> > With the fact that sched_group_cpus(sd->groups) ==
> > sched_domain_span(sibling->child) (if child exists) established in the
> > previous patches, could we not simplify this like the below?

> We can. We just need to better handle the case when there is no child or
> we will have empty masks.
> We have to replicate the build_group_from_child_sched_domain() behavior:
> 
> if (sd->child)
>     cpumask_copy(sg_span, sched_domain_span(sd->child));
> else
>     cpumask_copy(sg_span, sched_domain_span(sd));
> 
> 
> So we need something like:
> 
> 
> if (sibling->child)
>     gsd = sibling->child;
> else
>     gsd = sibling;
> 
> if (!cpumask_equal(sg_span, sched_domain_span(gsd)))
> 
> 	continue;

Right, ran into that already. My truncated topologies (single cpu per
node) insta triggered it. But somehow removing the WARN was sufficient
and the mask didn't end up empty.

This is the ring topo:

[    0.086772] CPU0 attaching sched-domain:
[    0.087005]  domain 0: span 0-1,3 level NUMA
[    0.088002]   groups: 0 (mask: 0), 1, 3
[    0.089002]   domain 1: span 0-3 level NUMA
[    0.090002]    groups: 0-1,3 (mask: 0) (cpu_capacity: 3072), 1-3 (cpu_capacity: 3072)
[    0.091005] CPU1 attaching sched-domain:
[    0.092003]  domain 0: span 0-2 level NUMA
[    0.093002]   groups: 1 (mask: 1), 2, 0
[    0.094002]   domain 1: span 0-3 level NUMA
[    0.095002]    groups: 0-2 (mask: 1) (cpu_capacity: 3072), 0,2-3 (cpu_capacity: 3072)
[    0.096005] CPU2 attaching sched-domain:
[    0.097002]  domain 0: span 1-3 level NUMA
[    0.098002]   groups: 2 (mask: 2), 3, 1
[    0.099002]   domain 1: span 0-3 level NUMA
[    0.100002]    groups: 1-3 (mask: 2) (cpu_capacity: 3072), 0-1,3 (cpu_capacity: 3072)
[    0.101004] CPU3 attaching sched-domain:
[    0.102002]  domain 0: span 0,2-3 level NUMA
[    0.103002]   groups: 3 (mask: 3), 0, 2
[    0.104002]   domain 1: span 0-3 level NUMA
[    0.105002]    groups: 0,2-3 (mask: 3) (cpu_capacity: 3072), 0-2 (cpu_capacity: 3072)

See how the domain-0 mask isn't empty.


That said, when !child, ->groups ends up being a single cpu.

So I was thinking:

	struct cpumask *i_span;

	if (sibling->child)
		i_span = sched_domain_span(sibling->child);
	else
		i_span = cpumask_of(i);

	if (!cpumask_equal(sg_span, i_span))
		continue;


But I'll first try and figure out why I'm not having empty masks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web