| Message ID | 20231114071219.198222-1-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6f4b7052daa060e7d20d6d599697b8ac702a7e69 |
| Headers | show |
| Series | powerpc/sched: Cleanup vcpu_is_preempted() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
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;
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
* 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.
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
* 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
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
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;
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(-)