[PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Srikar Dronamraju]

PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
CPU from a core that is preempted may have a larger latency. In
such a scenario, its preferable to choose a different CPU to run.

If one of the CPUs in the core is active, i.e neither CEDED nor
preempted, then consider this CPU as not preempted.

Also if any of the CPUs in the core has yielded but OS has not requested
CEDE or CONFER, then consider this CPU to be preempted.

Cc: Ajay Kaher <akaher@vmware.com>
Cc: Alexey Makhalov <amakhalov@vmware.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Juergen Gross <jgross@suse.com>
Cc: linux-kernel@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index e08513d73119..a980756f58df 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
 	if (!is_shared_processor())
 		return false;
 
+	if (!(yield_count_of(cpu) & 1))
+		return false;
+
+	/*
+	 * If CPU has yielded but OS has not requested idle then this CPU is
+	 * definitely preempted.
+	 */
+	if (!lppaca_of(cpu).idle)
+		return true;
+
 #ifdef CONFIG_PPC_SPLPAR
 	if (!is_kvm_guest()) {
-		int first_cpu;
+		int first_cpu, i;
 
 		/*
 		 * The result of vcpu_is_preempted() is used in a
@@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
 		 */
 		if (cpu_first_thread_sibling(cpu) == first_cpu)
 			return false;
+
+		/*
+		 * If any of the threads of this core is not preempted or
+		 * ceded, then consider this CPU to be non-preempted
+		 */
+		first_cpu = cpu_first_thread_sibling(cpu);
+		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
+			if (i == cpu)
+				continue;
+			if (!(yield_count_of(i) & 1))
+				return false;
+			if (!lppaca_of(i).idle)
+				return true;
+		}
 	}
 #endif
 
-	if (yield_count_of(cpu) & 1)
-		return true;
+	/*
+	 * None of the threads in this thread group are running but none of
+	 * them were preempted too. Hence assume the thread to be
+	 * non-preempted.
+	 */
 	return false;
 }
 
-- 
2.31.1

[PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Srikar Dronamraju]

PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
CPU from a core that is preempted may have a larger latency. In
such a scenario, its preferable to choose a different CPU to run.

If one of the CPUs in the core is active, i.e neither CEDED nor
preempted, then consider this CPU as not preempted.

Also if any of the CPUs in the core has yielded but OS has not requested
CEDE or CONFER, then consider this CPU to be preempted.

Cc: Ajay Kaher <akaher@vmware.com>
Cc: Alexey Makhalov <amakhalov@vmware.com>
Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Cc: Juergen Gross <jgross@suse.com>
Cc: linux-kernel@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index e08513d73119..a980756f58df 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
 	if (!is_shared_processor())
 		return false;
 
+	if (!(yield_count_of(cpu) & 1))
+		return false;
+
+	/*
+	 * If CPU has yielded but OS has not requested idle then this CPU is
+	 * definitely preempted.
+	 */
+	if (!lppaca_of(cpu).idle)
+		return true;
+
 #ifdef CONFIG_PPC_SPLPAR
 	if (!is_kvm_guest()) {
-		int first_cpu;
+		int first_cpu, i;
 
 		/*
 		 * The result of vcpu_is_preempted() is used in a
@@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
 		 */
 		if (cpu_first_thread_sibling(cpu) == first_cpu)
 			return false;
+
+		/*
+		 * If any of the threads of this core is not preempted or
+		 * ceded, then consider this CPU to be non-preempted
+		 */
+		first_cpu = cpu_first_thread_sibling(cpu);
+		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
+			if (i == cpu)
+				continue;
+			if (!(yield_count_of(i) & 1))
+				return false;
+			if (!lppaca_of(i).idle)
+				return true;
+		}
 	}
 #endif
 
-	if (yield_count_of(cpu) & 1)
-		return true;
+	/*
+	 * None of the threads in this thread group are running but none of
+	 * them were preempted too. Hence assume the thread to be
+	 * non-preempted.
+	 */
 	return false;
 }
 
-- 
2.31.1

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[kernel test robot]

