[PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Aneesh Kumar K.V]

No functional change in this patch. A helper is added to find if
vcpu is dispatched by hypervisor. Use that instead of opencoding.
Also clarify some of the comments.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
 arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index ac4279208d63..b78b82d66057 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -76,6 +76,17 @@ static inline bool is_vcpu_idle(int vcpu)
 {
 	return lppaca_of(vcpu).idle;
 }
+
+static inline bool vcpu_is_dispatched(int vcpu)
+{
+	/*
+	 * This is the yield_count.  An "odd" value (low bit on) means that
+	 * the processor is yielded (either because of an OS yield or a
+	 * hypervisor preempt).  An even value implies that the processor is
+	 * currently executing.
+	 */
+	return (!(yield_count_of(vcpu) & 1));
+}
 #else
 static inline bool is_shared_processor(void)
 {
@@ -109,6 +120,10 @@ static inline bool is_vcpu_idle(int vcpu)
 {
 	return false;
 }
+static inline bool vcpu_is_dispatched(int vcpu)
+{
+	return true;
+}
 #endif
 
 #define vcpu_is_preempted vcpu_is_preempted
@@ -134,12 +149,12 @@ static inline bool vcpu_is_preempted(int cpu)
 	 * If the hypervisor has dispatched the target CPU on a physical
 	 * processor, then the target CPU is definitely not preempted.
 	 */
-	if (!(yield_count_of(cpu) & 1))
+	if (vcpu_is_dispatched(cpu))
 		return false;
 
 	/*
-	 * If the target CPU has yielded to Hypervisor but OS has not
-	 * requested idle then the target CPU is definitely preempted.
+	 * if the target CPU is not dispatched and the guest OS
+	 * has not marked the CPU idle, then it is hypervisor preempted.
 	 */
 	if (!is_vcpu_idle(cpu))
 		return true;
@@ -166,7 +181,7 @@ static inline bool vcpu_is_preempted(int cpu)
 
 		/*
 		 * The PowerVM hypervisor dispatches VMs on a whole core
-		 * basis. So we know that a thread sibling of the local CPU
+		 * basis. So we know that a thread sibling of the executing CPU
 		 * cannot have been preempted by the hypervisor, even if it
 		 * has called H_CONFER, which will set the yield bit.
 		 */
@@ -174,15 +189,17 @@ static inline bool vcpu_is_preempted(int cpu)
 			return false;
 
 		/*
-		 * If any of the threads of the target CPU's core are not
-		 * preempted or ceded, then consider target CPU to be
-		 * non-preempted.
+		 * The specific target CPU was marked by guest OS as idle, but
+		 * then also check all other cpus in the core for PowerVM
+		 * because it does core scheduling and one of the vcpu
+		 * of the core getting preempted by hypervisor implies
+		 * other vcpus can also be considered 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))
+			if (vcpu_is_dispatched(i))
 				return false;
 			if (!is_vcpu_idle(i))
 				return true;
-- 
2.41.0

Re: [PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Shrikanth Hegde]

On 11/14/23 12:42 PM, Aneesh Kumar K.V wrote:
> No functional change in this patch. A helper is added to find if
> vcpu is dispatched by hypervisor. Use that instead of opencoding.
> Also clarify some of the comments.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index ac4279208d63..b78b82d66057 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -76,6 +76,17 @@ static inline bool is_vcpu_idle(int vcpu)
>  {
>  	return lppaca_of(vcpu).idle;
>  }
> +
> +static inline bool vcpu_is_dispatched(int vcpu)
> +{
> +	/*
> +	 * This is the yield_count.  An "odd" value (low bit on) means that
> +	 * the processor is yielded (either because of an OS yield or a
> +	 * hypervisor preempt).  An even value implies that the processor is
> +	 * currently executing.
> +	 */
> +	return (!(yield_count_of(vcpu) & 1));
> +}
>  #else
>  static inline bool is_shared_processor(void)
>  {
> @@ -109,6 +120,10 @@ static inline bool is_vcpu_idle(int vcpu)
>  {
>  	return false;
>  }
> +static inline bool vcpu_is_dispatched(int vcpu)
> +{
> +	return true;
> +}
>  #endif
> 

Similar code can be changed in lib/qspinlock.c and lib/locks.c as well. 


>  #define vcpu_is_preempted vcpu_is_preempted
> @@ -134,12 +149,12 @@ static inline bool vcpu_is_preempted(int cpu)
>  	 * If the hypervisor has dispatched the target CPU on a physical
>  	 * processor, then the target CPU is definitely not preempted.
>  	 */
> -	if (!(yield_count_of(cpu) & 1))
> +	if (vcpu_is_dispatched(cpu))
>  		return false;
> 
>  	/*
> -	 * If the target CPU has yielded to Hypervisor but OS has not
> -	 * requested idle then the target CPU is definitely preempted.
> +	 * if the target CPU is not dispatched and the guest OS
> +	 * has not marked the CPU idle, then it is hypervisor preempted.
>  	 */
>  	if (!is_vcpu_idle(cpu))
>  		return true;
> @@ -166,7 +181,7 @@ static inline bool vcpu_is_preempted(int cpu)
> 
>  		/*
>  		 * The PowerVM hypervisor dispatches VMs on a whole core
> -		 * basis. So we know that a thread sibling of the local CPU
> +		 * basis. So we know that a thread sibling of the executing CPU
>  		 * cannot have been preempted by the hypervisor, even if it
>  		 * has called H_CONFER, which will set the yield bit.
>  		 */
> @@ -174,15 +189,17 @@ static inline bool vcpu_is_preempted(int cpu)
>  			return false;
> 
>  		/*
> -		 * If any of the threads of the target CPU's core are not
> -		 * preempted or ceded, then consider target CPU to be
> -		 * non-preempted.
> +		 * The specific target CPU was marked by guest OS as idle, but
> +		 * then also check all other cpus in the core for PowerVM
> +		 * because it does core scheduling and one of the vcpu
> +		 * of the core getting preempted by hypervisor implies
> +		 * other vcpus can also be considered 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))
> +			if (vcpu_is_dispatched(i))
>  				return false;
>  			if (!is_vcpu_idle(i))
>  				return true;

Re: [PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Aneesh Kumar K V]

On 11/14/23 2:53 PM, Shrikanth Hegde wrote:
> 
> 
> On 11/14/23 12:42 PM, Aneesh Kumar K.V wrote:
>> No functional change in this patch. A helper is added to find if
>> vcpu is dispatched by hypervisor. Use that instead of opencoding.
>> Also clarify some of the comments.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
>> index ac4279208d63..b78b82d66057 100644
>> --- a/arch/powerpc/include/asm/paravirt.h
>> +++ b/arch/powerpc/include/asm/paravirt.h
>> @@ -76,6 +76,17 @@ static inline bool is_vcpu_idle(int vcpu)
>>  {
>>  	return lppaca_of(vcpu).idle;
>>  }
>> +
>> +static inline bool vcpu_is_dispatched(int vcpu)
>> +{
>> +	/*
>> +	 * This is the yield_count.  An "odd" value (low bit on) means that
>> +	 * the processor is yielded (either because of an OS yield or a
>> +	 * hypervisor preempt).  An even value implies that the processor is
>> +	 * currently executing.
>> +	 */
>> +	return (!(yield_count_of(vcpu) & 1));
>> +}
>>  #else
>>  static inline bool is_shared_processor(void)
>>  {
>> @@ -109,6 +120,10 @@ static inline bool is_vcpu_idle(int vcpu)
>>  {
>>  	return false;
>>  }
>> +static inline bool vcpu_is_dispatched(int vcpu)
>> +{
>> +	return true;
>> +}
>>  #endif
>>
> 
> Similar code can be changed in lib/qspinlock.c and lib/locks.c as well. 

I avoided doing that because they used the fetched yield_count value

yield_to_preempted(owner, yield_count);


and the checking comes with a comment

	if ((yield_count & 1) == 0)
		goto relax; /* owner vcpu is running */


-aneesh

Re: [PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Srikar Dronamraju]

* Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> [2023-11-14 12:42:19]:

> No functional change in this patch. A helper is added to find if
> vcpu is dispatched by hypervisor. Use that instead of opencoding.
> Also clarify some of the comments.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index ac4279208d63..b78b82d66057 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -76,6 +76,17 @@ static inline bool is_vcpu_idle(int vcpu)
>  {
>  	return lppaca_of(vcpu).idle;
>  }
> +
> +static inline bool vcpu_is_dispatched(int vcpu)
> +{
> +	/*
> +	 * This is the yield_count.  An "odd" value (low bit on) means that
> +	 * the processor is yielded (either because of an OS yield or a
> +	 * hypervisor preempt).  An even value implies that the processor is
> +	 * currently executing.
> +	 */
> +	return (!(yield_count_of(vcpu) & 1));
> +}
>  #else
>  static inline bool is_shared_processor(void)
>  {
> @@ -109,6 +120,10 @@ static inline bool is_vcpu_idle(int vcpu)
>  {
>  	return false;
>  }
> +static inline bool vcpu_is_dispatched(int vcpu)
> +{
> +	return true;
> +}
>  #endif

If we are introducing vcpu_is_dispatched, we should remove 
yield_count_of() and use vcpu_is_dispatched everwhere

No point in having yield_count_of() and vcpu_is_dispatched, since
yield_count_of() is only used to check if we are running in OS or not.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Aneesh Kumar K V]

On 11/14/23 3:16 PM, Srikar Dronamraju wrote:
> * Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> [2023-11-14 12:42:19]:
> 
>> No functional change in this patch. A helper is added to find if
>> vcpu is dispatched by hypervisor. Use that instead of opencoding.
>> Also clarify some of the comments.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>  arch/powerpc/include/asm/paravirt.h | 33 ++++++++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
>> index ac4279208d63..b78b82d66057 100644
>> --- a/arch/powerpc/include/asm/paravirt.h
>> +++ b/arch/powerpc/include/asm/paravirt.h
>> @@ -76,6 +76,17 @@ static inline bool is_vcpu_idle(int vcpu)
>>  {
>>  	return lppaca_of(vcpu).idle;
>>  }
>> +
>> +static inline bool vcpu_is_dispatched(int vcpu)
>> +{
>> +	/*
>> +	 * This is the yield_count.  An "odd" value (low bit on) means that
>> +	 * the processor is yielded (either because of an OS yield or a
>> +	 * hypervisor preempt).  An even value implies that the processor is
>> +	 * currently executing.
>> +	 */
>> +	return (!(yield_count_of(vcpu) & 1));
>> +}
>>  #else
>>  static inline bool is_shared_processor(void)
>>  {
>> @@ -109,6 +120,10 @@ static inline bool is_vcpu_idle(int vcpu)
>>  {
>>  	return false;
>>  }
>> +static inline bool vcpu_is_dispatched(int vcpu)
>> +{
>> +	return true;
>> +}
>>  #endif
> 
> If we are introducing vcpu_is_dispatched, we should remove 
> yield_count_of() and use vcpu_is_dispatched everwhere
> 
> No point in having yield_count_of() and vcpu_is_dispatched, since
> yield_count_of() is only used to check if we are running in OS or not.
> 

We do

yield_count = yield_count_of(owner);
yield_to_preempted(owner, yield_count);

-aneesh

Re: [PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Srikar Dronamraju]

* Aneesh Kumar K V <aneesh.kumar@linux.ibm.com> [2023-11-14 15:45:35]:

> On 11/14/23 3:16 PM, Srikar Dronamraju wrote:
> > * Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> [2023-11-14 12:42:19]:
> > 
> >> No functional change in this patch. A helper is added to find if
> >> vcpu is dispatched by hypervisor. Use that instead of opencoding.
> >> Also clarify some of the comments.
> >>
> > 
> > If we are introducing vcpu_is_dispatched, we should remove 
> > yield_count_of() and use vcpu_is_dispatched everwhere
> > 
> > No point in having yield_count_of() and vcpu_is_dispatched, since
> > yield_count_of() is only used to check if we are running in OS or not.
> > 
> 
> We do
> 
> yield_count = yield_count_of(owner);
> yield_to_preempted(owner, yield_count);

yield_to_preempted is defined just below yield_count_of() and we are anyway
passing the CPU, so we dont have to pass yield_count to yield_to_preempted


-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH] powerpc/sched: Cleanup vcpu_is_preempted()

[Michael Ellerman]

On Tue, 14 Nov 2023 12:42:19 +0530, Aneesh Kumar K.V wrote:
> No functional change in this patch. A helper is added to find if
> vcpu is dispatched by hypervisor. Use that instead of opencoding.
> Also clarify some of the comments.
> 
> 

Applied to powerpc/next.

[1/1] powerpc/sched: Cleanup vcpu_is_preempted()
      https://git.kernel.org/powerpc/c/6f4b7052daa060e7d20d6d599697b8ac702a7e69

cheers