mask is built in build_balance_mask() by for_each_cpu(i, sg_span), so it must be a subset of sched_group_span(sg). Though cpumask_first_and doesn't lead to a wrong result of balance cpu, it is pointless to do cpumask_and again.
Signed-off-by: Barry Song song.bao.hua@hisilicon.com --- kernel/sched/topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c index 12f8058..45f3db2 100644 --- a/kernel/sched/topology.c +++ b/kernel/sched/topology.c @@ -934,7 +934,7 @@ static void init_overlap_sched_group(struct sched_domain *sd, int cpu;
build_balance_mask(sd, sg, mask); - cpu = cpumask_first_and(sched_group_span(sg), mask); + cpu = cpumask_first(mask);
sg->sgc = *per_cpu_ptr(sdd->sgc, cpu); if (atomic_inc_return(&sg->sgc->ref) == 1)
On 05/03/21 11:29, Barry Song wrote:
mask is built in build_balance_mask() by for_each_cpu(i, sg_span), so it must be a subset of sched_group_span(sg).
So we should indeed have
cpumask_subset(sched_group_span(sg), mask)
but that doesn't imply
cpumask_first(sched_group_span(sg)) == cpumask_first(mask)
does it? I'm thinking if in your topology of N CPUs, CPUs 0 and N-1 are the furthest away, you will most likely hit
!cpumask_equal(sg_pan, sched_domain_span(sibling->child)) ^^^^^^ ^^^^^^^^^^^^^ CPUN-1 CPU0
which should be the case on your Kunpeng920 system.
Though cpumask_first_and doesn't lead to a wrong result of balance cpu, it is pointless to do cpumask_and again.
Signed-off-by: Barry Song song.bao.hua@hisilicon.com
kernel/sched/topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c index 12f8058..45f3db2 100644 --- a/kernel/sched/topology.c +++ b/kernel/sched/topology.c @@ -934,7 +934,7 @@ static void init_overlap_sched_group(struct sched_domain *sd, int cpu;
build_balance_mask(sd, sg, mask);
- cpu = cpumask_first_and(sched_group_span(sg), mask);
cpu = cpumask_first(mask);
sg->sgc = *per_cpu_ptr(sdd->sgc, cpu); if (atomic_inc_return(&sg->sgc->ref) == 1)
-- 1.8.3.1
-----Original Message----- From: Valentin Schneider [mailto:valentin.schneider@arm.com] Sent: Saturday, March 6, 2021 12:49 AM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com; vincent.guittot@linaro.org; mingo@redhat.com; peterz@infradead.org; juri.lelli@redhat.com; dietmar.eggemann@arm.com; rostedt@goodmis.org; bsegall@google.com; mgorman@suse.de Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org; Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com Subject: Re: [PATCH] sched/topology: remove redundant cpumask_and in init_overlap_sched_group
On 05/03/21 11:29, Barry Song wrote:
mask is built in build_balance_mask() by for_each_cpu(i, sg_span), so it must be a subset of sched_group_span(sg).
So we should indeed have
cpumask_subset(sched_group_span(sg), mask)
but that doesn't imply
cpumask_first(sched_group_span(sg)) == cpumask_first(mask)
does it? I'm thinking if in your topology of N CPUs, CPUs 0 and N-1 are the furthest away, you will most likely hit
It is true: cpumask_first(sched_group_span(sg)) != cpumask_first(mask)
but
cpumask_first_and(sched_group_span(sg), mask) = cpumask_first(mask)
since mask is always subset of sched_group_span(sg).
/** * cpumask_first_and - return the first cpu from *srcp1 & *srcp2 * @src1p: the first input * @src2p: the second input * * Returns >= nr_cpu_ids if no cpus set in both. See also cpumask_next_and(). */
*srcp2 is subset of *srcp1, so *srcp1 & *srcp2 = *srcp2
!cpumask_equal(sg_pan, sched_domain_span(sibling->child)) ^^^^^^ ^^^^^^^^^^^^^ CPUN-1 CPU0
which should be the case on your Kunpeng920 system.
Though cpumask_first_and doesn't lead to a wrong result of balance cpu, it is pointless to do cpumask_and again.
Signed-off-by: Barry Song song.bao.hua@hisilicon.com
kernel/sched/topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c index 12f8058..45f3db2 100644 --- a/kernel/sched/topology.c +++ b/kernel/sched/topology.c @@ -934,7 +934,7 @@ static void init_overlap_sched_group(struct sched_domain
*sd,
int cpu;
build_balance_mask(sd, sg, mask);
- cpu = cpumask_first_and(sched_group_span(sg), mask);
cpu = cpumask_first(mask);
sg->sgc = *per_cpu_ptr(sdd->sgc, cpu); if (atomic_inc_return(&sg->sgc->ref) == 1)
-- 1.8.3.1
Thanks Barry
On 05/03/21 20:25, Song Bao Hua (Barry Song) wrote:
-----Original Message----- From: Valentin Schneider [mailto:valentin.schneider@arm.com] Sent: Saturday, March 6, 2021 12:49 AM To: Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com; vincent.guittot@linaro.org; mingo@redhat.com; peterz@infradead.org; juri.lelli@redhat.com; dietmar.eggemann@arm.com; rostedt@goodmis.org; bsegall@google.com; mgorman@suse.de Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org; Song Bao Hua (Barry Song) song.bao.hua@hisilicon.com Subject: Re: [PATCH] sched/topology: remove redundant cpumask_and in init_overlap_sched_group
On 05/03/21 11:29, Barry Song wrote:
mask is built in build_balance_mask() by for_each_cpu(i, sg_span), so it must be a subset of sched_group_span(sg).
So we should indeed have
cpumask_subset(sched_group_span(sg), mask)
but that doesn't imply
cpumask_first(sched_group_span(sg)) == cpumask_first(mask)
does it? I'm thinking if in your topology of N CPUs, CPUs 0 and N-1 are the furthest away, you will most likely hit
It is true: cpumask_first(sched_group_span(sg)) != cpumask_first(mask)
but
cpumask_first_and(sched_group_span(sg), mask) = cpumask_first(mask)
since mask is always subset of sched_group_span(sg).
You're right, I read it the wrong way around, sorry about that.
Reviewed-by: Valentin Schneider Valentin.Schneider@arm.com
-
Barry Song -
Song Bao Hua (Barry Song)
-
Valentin Schneider