Hi Srikar,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on linus/master v6.6-rc5 next-20231009]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Srikar-Dronamraju/powerpc-paravirt-Improve-vcpu_is_preempted/20231009-131957
base:   tip/x86/core
patch link:    https://lore.kernel.org/r/20231009051740.17683-1-srikar%40linux.vnet.ibm.com
patch subject: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted
config: powerpc-randconfig-002-20231009 (https://download.01.org/0day-ci/archive/20231009/202310091636.lElmJkYV-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231009/202310091636.lElmJkYV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310091636.lElmJkYV-lkp@intel.com/

All errors (new ones prefixed by >>):

   scripts/genksyms/parse.y: warning: 9 shift/reduce conflicts [-Wconflicts-sr]
   scripts/genksyms/parse.y: warning: 5 reduce/reduce conflicts [-Wconflicts-rr]
   scripts/genksyms/parse.y: note: rerun with option '-Wcounterexamples' to generate conflict counterexamples
   In file included from arch/powerpc/include/asm/qspinlock.h:7,
                    from arch/powerpc/include/asm/spinlock.h:7,
                    from include/linux/spinlock.h:95,
                    from include/linux/ipc.h:5,
                    from include/uapi/linux/sem.h:5,
                    from include/linux/sem.h:5,
                    from include/linux/compat.h:14,
                    from arch/powerpc/kernel/asm-offsets.c:12:
   arch/powerpc/include/asm/paravirt.h: In function 'vcpu_is_preempted':
>> arch/powerpc/include/asm/paravirt.h:130:14: error: implicit declaration of function 'lppaca_of' [-Werror=implicit-function-declaration]
     130 |         if (!lppaca_of(cpu).idle)
         |              ^~~~~~~~~
>> arch/powerpc/include/asm/paravirt.h:130:28: error: request for member 'idle' in something not a structure or union
     130 |         if (!lppaca_of(cpu).idle)
         |                            ^
   cc1: some warnings being treated as errors
   make[3]: *** [scripts/Makefile.build:116: arch/powerpc/kernel/asm-offsets.s] Error 1 shuffle=3266844477
   make[3]: Target 'prepare' not remade because of errors.
   make[2]: *** [Makefile:1287: prepare0] Error 2 shuffle=3266844477
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:234: __sub-make] Error 2 shuffle=3266844477
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:234: __sub-make] Error 2 shuffle=3266844477
   make: Target 'prepare' not remade because of errors.


vim +/lppaca_of +130 arch/powerpc/include/asm/paravirt.h

   103	
   104	#define vcpu_is_preempted vcpu_is_preempted
   105	static inline bool vcpu_is_preempted(int cpu)
   106	{
   107		/*
   108		 * The dispatch/yield bit alone is an imperfect indicator of
   109		 * whether the hypervisor has dispatched @cpu to run on a physical
   110		 * processor. When it is clear, @cpu is definitely not preempted.
   111		 * But when it is set, it means only that it *might* be, subject to
   112		 * other conditions. So we check other properties of the VM and
   113		 * @cpu first, resorting to the yield count last.
   114		 */
   115	
   116		/*
   117		 * Hypervisor preemption isn't possible in dedicated processor
   118		 * mode by definition.
   119		 */
   120		if (!is_shared_processor())
   121			return false;
   122	
   123		if (!(yield_count_of(cpu) & 1))
   124			return false;
   125	
   126		/*
   127		 * If CPU has yielded but OS has not requested idle then this CPU is
   128		 * definitely preempted.
   129		 */
 > 130		if (!lppaca_of(cpu).idle)
   131			return true;
   132	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[kernel test robot]

Hi Srikar,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/x86/core]
[also build test ERROR on linus/master v6.6-rc5 next-20231010]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Srikar-Dronamraju/powerpc-paravirt-Improve-vcpu_is_preempted/20231009-131957
base:   tip/x86/core
patch link:    https://lore.kernel.org/r/20231009051740.17683-1-srikar%40linux.vnet.ibm.com
patch subject: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted
config: powerpc64-randconfig-003-20231010 (https://download.01.org/0day-ci/archive/20231010/202310102341.K0sgoqQL-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231010/202310102341.K0sgoqQL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310102341.K0sgoqQL-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/paca.h:18,
                    from arch/powerpc/include/asm/current.h:13,
                    from include/linux/thread_info.h:23,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/powerpc/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/ipc.h:5,
                    from include/uapi/linux/sem.h:5,
                    from include/linux/sem.h:5,
                    from include/linux/compat.h:14,
                    from arch/powerpc/kernel/asm-offsets.c:12:
   arch/powerpc/include/asm/paravirt.h: In function 'vcpu_is_preempted':
>> arch/powerpc/include/asm/lppaca.h:121:41: error: 'struct paca_struct' has no member named 'lppaca_ptr'
     121 | #define lppaca_of(cpu)  (*paca_ptrs[cpu]->lppaca_ptr)
         |                                         ^~
   arch/powerpc/include/asm/paravirt.h:130:14: note: in expansion of macro 'lppaca_of'
     130 |         if (!lppaca_of(cpu).idle)
         |              ^~~~~~~~~
   make[3]: *** [scripts/Makefile.build:116: arch/powerpc/kernel/asm-offsets.s] Error 1 shuffle=2717580973
   make[3]: Target 'prepare' not remade because of errors.
   make[2]: *** [Makefile:1287: prepare0] Error 2 shuffle=2717580973
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:234: __sub-make] Error 2 shuffle=2717580973
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:234: __sub-make] Error 2 shuffle=2717580973
   make: Target 'prepare' not remade because of errors.


