[PATCH v2 1/6] powerpc/smp: Cache CPU has Asymmetric SMP

[PATCH v2 1/6] powerpc/smp: Cache CPU has Asymmetric SMP

[Srikar Dronamraju]

Currently cpu feature flag is checked whenever powerpc_smt_flags gets
called. This is an unnecessary overhead. CPU_FTR_ASYM_SMT is set based
on the processor and all processors will either have this set or will
have it unset.

Hence only check for the feature flag once and cache it to be used
subsequently. This commit will help avoid a branch in powerpc_smt_flags

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
Changelog:
v1->v2: Using static keys instead of a variable.
Using pr_info_once instead of printk

 arch/powerpc/kernel/smp.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 5826f5108a12..37c41297c9ce 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -988,18 +988,16 @@ static int __init init_thread_group_cache_map(int cpu, int cache_property)
 }
 
 static bool shared_caches;
+DEFINE_STATIC_KEY_FALSE(powerpc_asym_packing);
 
 #ifdef CONFIG_SCHED_SMT
 /* cpumask of CPUs with asymmetric SMT dependency */
 static int powerpc_smt_flags(void)
 {
-	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
+	if (static_branch_unlikely(&powerpc_asym_packing))
+		return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
 
-	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
-		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
-		flags |= SD_ASYM_PACKING;
-	}
-	return flags;
+	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
 }
 #endif
 
@@ -1686,6 +1684,11 @@ static void __init fixup_topology(void)
 {
 	int i;
 
+	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
+		pr_info_once("Enabling Asymmetric SMT scheduling\n");
+		static_branch_enable(&powerpc_asym_packing);
+	}
+
 #ifdef CONFIG_SCHED_SMT
 	if (has_big_cores) {
 		pr_info("Big cores detected but using small core scheduling\n");
-- 
2.31.1

Re: [PATCH v2 1/6] powerpc/smp: Cache CPU has Asymmetric SMP

[Michael Ellerman]

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> Currently cpu feature flag is checked whenever powerpc_smt_flags gets
> called. This is an unnecessary overhead. CPU_FTR_ASYM_SMT is set based
> on the processor and all processors will either have this set or will
> have it unset.

The cpu_has_feature() test is implemented with a static key.

So AFAICS this is just replacing one static key with another?

I see that you use the new static key in subsequent patches. But
couldn't those just use the existing cpu feature test?

Anyway I'd be interested to see how the generated code differs
before/after this.

cheers

> Hence only check for the feature flag once and cache it to be used
> subsequently. This commit will help avoid a branch in powerpc_smt_flags
>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> Changelog:
> v1->v2: Using static keys instead of a variable.
> Using pr_info_once instead of printk
>
>  arch/powerpc/kernel/smp.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 5826f5108a12..37c41297c9ce 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -988,18 +988,16 @@ static int __init init_thread_group_cache_map(int cpu, int cache_property)
>  }
>  
>  static bool shared_caches;
> +DEFINE_STATIC_KEY_FALSE(powerpc_asym_packing);
>  
>  #ifdef CONFIG_SCHED_SMT
>  /* cpumask of CPUs with asymmetric SMT dependency */
>  static int powerpc_smt_flags(void)
>  {
> -	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
> +	if (static_branch_unlikely(&powerpc_asym_packing))
> +		return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
>  
> -	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
> -		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
> -		flags |= SD_ASYM_PACKING;
> -	}
> -	return flags;
> +	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
>  }
>  #endif
>  
> @@ -1686,6 +1684,11 @@ static void __init fixup_topology(void)
>  {
>  	int i;
>  
> +	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
> +		pr_info_once("Enabling Asymmetric SMT scheduling\n");
> +		static_branch_enable(&powerpc_asym_packing);
> +	}
> +
>  #ifdef CONFIG_SCHED_SMT
>  	if (has_big_cores) {
>  		pr_info("Big cores detected but using small core scheduling\n");
> -- 
> 2.31.1

Re: [PATCH v2 1/6] powerpc/smp: Cache CPU has Asymmetric SMP

[Srikar Dronamraju]

* Michael Ellerman <mpe@ellerman.id.au> [2023-10-19 15:33:16]:

> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > Currently cpu feature flag is checked whenever powerpc_smt_flags gets
> > called. This is an unnecessary overhead. CPU_FTR_ASYM_SMT is set based
> > on the processor and all processors will either have this set or will
> > have it unset.
> 
> The cpu_has_feature() test is implemented with a static key.
> 
> So AFAICS this is just replacing one static key with another?
> 

> I see that you use the new static key in subsequent patches. But
> couldn't those just use the existing cpu feature test?
> 

Yes, we can use the existing cpu feature test itself.

> Anyway I'd be interested to see how the generated code differs
> before/after this.
> 
---------------------------->8----------------------------------------------8<------------
Before this change
0000000000000500 <powerpc_smt_flags>:
{
     500:	00 00 4c 3c 	addis   r2,r12,0
     504:	00 00 42 38 	addi    r2,r2,0
     508:	a6 02 08 7c 	mflr    r0
     50c:	01 00 00 48 	bl      50c <powerpc_smt_flags+0xc>
     510:	f8 ff e1 fb 	std     r31,-8(r1)
     514:	91 ff 21 f8 	stdu    r1,-112(r1)
#define JUMP_ENTRY_TYPE		stringify_in_c(FTR_ENTRY_LONG)
#define JUMP_LABEL_NOP_SIZE	4

static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
	asm_volatile_goto("1:\n\t"
     518:	00 00 00 60 	nop
		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
     51c:	00 00 22 3d 	addis   r9,r2,0
		flags |= SD_ASYM_PACKING;
     520:	80 05 e0 3b 	li      r31,1408
		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
     524:	00 00 29 89 	lbz     r9,0(r9)
     528:	00 00 09 2c 	cmpwi   r9,0
     52c:	28 00 82 41 	beq     554 <powerpc_smt_flags+0x54>
}
     530:	70 00 21 38 	addi    r1,r1,112
     534:	b4 07 e3 7f 	extsw   r3,r31
     538:	f8 ff e1 eb 	ld      r31,-8(r1)
     53c:	20 00 80 4e 	blr
	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
     540:	80 01 e0 3b 	li      r31,384
}
     544:	70 00 21 38 	addi    r1,r1,112
     548:	b4 07 e3 7f 	extsw   r3,r31
     54c:	f8 ff e1 eb 	ld      r31,-8(r1)
     550:	20 00 80 4e 	blr
		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
     554:	a6 02 08 7c 	mflr    r0
     558:	00 00 62 3c 	addis   r3,r2,0
     55c:	01 00 20 39 	li      r9,1
     560:	00 00 42 3d 	addis   r10,r2,0
     564:	00 00 63 38 	addi    r3,r3,0
     568:	00 00 2a 99 	stb     r9,0(r10)
     56c:	80 00 01 f8 	std     r0,128(r1)
     570:	01 00 00 48 	bl      570 <powerpc_smt_flags+0x70>
     574:	00 00 00 60 	nop
     578:	80 00 01 e8 	ld      r0,128(r1)
     57c:	a6 03 08 7c 	mtlr    r0
     580:	b0 ff ff 4b 	b       530 <powerpc_smt_flags+0x30>
     584:	00 00 00 60 	nop
     588:	00 00 00 60 	nop
     58c:	00 00 00 60 	nop


post this change.
0000000000000340 <powerpc_smt_flags>:
{
     340:	a6 02 08 7c 	mflr    r0
     344:	01 00 00 48 	bl      344 <powerpc_smt_flags+0x4>
#define JUMP_ENTRY_TYPE		stringify_in_c(FTR_ENTRY_LONG)
#define JUMP_LABEL_NOP_SIZE	4

static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
	asm_volatile_goto("1:\n\t"
     348:	00 00 00 60 	nop
	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
     34c:	80 01 60 38 	li      r3,384
}
     350:	b4 07 63 7c 	extsw   r3,r3
     354:	20 00 80 4e 	blr
     358:	00 00 00 60 	nop
     35c:	00 00 00 60 	nop
		return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
     360:	80 05 60 38 	li      r3,1408
}
     364:	b4 07 63 7c 	extsw   r3,r3
     368:	20 00 80 4e 	blr
     36c:	00 00 00 60 	nop

---------------------------->8----------------------------------------------8<------------

I think the most of the difference is due to moving pr_info_once to
fixup_topology. Does it make sense to move the pr_info_once to
fixup_topology (which is called less often) from powerpc_smt_flags?

Even though the pr_info_once would probably translate to load + cmp + branch
we could avoid that for each smt_flag call.

So something like

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 5826f5108a12..bc22f775425b 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -993,13 +993,10 @@ static bool shared_caches;
 /* cpumask of CPUs with asymmetric SMT dependency */
 static int powerpc_smt_flags(void)
 {
-	int flags = SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
+	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
+		return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
 
-	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
-		printk_once(KERN_INFO "Enabling Asymmetric SMT scheduling\n");
-		flags |= SD_ASYM_PACKING;
-	}
-	return flags;
+	return SD_SHARE_CPUCAPACITY | SD_SHARE_PKG_RESOURCES;
 }
 #endif
 