vim +121 arch/powerpc/include/asm/lppaca.h

3356bb9f7ba378a include/asm-powerpc/lppaca.h      David Gibson    2006-01-13  120  
d2e60075a3d4422 arch/powerpc/include/asm/lppaca.h Nicholas Piggin 2018-02-14 @121  #define lppaca_of(cpu)	(*paca_ptrs[cpu]->lppaca_ptr)
8154c5d22d91cd1 arch/powerpc/include/asm/lppaca.h Paul Mackerras  2010-08-12  122  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Shrikanth Hegde]

On 10/9/23 10:47 AM, Srikar Dronamraju wrote:

Hi Srikar. This is an interesting patch. 

> PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
s/whole/big 

Can we mention that a big core consist of two small cores. and w.r.t
linux a core is at small core. Hence there is mismatch. 
> CPU from a core that is preempted may have a larger latency. In
> such a scenario, its preferable to choose a different CPU to run.
> 
> If one of the CPUs in the core is active, i.e neither CEDED nor
> preempted, then consider this CPU as not preempted
> 
> Also if any of the CPUs in the core has yielded but OS has not requested
> CEDE or CONFER, then consider this CPU to be preempted.
> 

This is because an idle CPU cannot be preempted. Right?

This patch should help address the has_idle_core functionality and ttwu path 
in powerpc SPLPAR based on powerVM. Currently they are not correct.  

when the all the CPU's are idle, __update_idle_core will not set has_idle_core
 which is functionally not right. That is one example, there are other places where correct 
functionality of vcpu_is_preempted is crucial as well. 


> Cc: Ajay Kaher <akaher@vmware.com>
> Cc: Alexey Makhalov <amakhalov@vmware.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index e08513d73119..a980756f58df 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
>  	if (!is_shared_processor())
>  		return false;
> 
> +	if (!(yield_count_of(cpu) & 1))
> +		return false;
> +
> +	/*
> +	 * If CPU has yielded but OS has not requested idle then this CPU is

nit: can it be "if CPU is in hypervisor but OS has not requested ..." ?

> +	 * definitely preempted.
> +	 */
> +	if (!lppaca_of(cpu).idle)
> +		return true;
> +
>  #ifdef CONFIG_PPC_SPLPAR
>  	if (!is_kvm_guest()) {
> -		int first_cpu;
> +		int first_cpu, i;
> 
>  		/*
>  		 * The result of vcpu_is_preempted() is used in a
> @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
>  		 */
>  		if (cpu_first_thread_sibling(cpu) == first_cpu)
>  			return false;
> +
> +		/*
> +		 * If any of the threads of this core is not preempted or
> +		 * ceded, then consider this CPU to be non-preempted
> +		 */
> +		first_cpu = cpu_first_thread_sibling(cpu);
> +		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
> +			if (i == cpu)
> +				continue;
> +			if (!(yield_count_of(i) & 1))
> +				return false;
> +			if (!lppaca_of(i).idle)
> +				return true;
> +		}
>  	}
>  #endif
> 
> -	if (yield_count_of(cpu) & 1)
> -		return true;
> +	/*
> +	 * None of the threads in this thread group are running but none of
> +	 * them were preempted too. Hence assume the thread to be
> +	 * non-preempted.
> +	 */

That comment is bit confusing. instead of threads it would be better say CPUs

"None of the CPUs in this Big Core are running but none of them were preempted too. Hence assume the 
the CPU to be non-preempted."


>  	return false;
>  }
> 

Otherwise LGTM
Reviewed-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Shrikanth Hegde]

On 10/9/23 10:47 AM, Srikar Dronamraju wrote:

Hi Srikar. This is an interesting patch. 

> PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
s/whole/big 

Can we mention that a big core consist of two small cores. and w.r.t
linux a core is at small core. Hence there is mismatch. 
> CPU from a core that is preempted may have a larger latency. In
> such a scenario, its preferable to choose a different CPU to run.
> 
> If one of the CPUs in the core is active, i.e neither CEDED nor
> preempted, then consider this CPU as not preempted
> 
> Also if any of the CPUs in the core has yielded but OS has not requested
> CEDE or CONFER, then consider this CPU to be preempted.
> 

This is because an idle CPU cannot be preempted. Right?

This patch should help address the has_idle_core functionality and ttwu path 
in powerpc SPLPAR based on powerVM. Currently they are not correct.  

when the all the CPU's are idle, __update_idle_core will not set has_idle_core
 which is functionally not right. That is one example, there are other places where correct 
functionality of vcpu_is_preempted is crucial as well. 


> Cc: Ajay Kaher <akaher@vmware.com>
> Cc: Alexey Makhalov <amakhalov@vmware.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index e08513d73119..a980756f58df 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
>  	if (!is_shared_processor())
>  		return false;
> 
> +	if (!(yield_count_of(cpu) & 1))
> +		return false;
> +
> +	/*
> +	 * If CPU has yielded but OS has not requested idle then this CPU is

nit: can it be "if CPU is in hypervisor but OS has not requested ..." ?

> +	 * definitely preempted.
> +	 */
> +	if (!lppaca_of(cpu).idle)
> +		return true;
> +
>  #ifdef CONFIG_PPC_SPLPAR
>  	if (!is_kvm_guest()) {
> -		int first_cpu;
> +		int first_cpu, i;
> 
>  		/*
>  		 * The result of vcpu_is_preempted() is used in a
> @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
>  		 */
>  		if (cpu_first_thread_sibling(cpu) == first_cpu)
>  			return false;
> +
> +		/*
> +		 * If any of the threads of this core is not preempted or
> +		 * ceded, then consider this CPU to be non-preempted
> +		 */
> +		first_cpu = cpu_first_thread_sibling(cpu);
> +		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
> +			if (i == cpu)
> +				continue;
> +			if (!(yield_count_of(i) & 1))
> +				return false;
> +			if (!lppaca_of(i).idle)
> +				return true;
> +		}
>  	}
>  #endif
> 
> -	if (yield_count_of(cpu) & 1)
> -		return true;
> +	/*
> +	 * None of the threads in this thread group are running but none of
> +	 * them were preempted too. Hence assume the thread to be
> +	 * non-preempted.
> +	 */

That comment is bit confusing. instead of threads it would be better say CPUs

"None of the CPUs in this Big Core are running but none of them were preempted too. Hence assume the 
the CPU to be non-preempted."


>  	return false;
>  }
> 

Otherwise LGTM
Reviewed-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Srikar Dronamraju]

* Shrikanth Hegde <sshegde@linux.vnet.ibm.com> [2023-10-11 14:33:34]:
> On 10/9/23 10:47 AM, Srikar Dronamraju wrote:
> 
> Hi Srikar. This is an interesting patch. 
> 
> > PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
> s/whole/big 
> 
> Can we mention that a big core consist of two small cores. and w.r.t
> linux a core is at small core. Hence there is mismatch. 

PowerVM currently always schedules at a Big core granularity. And we would
want to transparent about it even if it changes.

> > CPU from a core that is preempted may have a larger latency. In
> > such a scenario, its preferable to choose a different CPU to run.
> > 
> > If one of the CPUs in the core is active, i.e neither CEDED nor
> > preempted, then consider this CPU as not preempted
> > 
> > Also if any of the CPUs in the core has yielded but OS has not requested
> > CEDE or CONFER, then consider this CPU to be preempted.
> > 
> 
> This is because an idle CPU cannot be preempted. Right?

If a CPU from the same SMT8 core has been preempted, we should consider this CPU
also has been preempted.

> 
> This patch should help address the has_idle_core functionality and ttwu path 
> in powerpc SPLPAR based on powerVM. Currently they are not correct.  
> 
> when the all the CPU's are idle, __update_idle_core will not set has_idle_core
>  which is functionally not right. That is one example, there are other places where correct 
> functionality of vcpu_is_preempted is crucial as well. 
> 

Right, its a crucial from a functionality perspective on shared LPARs.
The Dedicated ones dont have this issue.