@@ -1687,6 +1684,9 @@ static void __init fixup_topology(void)
 	int i;
 
 #ifdef CONFIG_SCHED_SMT
+	if (cpu_has_feature(CPU_FTR_ASYM_SMT))
+		pr_info_once("Enabling Asymmetric SMT scheduling\n");
+
 	if (has_big_cores) {
 		pr_info("Big cores detected but using small core scheduling\n");
 		powerpc_topology[smt_idx].mask = smallcore_smt_mask;
-- 
Thanks and Regards
Srikar Dronamraju

[PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Srikar Dronamraju]

If there are shared processor LPARs, underlying Hypervisor can have more
virtual cores to handle than actual physical cores.

Starting with Power 9, a core has 2 nearly independent thread groups.
On a shared processors LPARs, it helps to pack threads to lesser number
of cores so that the overall system performance and utilization
improves. PowerVM schedules at a core level. Hence packing to fewer
cores helps.

For example: Lets says there are two 8-core Shared LPARs that are
actually sharing a 8 Core shared physical pool, each running 8 threads
each. Then Consolidating 8 threads to 4 cores on each LPAR would help
them to perform better. This is because each of the LPAR will get
100% time to run applications and there will no switching required by
the Hypervisor.

To achieve this, enable SD_ASYM_PACKING flag at CACHE, MC and DIE level.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
Changelog:
v1->v2: Using static key instead of a variable.

 arch/powerpc/kernel/smp.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 37c41297c9ce..498c2d51fc20 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1009,9 +1009,20 @@ static int powerpc_smt_flags(void)
  */
 static int powerpc_shared_cache_flags(void)
 {
+	if (static_branch_unlikely(&powerpc_asym_packing))
+		return SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
+
 	return SD_SHARE_PKG_RESOURCES;
 }
 
+static int powerpc_shared_proc_flags(void)
+{
+	if (static_branch_unlikely(&powerpc_asym_packing))
+		return SD_ASYM_PACKING;
+
+	return 0;
+}
+
 /*
  * We can't just pass cpu_l2_cache_mask() directly because
  * returns a non-const pointer and the compiler barfs on that.
@@ -1048,8 +1059,8 @@ static struct sched_domain_topology_level powerpc_topology[] = {
 	{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
 #endif
 	{ shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
-	{ cpu_mc_mask, SD_INIT_NAME(MC) },
-	{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
+	{ cpu_mc_mask, powerpc_shared_proc_flags, SD_INIT_NAME(MC) },
+	{ cpu_cpu_mask, powerpc_shared_proc_flags, SD_INIT_NAME(DIE) },
 	{ NULL, },
 };
 
@@ -1687,6 +1698,8 @@ static void __init fixup_topology(void)
 	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
 		pr_info_once("Enabling Asymmetric SMT scheduling\n");
 		static_branch_enable(&powerpc_asym_packing);
+	} else if (is_shared_processor() && has_big_cores) {
+		static_branch_enable(&powerpc_asym_packing);
 	}
 
 #ifdef CONFIG_SCHED_SMT
-- 
2.31.1

Re: [PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Michael Ellerman]

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> If there are shared processor LPARs, underlying Hypervisor can have more
> virtual cores to handle than actual physical cores.
>
> Starting with Power 9, a core has 2 nearly independent thread groups.

You need to be clearer here that you're talking about "big cores", not
SMT4 cores as seen on bare metal systems.

> On a shared processors LPARs, it helps to pack threads to lesser number
> of cores so that the overall system performance and utilization
> improves. PowerVM schedules at a core level. Hence packing to fewer
> cores helps.
>
> For example: Lets says there are two 8-core Shared LPARs that are
> actually sharing a 8 Core shared physical pool, each running 8 threads
> each. Then Consolidating 8 threads to 4 cores on each LPAR would help
> them to perform better. This is because each of the LPAR will get
> 100% time to run applications and there will no switching required by
> the Hypervisor.
>
> To achieve this, enable SD_ASYM_PACKING flag at CACHE, MC and DIE level.

.. when the system is running in shared processor mode and has big cores.

cheers

> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 37c41297c9ce..498c2d51fc20 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1009,9 +1009,20 @@ static int powerpc_smt_flags(void)
>   */
>  static int powerpc_shared_cache_flags(void)
>  {
> +	if (static_branch_unlikely(&powerpc_asym_packing))
> +		return SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
> +
>  	return SD_SHARE_PKG_RESOURCES;
>  }
>  
> +static int powerpc_shared_proc_flags(void)
> +{
> +	if (static_branch_unlikely(&powerpc_asym_packing))
> +		return SD_ASYM_PACKING;
> +
> +	return 0;
> +}
> +
>  /*
>   * We can't just pass cpu_l2_cache_mask() directly because
>   * returns a non-const pointer and the compiler barfs on that.
> @@ -1048,8 +1059,8 @@ static struct sched_domain_topology_level powerpc_topology[] = {
>  	{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
>  #endif
>  	{ shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
> -	{ cpu_mc_mask, SD_INIT_NAME(MC) },
> -	{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
> +	{ cpu_mc_mask, powerpc_shared_proc_flags, SD_INIT_NAME(MC) },
> +	{ cpu_cpu_mask, powerpc_shared_proc_flags, SD_INIT_NAME(DIE) },
>  	{ NULL, },
>  };
>  
> @@ -1687,6 +1698,8 @@ static void __init fixup_topology(void)
>  	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
>  		pr_info_once("Enabling Asymmetric SMT scheduling\n");
>  		static_branch_enable(&powerpc_asym_packing);
> +	} else if (is_shared_processor() && has_big_cores) {
> +		static_branch_enable(&powerpc_asym_packing);
>  	}
>  
>  #ifdef CONFIG_SCHED_SMT
> -- 
> 2.31.1

Re: [PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Peter Zijlstra]

On Thu, Oct 19, 2023 at 03:38:40PM +1100, Michael Ellerman wrote:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > If there are shared processor LPARs, underlying Hypervisor can have more
> > virtual cores to handle than actual physical cores.
> >
> > Starting with Power 9, a core has 2 nearly independent thread groups.
> 
> You need to be clearer here that you're talking about "big cores", not
> SMT4 cores as seen on bare metal systems.

What is a 'big core' ? I'm thinking big.LITTLE, but I didn't think Power
went that route (yet?).. help?

> > On a shared processors LPARs, it helps to pack threads to lesser number
> > of cores so that the overall system performance and utilization
> > improves. PowerVM schedules at a core level. Hence packing to fewer
> > cores helps.
> >
> > For example: Lets says there are two 8-core Shared LPARs that are
> > actually sharing a 8 Core shared physical pool, each running 8 threads
> > each. Then Consolidating 8 threads to 4 cores on each LPAR would help
> > them to perform better. This is because each of the LPAR will get
> > 100% time to run applications and there will no switching required by
> > the Hypervisor.
> >
> > To achieve this, enable SD_ASYM_PACKING flag at CACHE, MC and DIE level.
> 
> .. when the system is running in shared processor mode and has big cores.
> 
> cheers
> 
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 37c41297c9ce..498c2d51fc20 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -1009,9 +1009,20 @@ static int powerpc_smt_flags(void)
> >   */
> >  static int powerpc_shared_cache_flags(void)
> >  {
> > +	if (static_branch_unlikely(&powerpc_asym_packing))
> > +		return SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
> > +
> >  	return SD_SHARE_PKG_RESOURCES;
> >  }
> >  
> > +static int powerpc_shared_proc_flags(void)
> > +{
> > +	if (static_branch_unlikely(&powerpc_asym_packing))
> > +		return SD_ASYM_PACKING;
> > +
> > +	return 0;
> > +}

Can you leave the future reader a clue in the form of a comment around
here perhaps? Explaining *why* things are as they are etc..

> > +
> >  /*
> >   * We can't just pass cpu_l2_cache_mask() directly because
> >   * returns a non-const pointer and the compiler barfs on that.
> > @@ -1048,8 +1059,8 @@ static struct sched_domain_topology_level powerpc_topology[] = {
> >  	{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
> >  #endif
> >  	{ shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
> > -	{ cpu_mc_mask, SD_INIT_NAME(MC) },
> > -	{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
> > +	{ cpu_mc_mask, powerpc_shared_proc_flags, SD_INIT_NAME(MC) },
> > +	{ cpu_cpu_mask, powerpc_shared_proc_flags, SD_INIT_NAME(DIE) },
> >  	{ NULL, },
> >  };
> >  
> > @@ -1687,6 +1698,8 @@ static void __init fixup_topology(void)
> >  	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
> >  		pr_info_once("Enabling Asymmetric SMT scheduling\n");
> >  		static_branch_enable(&powerpc_asym_packing);
> > +	} else if (is_shared_processor() && has_big_cores) {
> > +		static_branch_enable(&powerpc_asym_packing);
> >  	}
> >  
> >  #ifdef CONFIG_SCHED_SMT
> > -- 
> > 2.31.1

Re: [PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Michael Ellerman]

Peter Zijlstra <peterz@infradead.org> writes:
> On Thu, Oct 19, 2023 at 03:38:40PM +1100, Michael Ellerman wrote:
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > If there are shared processor LPARs, underlying Hypervisor can have more
>> > virtual cores to handle than actual physical cores.
>> >
>> > Starting with Power 9, a core has 2 nearly independent thread groups.
>> 
>> You need to be clearer here that you're talking about "big cores", not
>> SMT4 cores as seen on bare metal systems.
>
> What is a 'big core' ? I'm thinking big.LITTLE, but I didn't think Power
> went that route (yet?).. help?

No it's not big.LITTLE :)

It means we have two SMT4 cores glued together that behave as a single
SMT8 core, a system is either in "big core" mode or it's not, it's never
heterogeneous.

If you grep for "big_core" there's some code in the kernel for dealing
with it, though it's probably not very illuminating.

Possibly we should switch to using the "thread group" terminology, so
that it doesn't confuse folks about big.LITTLE. Also the device tree
property that we use to discover if we're using big cores is called
ibm,thread-groups.

cheers

Re: [PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2023-10-19 09:48:28]:

> On Thu, Oct 19, 2023 at 03:38:40PM +1100, Michael Ellerman wrote:
> > Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > > If there are shared processor LPARs, underlying Hypervisor can have more
> > > virtual cores to handle than actual physical cores.
> > >
> > > Starting with Power 9, a core has 2 nearly independent thread groups.
> > 
> > You need to be clearer here that you're talking about "big cores", not
> > SMT4 cores as seen on bare metal systems.
> 
> What is a 'big core' ? I'm thinking big.LITTLE, but I didn't think Power
> went that route (yet?).. help?
> 

Each independent thread group acts as a SMT4 core or a small core. A set of
2 thread groups form a SMT8 core aka big core. PowerVM aka pHYp schedules
at a big core granularity

So if we have 2 LPARS, each spanning 2 big cores, aka 16 CPUs, and if at
somepoint, each LPAR has only 2 threads to run, we are exploring if we can
run both the threads on just one big core, so that PhyP can schedule both
LPARS at the same time and avoid having to switch/multiplex between these
two LPARS.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Shrikanth Hegde]

On 10/18/23 10:07 PM, Srikar Dronamraju wrote:
> If there are shared processor LPARs, underlying Hypervisor can have more
> virtual cores to handle than actual physical cores.
> 
> Starting with Power 9, a core has 2 nearly independent thread groups.
> On a shared processors LPARs, it helps to pack threads to lesser number
> of cores so that the overall system performance and utilization
> improves. PowerVM schedules at a core level. Hence packing to fewer
> cores helps.
> 
> For example: Lets says there are two 8-core Shared LPARs that are
> actually sharing a 8 Core shared physical pool, each running 8 threads
> each. Then Consolidating 8 threads to 4 cores on each LPAR would help
> them to perform better. This is because each of the LPAR will get
> 100% time to run applications and there will no switching required by
> the Hypervisor.
> 
> To achieve this, enable SD_ASYM_PACKING flag at CACHE, MC and DIE level.

This would have a conflict with tip/master. 
DIE has been renamed to PKG and Both changelog and code below should 
change DIE to PKG. 

> 
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> Changelog:
> v1->v2: Using static key instead of a variable.
> 
>  arch/powerpc/kernel/smp.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 37c41297c9ce..498c2d51fc20 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1009,9 +1009,20 @@ static int powerpc_smt_flags(void)
>   */
>  static int powerpc_shared_cache_flags(void)
>  {
> +	if (static_branch_unlikely(&powerpc_asym_packing))
> +		return SD_SHARE_PKG_RESOURCES | SD_ASYM_PACKING;
> +
>  	return SD_SHARE_PKG_RESOURCES;
>  }
> 
> +static int powerpc_shared_proc_flags(void)
> +{
> +	if (static_branch_unlikely(&powerpc_asym_packing))
> +		return SD_ASYM_PACKING;
> +
> +	return 0;
> +}
> +
>  /*
>   * We can't just pass cpu_l2_cache_mask() directly because
>   * returns a non-const pointer and the compiler barfs on that.
> @@ -1048,8 +1059,8 @@ static struct sched_domain_topology_level powerpc_topology[] = {
>  	{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
>  #endif
>  	{ shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
> -	{ cpu_mc_mask, SD_INIT_NAME(MC) },
> -	{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
> +	{ cpu_mc_mask, powerpc_shared_proc_flags, SD_INIT_NAME(MC) },
> +	{ cpu_cpu_mask, powerpc_shared_proc_flags, SD_INIT_NAME(DIE) },
>  	{ NULL, },
>  };
> 
> @@ -1687,6 +1698,8 @@ static void __init fixup_topology(void)
>  	if (cpu_has_feature(CPU_FTR_ASYM_SMT)) {
>  		pr_info_once("Enabling Asymmetric SMT scheduling\n");
>  		static_branch_enable(&powerpc_asym_packing);
> +	} else if (is_shared_processor() && has_big_cores) {
> +		static_branch_enable(&powerpc_asym_packing);
>  	}
> 
>  #ifdef CONFIG_SCHED_SMT

Re: [PATCH v2 2/6] powerpc/smp: Enable Asym packing for cores on shared processor

[Srikar Dronamraju]

* Shrikanth Hegde <sshegde@linux.vnet.ibm.com> [2023-10-19 21:26:56]:

> 
> 
> On 10/18/23 10:07 PM, Srikar Dronamraju wrote:
> > If there are shared processor LPARs, underlying Hypervisor can have more
> > virtual cores to handle than actual physical cores.
> > 
> > Starting with Power 9, a core has 2 nearly independent thread groups.
> > On a shared processors LPARs, it helps to pack threads to lesser number
> > of cores so that the overall system performance and utilization
> > improves. PowerVM schedules at a core level. Hence packing to fewer
> > cores helps.
> > 
> > For example: Lets says there are two 8-core Shared LPARs that are
> > actually sharing a 8 Core shared physical pool, each running 8 threads
> > each. Then Consolidating 8 threads to 4 cores on each LPAR would help
> > them to perform better. This is because each of the LPAR will get
> > 100% time to run applications and there will no switching required by
> > the Hypervisor.
> > 
> > To achieve this, enable SD_ASYM_PACKING flag at CACHE, MC and DIE level.
> 
> This would have a conflict with tip/master. 
> DIE has been renamed to PKG and Both changelog and code below should 
> change DIE to PKG. 

Once the changes are part of powerpc/merge, will rebase and accomodate the
changes from DIE to PKG.


-- 
Thanks and Regards
Srikar Dronamraju

[PATCH v2 3/6] powerpc/smp: Move shared_processor static key to smp.h

[Srikar Dronamraju]

The ability to detect if the system is running in a shared processor
mode is helpful in few more generic cases not just in
paravirtualization.
For example: At boot time, different scheduler/ topology flags may be
set based on the processor mode. Hence move it to a more generic file.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/paravirt.h | 12 ------------
 arch/powerpc/include/asm/smp.h      | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index 0372b0093f72..cf83e837a571 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -15,13 +15,6 @@
 #include <asm/kvm_guest.h>
 #include <asm/cputhreads.h>
 
-DECLARE_STATIC_KEY_FALSE(shared_processor);
-
-static inline bool is_shared_processor(void)
-{
-	return static_branch_unlikely(&shared_processor);
-}
-
 #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
 extern struct static_key paravirt_steal_enabled;
 extern struct static_key paravirt_steal_rq_enabled;
@@ -77,11 +70,6 @@ static inline bool is_vcpu_idle(int vcpu)
 	return lppaca_of(vcpu).idle;
 }
 #else
-static inline bool is_shared_processor(void)
-{
-	return false;
-}
-
 static inline u32 yield_count_of(int cpu)
 {
 	return 0;
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index aaaa576d0e15..08631b2a4528 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -34,6 +34,20 @@ extern bool coregroup_enabled;
 extern int cpu_to_chip_id(int cpu);
 extern int *chip_id_lookup_table;
 
+#ifdef CONFIG_PPC_SPLPAR
+DECLARE_STATIC_KEY_FALSE(shared_processor);
+
+static inline bool is_shared_processor(void)
+{
+	return static_branch_unlikely(&shared_processor);
+}
+#else
+static inline bool is_shared_processor(void)
+{
+	return false;
+}
+#endif
+
 DECLARE_PER_CPU(cpumask_var_t, thread_group_l1_cache_map);
 DECLARE_PER_CPU(cpumask_var_t, thread_group_l2_cache_map);
 DECLARE_PER_CPU(cpumask_var_t, thread_group_l3_cache_map);
-- 
2.31.1

Re: [PATCH v2 3/6] powerpc/smp: Move shared_processor static key to smp.h

[Michael Ellerman]

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> The ability to detect if the system is running in a shared processor
> mode is helpful in few more generic cases not just in
> paravirtualization.
> For example: At boot time, different scheduler/ topology flags may be
> set based on the processor mode. Hence move it to a more generic file.

I'd rather you just included paravirt.h in the few files where you need it.

cheers

> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index 0372b0093f72..cf83e837a571 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -15,13 +15,6 @@
>  #include <asm/kvm_guest.h>
>  #include <asm/cputhreads.h>
>  
> -DECLARE_STATIC_KEY_FALSE(shared_processor);
> -
> -static inline bool is_shared_processor(void)
> -{
> -	return static_branch_unlikely(&shared_processor);
> -}
> -
>  #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
>  extern struct static_key paravirt_steal_enabled;
>  extern struct static_key paravirt_steal_rq_enabled;
> @@ -77,11 +70,6 @@ static inline bool is_vcpu_idle(int vcpu)
>  	return lppaca_of(vcpu).idle;
>  }
>  #else
> -static inline bool is_shared_processor(void)
> -{
> -	return false;
> -}
> -
>  static inline u32 yield_count_of(int cpu)
>  {
>  	return 0;
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index aaaa576d0e15..08631b2a4528 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -34,6 +34,20 @@ extern bool coregroup_enabled;
>  extern int cpu_to_chip_id(int cpu);
>  extern int *chip_id_lookup_table;
>  
> +#ifdef CONFIG_PPC_SPLPAR
> +DECLARE_STATIC_KEY_FALSE(shared_processor);
> +
> +static inline bool is_shared_processor(void)
> +{
> +	return static_branch_unlikely(&shared_processor);
> +}
> +#else
> +static inline bool is_shared_processor(void)
> +{
> +	return false;
> +}
> +#endif
> +
>  DECLARE_PER_CPU(cpumask_var_t, thread_group_l1_cache_map);
>  DECLARE_PER_CPU(cpumask_var_t, thread_group_l2_cache_map);
>  DECLARE_PER_CPU(cpumask_var_t, thread_group_l3_cache_map);
> -- 
> 2.31.1

Re: [PATCH v2 3/6] powerpc/smp: Move shared_processor static key to smp.h

[Srikar Dronamraju]

* Michael Ellerman <mpe@ellerman.id.au> [2023-10-19 15:41:40]:

> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > The ability to detect if the system is running in a shared processor
> > mode is helpful in few more generic cases not just in
> > paravirtualization.
> > For example: At boot time, different scheduler/ topology flags may be
> > set based on the processor mode. Hence move it to a more generic file.
> 
> I'd rather you just included paravirt.h in the few files where you need it.


I thought, detecting if a Processor was shared or not was more a
smp/processor related than a paravirt related.

Will drop as suggested.

> 
> cheers
> 
> > diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> > index 0372b0093f72..cf83e837a571 100644
> > --- a/arch/powerpc/include/asm/paravirt.h
> > +++ b/arch/powerpc/include/asm/paravirt.h
> > @@ -15,13 +15,6 @@
> >  #include <asm/kvm_guest.h>
> >  #include <asm/cputhreads.h>
> >  
> > -DECLARE_STATIC_KEY_FALSE(shared_processor);
> > -
> > -static inline bool is_shared_processor(void)
> > -{
> > -	return static_branch_unlikely(&shared_processor);
> > -}
> > -
> >  #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
> >  extern struct static_key paravirt_steal_enabled;
> >  extern struct static_key paravirt_steal_rq_enabled;
> > @@ -77,11 +70,6 @@ static inline bool is_vcpu_idle(int vcpu)
> >  	return lppaca_of(vcpu).idle;
> >  }
> >  #else
> > -static inline bool is_shared_processor(void)
> > -{
> > -	return false;
> > -}
> > -
> >  static inline u32 yield_count_of(int cpu)
> >  {
> >  	return 0;
> > diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> > index aaaa576d0e15..08631b2a4528 100644
> > --- a/arch/powerpc/include/asm/smp.h
> > +++ b/arch/powerpc/include/asm/smp.h
> > @@ -34,6 +34,20 @@ extern bool coregroup_enabled;
> >  extern int cpu_to_chip_id(int cpu);
> >  extern int *chip_id_lookup_table;
> >  
> > +#ifdef CONFIG_PPC_SPLPAR
> > +DECLARE_STATIC_KEY_FALSE(shared_processor);
> > +
> > +static inline bool is_shared_processor(void)
> > +{
> > +	return static_branch_unlikely(&shared_processor);
> > +}
> > +#else
> > +static inline bool is_shared_processor(void)
> > +{
> > +	return false;
> > +}
> > +#endif
> > +
> >  DECLARE_PER_CPU(cpumask_var_t, thread_group_l1_cache_map);
> >  DECLARE_PER_CPU(cpumask_var_t, thread_group_l2_cache_map);
> >  DECLARE_PER_CPU(cpumask_var_t, thread_group_l3_cache_map);
> > -- 
> > 2.31.1

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH v2 3/6] powerpc/smp: Move shared_processor static key to smp.h

[Michael Ellerman]

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Michael Ellerman <mpe@ellerman.id.au> [2023-10-19 15:41:40]:
>
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > The ability to detect if the system is running in a shared processor
>> > mode is helpful in few more generic cases not just in
>> > paravirtualization.
>> > For example: At boot time, different scheduler/ topology flags may be
>> > set based on the processor mode. Hence move it to a more generic file.
>> 
>> I'd rather you just included paravirt.h in the few files where you need it.
>
> I thought, detecting if a Processor was shared or not was more a
> smp/processor related than a paravirt related.

It's both really :)

It's definitely paravirt related though, because if we weren't
*para*virt then we wouldn't know there was a hypervisor at all :)

But having smaller more focused headers is preferable in general just
for mechanical reasons.

cheers

Re: [PATCH v2 3/6] powerpc/smp: Move shared_processor static key to smp.h

[Shrikanth Hegde]

On 10/18/23 10:07 PM, Srikar Dronamraju wrote:
> The ability to detect if the system is running in a shared processor
> mode is helpful in few more generic cases not just in
> paravirtualization.
> For example: At boot time, different scheduler/ topology flags may be
> set based on the processor mode. Hence move it to a more generic file.
> 
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 12 ------------
>  arch/powerpc/include/asm/smp.h      | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index 0372b0093f72..cf83e837a571 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -15,13 +15,6 @@
>  #include <asm/kvm_guest.h>
>  #include <asm/cputhreads.h>
> 
> -DECLARE_STATIC_KEY_FALSE(shared_processor);
> -
> -static inline bool is_shared_processor(void)
> -{
> -	return static_branch_unlikely(&shared_processor);
> -}
> -
>  #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
>  extern struct static_key paravirt_steal_enabled;
>  extern struct static_key paravirt_steal_rq_enabled;
> @@ -77,11 +70,6 @@ static inline bool is_vcpu_idle(int vcpu)
>  	return lppaca_of(vcpu).idle;
>  }

Hi Srikar, 

This patch fails to apply on tip/master as it depends on 
https://lore.kernel.org/all/20231019091452.95260-1-srikar@linux.vnet.ibm.com/ to be applied first.

>  #else
> -static inline bool is_shared_processor(void)
> -{
> -	return false;
> -}
> -
>  static inline u32 yield_count_of(int cpu)
>  {
>  	return 0;
> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
> index aaaa576d0e15..08631b2a4528 100644
> --- a/arch/powerpc/include/asm/smp.h
> +++ b/arch/powerpc/include/asm/smp.h
> @@ -34,6 +34,20 @@ extern bool coregroup_enabled;
>  extern int cpu_to_chip_id(int cpu);
>  extern int *chip_id_lookup_table;
> 
> +#ifdef CONFIG_PPC_SPLPAR
> +DECLARE_STATIC_KEY_FALSE(shared_processor);
> +
> +static inline bool is_shared_processor(void)
> +{
> +	return static_branch_unlikely(&shared_processor);
> +}
> +#else
> +static inline bool is_shared_processor(void)
> +{
> +	return false;
> +}
> +#endif
> +
>  DECLARE_PER_CPU(cpumask_var_t, thread_group_l1_cache_map);
>  DECLARE_PER_CPU(cpumask_var_t, thread_group_l2_cache_map);
>  DECLARE_PER_CPU(cpumask_var_t, thread_group_l3_cache_map);

[PATCH v2 4/6] powerpc/smp: Disable MC domain for shared processor

[Srikar Dronamraju]

Like L2-cache info, coregroup information which is used to determine MC
sched domains is only present on dedicated LPARs. i.e PowerVM doesn't
export coregroup information for shared processor LPARs. Hence disable
creating MC domains on shared LPAR Systems.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 498c2d51fc20..29da9262cb17 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1046,6 +1046,10 @@ static struct cpumask *cpu_coregroup_mask(int cpu)
 
 static bool has_coregroup_support(void)
 {
+	/* Coregroup identification not available on shared systems */
+	if (is_shared_processor())
+		return 0;
+
 	return coregroup_enabled;
 }
 