> 
> > Cc: Ajay Kaher <akaher@vmware.com>
> > Cc: Alexey Makhalov <amakhalov@vmware.com>
> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> > index e08513d73119..a980756f58df 100644
> > --- a/arch/powerpc/include/asm/paravirt.h
> > +++ b/arch/powerpc/include/asm/paravirt.h
> > @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
> >  	if (!is_shared_processor())
> >  		return false;
> > 
> > +	if (!(yield_count_of(cpu) & 1))
> > +		return false;
> > +
> > +	/*
> > +	 * If CPU has yielded but OS has not requested idle then this CPU is
> 
> nit: can it be "if CPU is in hypervisor but OS has not requested ..." ?

Ok, will take it.

> 
> > +	 * definitely preempted.
> > +	 */
> > +	if (!lppaca_of(cpu).idle)
> > +		return true;
> > +
> >  #ifdef CONFIG_PPC_SPLPAR
> >  	if (!is_kvm_guest()) {
> > -		int first_cpu;
> > +		int first_cpu, i;
> > 
> >  		/*
> >  		 * The result of vcpu_is_preempted() is used in a
> > @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
> >  		 */
> >  		if (cpu_first_thread_sibling(cpu) == first_cpu)
> >  			return false;
> > +
> > +		/*
> > +		 * If any of the threads of this core is not preempted or
> > +		 * ceded, then consider this CPU to be non-preempted
> > +		 */
> > +		first_cpu = cpu_first_thread_sibling(cpu);
> > +		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
> > +			if (i == cpu)
> > +				continue;
> > +			if (!(yield_count_of(i) & 1))
> > +				return false;
> > +			if (!lppaca_of(i).idle)
> > +				return true;
> > +		}
> >  	}
> >  #endif
> > 
> > -	if (yield_count_of(cpu) & 1)
> > -		return true;
> > +	/*
> > +	 * None of the threads in this thread group are running but none of
> > +	 * them were preempted too. Hence assume the thread to be
> > +	 * non-preempted.
> > +	 */
> 
> That comment is bit confusing. instead of threads it would be better say CPUs
> 
> "None of the CPUs in this Big Core are running but none of them were preempted too. Hence assume the 
> the CPU to be non-preempted."
> 
> 
> >  	return false;
> >  }
> > 
> 
> Otherwise LGTM
> Reviewed-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>


Thanks Shrikanth.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Srikar Dronamraju]

* Shrikanth Hegde <sshegde@linux.vnet.ibm.com> [2023-10-11 14:33:34]:
> On 10/9/23 10:47 AM, Srikar Dronamraju wrote:
> 
> Hi Srikar. This is an interesting patch. 
> 
> > PowerVM Hypervisor dispatches on a whole core basis. In a shared LPAR, a
> s/whole/big 
> 
> Can we mention that a big core consist of two small cores. and w.r.t
> linux a core is at small core. Hence there is mismatch. 

PowerVM currently always schedules at a Big core granularity. And we would
want to transparent about it even if it changes.

> > CPU from a core that is preempted may have a larger latency. In
> > such a scenario, its preferable to choose a different CPU to run.
> > 
> > If one of the CPUs in the core is active, i.e neither CEDED nor
> > preempted, then consider this CPU as not preempted
> > 
> > Also if any of the CPUs in the core has yielded but OS has not requested
> > CEDE or CONFER, then consider this CPU to be preempted.
> > 
> 
> This is because an idle CPU cannot be preempted. Right?

If a CPU from the same SMT8 core has been preempted, we should consider this CPU
also has been preempted.

> 
> This patch should help address the has_idle_core functionality and ttwu path 
> in powerpc SPLPAR based on powerVM. Currently they are not correct.  
> 
> when the all the CPU's are idle, __update_idle_core will not set has_idle_core
>  which is functionally not right. That is one example, there are other places where correct 
> functionality of vcpu_is_preempted is crucial as well. 
> 

Right, its a crucial from a functionality perspective on shared LPARs.
The Dedicated ones dont have this issue.

> 
> > Cc: Ajay Kaher <akaher@vmware.com>
> > Cc: Alexey Makhalov <amakhalov@vmware.com>
> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> > Cc: Juergen Gross <jgross@suse.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Nicholas Piggin <npiggin@gmail.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> > index e08513d73119..a980756f58df 100644
> > --- a/arch/powerpc/include/asm/paravirt.h
> > +++ b/arch/powerpc/include/asm/paravirt.h
> > @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
> >  	if (!is_shared_processor())
> >  		return false;
> > 
> > +	if (!(yield_count_of(cpu) & 1))
> > +		return false;
> > +
> > +	/*
> > +	 * If CPU has yielded but OS has not requested idle then this CPU is
> 
> nit: can it be "if CPU is in hypervisor but OS has not requested ..." ?

Ok, will take it.

> 
> > +	 * definitely preempted.
> > +	 */
> > +	if (!lppaca_of(cpu).idle)
> > +		return true;
> > +
> >  #ifdef CONFIG_PPC_SPLPAR
> >  	if (!is_kvm_guest()) {
> > -		int first_cpu;
> > +		int first_cpu, i;
> > 
> >  		/*
> >  		 * The result of vcpu_is_preempted() is used in a
> > @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
> >  		 */
> >  		if (cpu_first_thread_sibling(cpu) == first_cpu)
> >  			return false;
> > +
> > +		/*
> > +		 * If any of the threads of this core is not preempted or
> > +		 * ceded, then consider this CPU to be non-preempted
> > +		 */
> > +		first_cpu = cpu_first_thread_sibling(cpu);
> > +		for (i = first_cpu; i < first_cpu + threads_per_core; i++) {
> > +			if (i == cpu)
> > +				continue;
> > +			if (!(yield_count_of(i) & 1))
> > +				return false;
> > +			if (!lppaca_of(i).idle)
> > +				return true;
> > +		}
> >  	}
> >  #endif
> > 
> > -	if (yield_count_of(cpu) & 1)
> > -		return true;
> > +	/*
> > +	 * None of the threads in this thread group are running but none of
> > +	 * them were preempted too. Hence assume the thread to be
> > +	 * non-preempted.
> > +	 */
> 
> That comment is bit confusing. instead of threads it would be better say CPUs
> 
> "None of the CPUs in this Big Core are running but none of them were preempted too. Hence assume the 
> the CPU to be non-preempted."
> 
> 
> >  	return false;
> >  }
> > 
> 
> Otherwise LGTM
> Reviewed-by: Shrikanth Hegde <sshegde@linux.vnet.ibm.com>


Thanks Shrikanth.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Aboorva Devarajan]

On Mon, 2023-10-09 at 10:47 +0530, Srikar Dronamraju wrote:

Hi Srikar,

Benchmarked this patch on baremetal POWER9 node by launching KVM to
observe the improvements achieved in KVM with the patched kernel.
Below, you can find the schbench latency result comparision.

System was running on SMT4 mode with the below configuration:

Setup:

$ lscpu
Architecture:        ppc64le
Byte Order:          Little Endian
CPU(s):              128
On-line CPU(s) list: 0-127
Thread(s) per core:  4
Core(s) per socket:  16
Socket(s):           2
NUMA node(s):        8
Model:               2.3 (pvr 004e 1203)
Model name:          POWER9, altivec supported
CPU max MHz:         3800.0000
CPU min MHz:         2300.0000
L1d cache:           32K
L1i cache:           32K
L2 cache:            512K
L3 cache:            10240K
NUMA node0 CPU(s):   0-63
NUMA node8 CPU(s):   64-127
NUMA node250 CPU(s):
NUMA node251 CPU(s):
NUMA node252 CPU(s):
NUMA node253 CPU(s):
NUMA node254 CPU(s):
NUMA node255 CPU(s):

- Baseline kernel  : v6.6.0-rc5
- Patched kernel   : v6.6.0-rc5 with patchset
- schbench version : upto commit 2eef44 (schbench: record the 
  execution time in the matrix multiplication mode)


Results:
--------

These results shows the schbench latency on a patched kernel compared
to a baseline kernel on KVM. The numbers in the "compare%" column
represent the percentage difference between the latency measured on the
baseline kernel and the patched kernel. A negative percentage means the
patched kernel performs less optimially (higher latency) than the
baseline, while a positive percentage means it performs better (lower
latency).


Scenarios:
----------


Case 1: No Noise

Host: Idle
KVM 1: Launched a KVM affined to 0-39 CPUs (40 CPUs)
KVM 1 (Workload) : ./schbench -m 20 -t 2 -r 30 (benchmark)

schbench latency (niter: 20)

percentile      compare% (avg)
                (higher the better)

50.0th:	        -4.84
75.0th:	        -8.09
90.0th:	        -3.39
95.0th:	        +5.16
99.0th:	        +90.78
99.5th:	        +36.34
99.9th:	        +8.31

--------------------------------------------------------

Case 2: With Noise: Over-commit case: Multiple KVM guests sharing the
same set of CPUs

Two KVM instances are launched, where one being benchmarked, and the
other executing a workload to introduce noise.

KVM 1: Launched a KVM affined to 0-39 CPUs (40 CPUs)
KVM 1 (Workload) : ./schbench -m 20 -t 2 -r 30 (benchmark)

KVM 2 (Noise): Launched a KVM affined to 0-39 CPUs 

schbench latency (niter: 20)

percentile      compare% (avg) 
                (higher the better)

50.0th:	        -1.47
75.0th:         -5.72
90.0th:	        +7.88
95.0th:         +10.71
99.0th:	        +512.08
99.5th:	        +380.61
99.9th:	        +90.39

--------------------------------------------------------

Case 3: Overlap case: Multiple KVM guests sharing a subset of CPUs.

Two KVM instances are launched, where one being benchmarked, and the
other executing a workload to introduce noise.

KVM 1: Launched a KVM affined to 0-39 CPUs (40 CPUs)
KVM 1 (Workload) : ./schbench -m 20 -t 2 -r 30 (benchmark)

KVM 2 (Noise): Launched a KVM affined to 0-19 CPUs

schbench latency (niter: 20)

percentile      compare% (avg)
                (higher the better)