-- 
2.31.1

Re: [PATCH v2 4/6] powerpc/smp: Disable MC domain for shared processor

[Michael Ellerman]

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> Like L2-cache info, coregroup information which is used to determine MC
> sched domains is only present on dedicated LPARs. i.e PowerVM doesn't
> export coregroup information for shared processor LPARs. Hence disable
> creating MC domains on shared LPAR Systems.
>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/smp.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 498c2d51fc20..29da9262cb17 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1046,6 +1046,10 @@ static struct cpumask *cpu_coregroup_mask(int cpu)
>  
>  static bool has_coregroup_support(void)
>  {
> +	/* Coregroup identification not available on shared systems */
> +	if (is_shared_processor())
> +		return 0;

That will catch guests running under KVM too right? Do we want that?

>  	return coregroup_enabled;

What does coregroup_enabled mean now?

I'd rather this was actually checking the presence of something, rather
than just hard coding that shared processor means no coregroup support.

cheers

Re: [PATCH v2 4/6] powerpc/smp: Disable MC domain for shared processor

[Peter Zijlstra]

On Thu, Oct 19, 2023 at 03:48:48PM +1100, Michael Ellerman wrote:
> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > Like L2-cache info, coregroup information which is used to determine MC
> > sched domains is only present on dedicated LPARs. i.e PowerVM doesn't
> > export coregroup information for shared processor LPARs. Hence disable
> > creating MC domains on shared LPAR Systems.
> >
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/smp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 498c2d51fc20..29da9262cb17 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -1046,6 +1046,10 @@ static struct cpumask *cpu_coregroup_mask(int cpu)
> >  
> >  static bool has_coregroup_support(void)
> >  {
> > +	/* Coregroup identification not available on shared systems */
> > +	if (is_shared_processor())
> > +		return 0;
> 
> That will catch guests running under KVM too right? Do we want that?

Some KVM people use vcpu pinning and pass-through topology things,
slice-of-hardware or something like that. In that scenario you actively
do want this.

I'm fairly clueless on when this is_shared_processor() gets to be true,
so that might already be dealt with.. 

Re: [PATCH v2 4/6] powerpc/smp: Disable MC domain for shared processor

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2023-10-19 09:50:46]:

> On Thu, Oct 19, 2023 at 03:48:48PM +1100, Michael Ellerman wrote:
> > Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > > Like L2-cache info, coregroup information which is used to determine MC
> > > sched domains is only present on dedicated LPARs. i.e PowerVM doesn't
> > > export coregroup information for shared processor LPARs. Hence disable
> > > creating MC domains on shared LPAR Systems.
> > >
> > > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > > ---
> > >  arch/powerpc/kernel/smp.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > > index 498c2d51fc20..29da9262cb17 100644
> > > --- a/arch/powerpc/kernel/smp.c
> > > +++ b/arch/powerpc/kernel/smp.c
> > > @@ -1046,6 +1046,10 @@ static struct cpumask *cpu_coregroup_mask(int cpu)
> > >  
> > >  static bool has_coregroup_support(void)
> > >  {
> > > +	/* Coregroup identification not available on shared systems */
> > > +	if (is_shared_processor())
> > > +		return 0;
> > 
> > That will catch guests running under KVM too right? Do we want that?
> 
> Some KVM people use vcpu pinning and pass-through topology things,
> slice-of-hardware or something like that. In that scenario you actively
> do want this.
> 
> I'm fairly clueless on when this is_shared_processor() gets to be true,
> so that might already be dealt with.. 

Dedicated LPARs are like pinned VM instances. There is a one-to-one mapping
between the Physical core and the virtual core.

On Shared LPARs  are unlike regular unpinned VM instances. The Hypervisor
has the flexibility to schedule the virtual core on different Physical core
based on the availability. And like regular VMs, Shared LPARs also support
overcommiting i.e there can be more virtual cores than Physical cores. So
Hypervisor takes care of switching the cores based on its entitlement.

Hence On Shared LPARs, Hypervisors would like to keep the Hemisphere
locality of the cores transparent to the OS.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH v2 4/6] powerpc/smp: Disable MC domain for shared processor

[Srikar Dronamraju]

* Michael Ellerman <mpe@ellerman.id.au> [2023-10-19 15:48:48]:

> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> > Like L2-cache info, coregroup information which is used to determine MC
> > sched domains is only present on dedicated LPARs. i.e PowerVM doesn't
> > export coregroup information for shared processor LPARs. Hence disable
> > creating MC domains on shared LPAR Systems.
> >
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/smp.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 498c2d51fc20..29da9262cb17 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -1046,6 +1046,10 @@ static struct cpumask *cpu_coregroup_mask(int cpu)
> >  
> >  static bool has_coregroup_support(void)
> >  {
> > +	/* Coregroup identification not available on shared systems */
> > +	if (is_shared_processor())
> > +		return 0;
> 
> That will catch guests running under KVM too right? Do we want that?
> 

Only dedicated LPARS on PowerVM expose coregroup or Hemisphere information.
Currently other systems including KVMs don't expose this information to the
OS.

> >  	return coregroup_enabled;
> 
> What does coregroup_enabled mean now?
> 
> I'd rather this was actually checking the presence of something, rather
> than just hard coding that shared processor means no coregroup support.
> 

On a shared LPAR, the Hypervisors would like to have the flexibility to
schedule LPAR on any core within the DIE without having to further think
about the Hemisphere locality of the core.

> cheers

-- 
Thanks and Regards
Srikar Dronamraju

[PATCH v2 5/6] powerpc/smp: Add read_mostly attribute

[Srikar Dronamraju]

There are some variables that are only updated at boot time.
So add read_mostly attribute to such variables

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 29da9262cb17..b1eb11a66902 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -77,10 +77,10 @@ static DEFINE_PER_CPU(int, cpu_state) = { 0 };
 #endif
 
 struct task_struct *secondary_current;
-bool has_big_cores;
-bool coregroup_enabled;
-bool thread_group_shares_l2;
-bool thread_group_shares_l3;
+bool has_big_cores __read_mostly;
+bool coregroup_enabled __read_mostly;
+bool thread_group_shares_l2 __read_mostly;
+bool thread_group_shares_l3 __read_mostly;
 
 DEFINE_PER_CPU(cpumask_var_t, cpu_sibling_map);
 DEFINE_PER_CPU(cpumask_var_t, cpu_smallcore_map);
@@ -987,7 +987,7 @@ static int __init init_thread_group_cache_map(int cpu, int cache_property)
 	return 0;
 }
 
-static bool shared_caches;
+static bool shared_caches __read_mostly;
 DEFINE_STATIC_KEY_FALSE(powerpc_asym_packing);
 
 #ifdef CONFIG_SCHED_SMT
-- 
2.31.1

Re: [PATCH v2 5/6] powerpc/smp: Add read_mostly attribute

[Michael Ellerman]

Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> There are some variables that are only updated at boot time.
> So add read_mostly attribute to such variables

If they're only updated at boot time then __ro_after_init would be the
better annotation.

cheers

> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 29da9262cb17..b1eb11a66902 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -77,10 +77,10 @@ static DEFINE_PER_CPU(int, cpu_state) = { 0 };
>  #endif
>  
>  struct task_struct *secondary_current;
> -bool has_big_cores;
> -bool coregroup_enabled;
> -bool thread_group_shares_l2;
> -bool thread_group_shares_l3;
> +bool has_big_cores __read_mostly;
> +bool coregroup_enabled __read_mostly;
> +bool thread_group_shares_l2 __read_mostly;
> +bool thread_group_shares_l3 __read_mostly;
>  
>  DEFINE_PER_CPU(cpumask_var_t, cpu_sibling_map);
>  DEFINE_PER_CPU(cpumask_var_t, cpu_smallcore_map);
> @@ -987,7 +987,7 @@ static int __init init_thread_group_cache_map(int cpu, int cache_property)
>  	return 0;
>  }
>  
> -static bool shared_caches;
> +static bool shared_caches __read_mostly;
>  DEFINE_STATIC_KEY_FALSE(powerpc_asym_packing);
>  
>  #ifdef CONFIG_SCHED_SMT
> -- 
> 2.31.1