50.0th:	        -1.63
75.0th:	        -2.78
90.0th:         +57.62
95.0th:	        +87.90
99.0th:	        +343.66
99.5th:	        +178.01
99.9th:	        +36.07

--------------------------------------------------------

The above results demonstrate the effectiveness of the proposed
approach, which utilizes the idle-hint in lppaca to detect the
preempted vCPU more efficiently. This approach is beneficial for
improving schbench latency on KVM, particularly the tail latencies.

Thanks,
Aboorva

> PowerVM Hypervisor dispatches on a whole core basis. In a shared
> LPAR, a
> CPU from a core that is preempted may have a larger latency. In
> such a scenario, its preferable to choose a different CPU to run.
> 
> If one of the CPUs in the core is active, i.e neither CEDED nor
> preempted, then consider this CPU as not preempted.
> 
> Also if any of the CPUs in the core has yielded but OS has not
> requested
> CEDE or CONFER, then consider this CPU to be preempted.
> 
> Cc: Ajay Kaher <akaher@vmware.com>
> Cc: Alexey Makhalov <amakhalov@vmware.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++-
> --
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h
> b/arch/powerpc/include/asm/paravirt.h
> index e08513d73119..a980756f58df 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
>  	if (!is_shared_processor())
>  		return false;
>  
> +	if (!(yield_count_of(cpu) & 1))
> +		return false;
> +
> +	/*
> +	 * If CPU has yielded but OS has not requested idle then this
> CPU is
> +	 * definitely preempted.
> +	 */
> +	if (!lppaca_of(cpu).idle)
> +		return true;
> +
>  #ifdef CONFIG_PPC_SPLPAR
>  	if (!is_kvm_guest()) {
> -		int first_cpu;
> +		int first_cpu, i;
>  
>  		/*
>  		 * The result of vcpu_is_preempted() is used in a
> @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
>  		 */
>  		if (cpu_first_thread_sibling(cpu) == first_cpu)
>  			return false;
> +
> +		/*
> +		 * If any of the threads of this core is not preempted
> or
> +		 * ceded, then consider this CPU to be non-preempted
> +		 */
> +		first_cpu = cpu_first_thread_sibling(cpu);
> +		for (i = first_cpu; i < first_cpu + threads_per_core;
> i++) {
> +			if (i == cpu)
> +				continue;
> +			if (!(yield_count_of(i) & 1))
> +				return false;
> +			if (!lppaca_of(i).idle)
> +				return true;
> +		}
>  	}
>  #endif
>  
> -	if (yield_count_of(cpu) & 1)
> -		return true;
> +	/*
> +	 * None of the threads in this thread group are running but
> none of
> +	 * them were preempted too. Hence assume the thread to be
> +	 * non-preempted.
> +	 */
>  	return false;
>  }
>  

Re: [PATCH] powerpc/paravirt: Improve vcpu_is_preempted

[Aboorva Devarajan]

On Mon, 2023-10-09 at 10:47 +0530, Srikar Dronamraju wrote:

Hi Srikar,

Benchmarked this patch on baremetal POWER9 node by launching KVM to
observe the improvements achieved in KVM with the patched kernel.
Below, you can find the schbench latency result comparision.

System was running on SMT4 mode with the below configuration:

Setup:

$ lscpu
Architecture:        ppc64le
Byte Order:          Little Endian
CPU(s):              128
On-line CPU(s) list: 0-127
Thread(s) per core:  4
Core(s) per socket:  16
Socket(s):           2
NUMA node(s):        8
Model:               2.3 (pvr 004e 1203)
Model name:          POWER9, altivec supported
CPU max MHz:         3800.0000
CPU min MHz:         2300.0000
L1d cache:           32K
L1i cache:           32K
L2 cache:            512K
L3 cache:            10240K
NUMA node0 CPU(s):   0-63
NUMA node8 CPU(s):   64-127
NUMA node250 CPU(s):
NUMA node251 CPU(s):
NUMA node252 CPU(s):
NUMA node253 CPU(s):
NUMA node254 CPU(s):
NUMA node255 CPU(s):

- Baseline kernel  : v6.6.0-rc5
- Patched kernel   : v6.6.0-rc5 with patchset
- schbench version : upto commit 2eef44 (schbench: record the 
  execution time in the matrix multiplication mode)


Results:
--------

These results shows the schbench latency on a patched kernel compared
to a baseline kernel on KVM. The numbers in the "compare%" column
represent the percentage difference between the latency measured on the
baseline kernel and the patched kernel. A negative percentage means the
patched kernel performs less optimially (higher latency) than the
baseline, while a positive percentage means it performs better (lower
latency).


Scenarios:
----------


Case 1: No Noise

Host: Idle
KVM 1: Launched a KVM affined to 0-39 CPUs (40 CPUs)
KVM 1 (Workload) : ./schbench -m 20 -t 2 -r 30 (benchmark)

schbench latency (niter: 20)

percentile      compare% (avg)
                (higher the better)

50.0th:	        -4.84
75.0th:	        -8.09
90.0th:	        -3.39
95.0th:	        +5.16
99.0th:	        +90.78
99.5th:	        +36.34
99.9th:	        +8.31

--------------------------------------------------------

Case 2: With Noise: Over-commit case: Multiple KVM guests sharing the
same set of CPUs

Two KVM instances are launched, where one being benchmarked, and the
other executing a workload to introduce noise.

KVM 1: Launched a KVM affined to 0-39 CPUs (40 CPUs)
KVM 1 (Workload) : ./schbench -m 20 -t 2 -r 30 (benchmark)

KVM 2 (Noise): Launched a KVM affined to 0-39 CPUs 

schbench latency (niter: 20)

percentile      compare% (avg) 
                (higher the better)

50.0th:	        -1.47
75.0th:         -5.72
90.0th:	        +7.88
95.0th:         +10.71
99.0th:	        +512.08
99.5th:	        +380.61
99.9th:	        +90.39

--------------------------------------------------------

Case 3: Overlap case: Multiple KVM guests sharing a subset of CPUs.

Two KVM instances are launched, where one being benchmarked, and the
other executing a workload to introduce noise.

KVM 1: Launched a KVM affined to 0-39 CPUs (40 CPUs)
KVM 1 (Workload) : ./schbench -m 20 -t 2 -r 30 (benchmark)

KVM 2 (Noise): Launched a KVM affined to 0-19 CPUs

schbench latency (niter: 20)

percentile      compare% (avg)
                (higher the better)

50.0th:	        -1.63
75.0th:	        -2.78
90.0th:         +57.62
95.0th:	        +87.90
99.0th:	        +343.66
99.5th:	        +178.01
99.9th:	        +36.07

--------------------------------------------------------

The above results demonstrate the effectiveness of the proposed
approach, which utilizes the idle-hint in lppaca to detect the
preempted vCPU more efficiently. This approach is beneficial for
improving schbench latency on KVM, particularly the tail latencies.

Thanks,
Aboorva

> PowerVM Hypervisor dispatches on a whole core basis. In a shared
> LPAR, a
> CPU from a core that is preempted may have a larger latency. In
> such a scenario, its preferable to choose a different CPU to run.
> 
> If one of the CPUs in the core is active, i.e neither CEDED nor
> preempted, then consider this CPU as not preempted.
> 
> Also if any of the CPUs in the core has yielded but OS has not
> requested
> CEDE or CONFER, then consider this CPU to be preempted.
> 
> Cc: Ajay Kaher <akaher@vmware.com>
> Cc: Alexey Makhalov <amakhalov@vmware.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++++++-
> --
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h
> b/arch/powerpc/include/asm/paravirt.h
> index e08513d73119..a980756f58df 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -121,9 +121,19 @@ static inline bool vcpu_is_preempted(int cpu)
>  	if (!is_shared_processor())
>  		return false;
>  
> +	if (!(yield_count_of(cpu) & 1))
> +		return false;
> +
> +	/*
> +	 * If CPU has yielded but OS has not requested idle then this
> CPU is
> +	 * definitely preempted.
> +	 */
> +	if (!lppaca_of(cpu).idle)
> +		return true;
> +
>  #ifdef CONFIG_PPC_SPLPAR
>  	if (!is_kvm_guest()) {
> -		int first_cpu;
> +		int first_cpu, i;
>  
>  		/*
>  		 * The result of vcpu_is_preempted() is used in a
> @@ -149,11 +159,28 @@ static inline bool vcpu_is_preempted(int cpu)
>  		 */
>  		if (cpu_first_thread_sibling(cpu) == first_cpu)
>  			return false;
> +
> +		/*
> +		 * If any of the threads of this core is not preempted
> or
> +		 * ceded, then consider this CPU to be non-preempted
> +		 */
> +		first_cpu = cpu_first_thread_sibling(cpu);
> +		for (i = first_cpu; i < first_cpu + threads_per_core;
> i++) {
> +			if (i == cpu)
> +				continue;
> +			if (!(yield_count_of(i) & 1))
> +				return false;
> +			if (!lppaca_of(i).idle)
> +				return true;
> +		}
>  	}
>  #endif
>  
> -	if (yield_count_of(cpu) & 1)
> -		return true;
> +	/*
> +	 * None of the threads in this thread group are running but
> none of
> +	 * them were preempted too. Hence assume the thread to be
> +	 * non-preempted.
> +	 */
>  	return false;
>  }
>