Re: [PATCH v2 5/6] powerpc/smp: Add read_mostly attribute

[Peter Zijlstra]

On Wed, Oct 18, 2023 at 10:07:45PM +0530, Srikar Dronamraju wrote:
> There are some variables that are only updated at boot time.
> So add read_mostly attribute to such variables

You don't have __ro_after_init ?

Re: [PATCH v2 5/6] powerpc/smp: Add read_mostly attribute

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2023-10-19 09:51:27]:

> On Wed, Oct 18, 2023 at 10:07:45PM +0530, Srikar Dronamraju wrote:
> > There are some variables that are only updated at boot time.
> > So add read_mostly attribute to such variables
> 
> You don't have __ro_after_init ?

Michael also responded with the same input, will change over to
__ro_after_init in the next iteration.

-- 
Thanks and Regards
Srikar Dronamraju

[PATCH v2 6/6] powerpc/smp: Avoid asym packing within thread_group of a core

[Srikar Dronamraju]

PowerVM Hypervisor will schedule at a core granularity. However each
core can have more than one thread_groups. For better utilization in
case of a shared processor, its preferable for the scheduler to pack to
the lowest core. However there is no benefit of moving a thread between
two thread groups of the same core.

Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index b1eb11a66902..a710fb32a2a9 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1695,6 +1695,8 @@ void start_secondary(void *unused)
 	BUG();
 }
 
+DEFINE_STATIC_KEY_FALSE(splpar_asym_pack);
+
 static void __init fixup_topology(void)
 {
 	int i;
@@ -1704,6 +1706,7 @@ static void __init fixup_topology(void)
 		static_branch_enable(&powerpc_asym_packing);
 	} else if (is_shared_processor() && has_big_cores) {
 		static_branch_enable(&powerpc_asym_packing);
+		static_branch_enable(&splpar_asym_pack);
 	}
 
 #ifdef CONFIG_SCHED_SMT
@@ -1758,6 +1761,19 @@ void __init smp_cpus_done(unsigned int max_cpus)
 	set_sched_topology(powerpc_topology);
 }
 
+/*
+ * For asym packing, by default lower numbered CPU has higher priority.
+ * On shared processors, pack to lower numbered core. However avoid moving
+ * between thread_groups within the same core.
+ */
+int arch_asym_cpu_priority(int cpu)
+{
+	if (static_branch_unlikely(&splpar_asym_pack))
+		return -cpu / threads_per_core;
+
+	return -cpu;
+}
+
 #ifdef CONFIG_HOTPLUG_CPU
 int __cpu_disable(void)
 {
-- 
2.31.1