[PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks

[PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks

[Nathan Lynch]

Minor changes arising from discovering that this code throws warnings with
DEBUG_PREEMPT kernels.

Changes since v1:
* Additional commentary to (1) distinguish hypervisor dispatch and preempt
  behavior from kernel scheduler preemption; and (2) more clearly justify
  the use of raw_smp_processor_id().
* Additional patch to update existing comments before making the functional
  change.

v1: https://lore.kernel.org/linuxppc-dev/20210921031213.2029824-1-nathanl@linux.ibm.com/

Nathan Lynch (2):
  powerpc/paravirt: vcpu_is_preempted() commentary
  powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()

 arch/powerpc/include/asm/paravirt.h | 40 +++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary

[Nathan Lynch]

Add comments more clearly documenting that this function determines whether
hypervisor-level preemption of the VM has occurred.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
---
 arch/powerpc/include/asm/paravirt.h | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index bcb7b5f917be..39f173961f6a 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -21,7 +21,7 @@ static inline bool is_shared_processor(void)
 	return static_branch_unlikely(&shared_processor);
 }
 
-/* If bit 0 is set, the cpu has been preempted */
+/* If bit 0 is set, the cpu has been ceded, conferred, or preempted */
 static inline u32 yield_count_of(int cpu)
 {
 	__be32 yield_count = READ_ONCE(lppaca_of(cpu).yield_count);
@@ -92,6 +92,19 @@ static inline void prod_cpu(int cpu)
 #define vcpu_is_preempted vcpu_is_preempted
 static inline bool vcpu_is_preempted(int cpu)
 {
+	/*
+	 * The dispatch/yield bit alone is an imperfect indicator of
+	 * whether the hypervisor has dispatched @cpu to run on a physical
+	 * processor. When it is clear, @cpu is definitely not preempted.
+	 * But when it is set, it means only that it *might* be, subject to
+	 * other conditions. So we check other properties of the VM and
+	 * @cpu first, resorting to the yield count last.
+	 */
+
+	/*
+	 * Hypervisor preemption isn't possible in dedicated processor
+	 * mode by definition.
+	 */
 	if (!is_shared_processor())
 		return false;
 
@@ -100,9 +113,10 @@ static inline bool vcpu_is_preempted(int cpu)
 		int first_cpu = cpu_first_thread_sibling(smp_processor_id());
 
 		/*
-		 * Preemption can only happen at core granularity. This CPU
-		 * is not preempted if one of the CPU of this core is not
-		 * preempted.
+		 * The PowerVM hypervisor dispatches VMs on a whole core
+		 * basis. So we know that a thread sibling of the local CPU
+		 * cannot have been preempted by the hypervisor, even if it
+		 * has called H_CONFER, which will set the yield bit.
 		 */
 		if (cpu_first_thread_sibling(cpu) == first_cpu)
 			return false;
-- 
2.31.1

Re: [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary

[Srikar Dronamraju]

* Nathan Lynch <nathanl@linux.ibm.com> [2021-09-28 16:41:46]:

> Add comments more clearly documenting that this function determines whether
> hypervisor-level preemption of the VM has occurred.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>

Looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/powerpc/include/asm/paravirt.h | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index bcb7b5f917be..39f173961f6a 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -21,7 +21,7 @@ static inline bool is_shared_processor(void)
>  	return static_branch_unlikely(&shared_processor);
>  }
> 
> -/* If bit 0 is set, the cpu has been preempted */
> +/* If bit 0 is set, the cpu has been ceded, conferred, or preempted */
>  static inline u32 yield_count_of(int cpu)
>  {
>  	__be32 yield_count = READ_ONCE(lppaca_of(cpu).yield_count);
> @@ -92,6 +92,19 @@ static inline void prod_cpu(int cpu)
>  #define vcpu_is_preempted vcpu_is_preempted
>  static inline bool vcpu_is_preempted(int cpu)
>  {
> +	/*
> +	 * The dispatch/yield bit alone is an imperfect indicator of
> +	 * whether the hypervisor has dispatched @cpu to run on a physical
> +	 * processor. When it is clear, @cpu is definitely not preempted.
> +	 * But when it is set, it means only that it *might* be, subject to
> +	 * other conditions. So we check other properties of the VM and
> +	 * @cpu first, resorting to the yield count last.
> +	 */
> +
> +	/*
> +	 * Hypervisor preemption isn't possible in dedicated processor
> +	 * mode by definition.
> +	 */
>  	if (!is_shared_processor())
>  		return false;
> 
> @@ -100,9 +113,10 @@ static inline bool vcpu_is_preempted(int cpu)
>  		int first_cpu = cpu_first_thread_sibling(smp_processor_id());
> 
>  		/*
> -		 * Preemption can only happen at core granularity. This CPU
> -		 * is not preempted if one of the CPU of this core is not
> -		 * preempted.
> +		 * The PowerVM hypervisor dispatches VMs on a whole core
> +		 * basis. So we know that a thread sibling of the local CPU
> +		 * cannot have been preempted by the hypervisor, even if it
> +		 * has called H_CONFER, which will set the yield bit.
>  		 */
>  		if (cpu_first_thread_sibling(cpu) == first_cpu)
>  			return false;
> -- 
> 2.31.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

[PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()

[Nathan Lynch]

vcpu_is_preempted() can be used outside of preempt-disabled critical
sections, yielding warnings such as:

BUG: using smp_processor_id() in preemptible [00000000] code: systemd-udevd/185
caller is rwsem_spin_on_owner+0x1cc/0x2d0
CPU: 1 PID: 185 Comm: systemd-udevd Not tainted 5.15.0-rc2+ #33
Call Trace:
[c000000012907ac0] [c000000000aa30a8] dump_stack_lvl+0xac/0x108 (unreliable)
[c000000012907b00] [c000000001371f70] check_preemption_disabled+0x150/0x160
[c000000012907b90] [c0000000001e0e8c] rwsem_spin_on_owner+0x1cc/0x2d0
[c000000012907be0] [c0000000001e1408] rwsem_down_write_slowpath+0x478/0x9a0
[c000000012907ca0] [c000000000576cf4] filename_create+0x94/0x1e0
[c000000012907d10] [c00000000057ac08] do_symlinkat+0x68/0x1a0
[c000000012907d70] [c00000000057ae18] sys_symlink+0x58/0x70
[c000000012907da0] [c00000000002e448] system_call_exception+0x198/0x3c0
[c000000012907e10] [c00000000000c54c] system_call_common+0xec/0x250

The result of vcpu_is_preempted() is always used speculatively, and the
function does not access per-cpu resources in a (Linux) preempt-unsafe way.
Use raw_smp_processor_id() to avoid such warnings, adding explanatory
comments.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
Fixes: ca3f969dcb11 ("powerpc/paravirt: Use is_kvm_guest() in vcpu_is_preempted()")
---
 arch/powerpc/include/asm/paravirt.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index 39f173961f6a..eb7df559ae74 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -110,7 +110,23 @@ static inline bool vcpu_is_preempted(int cpu)
 
 #ifdef CONFIG_PPC_SPLPAR
 	if (!is_kvm_guest()) {
-		int first_cpu = cpu_first_thread_sibling(smp_processor_id());
+		int first_cpu;
+
+		/*
+		 * The result of vcpu_is_preempted() is used in a
+		 * speculative way, and is always subject to invalidation
+		 * by events internal and external to Linux. While we can
+		 * be called in preemptable context (in the Linux sense),
+		 * we're not accessing per-cpu resources in a way that can
+		 * race destructively with Linux scheduler preemption and
+		 * migration, and callers can tolerate the potential for
+		 * error introduced by sampling the CPU index without
+		 * pinning the task to it. So it is permissible to use
+		 * raw_smp_processor_id() here to defeat the preempt debug
+		 * warnings that can arise from using smp_processor_id()
+		 * in arbitrary contexts.
+		 */
+		first_cpu = cpu_first_thread_sibling(raw_smp_processor_id());
 
 		/*
 		 * The PowerVM hypervisor dispatches VMs on a whole core
-- 
2.31.1

Re: [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()

[Srikar Dronamraju]

* Nathan Lynch <nathanl@linux.ibm.com> [2021-09-28 16:41:47]:

> vcpu_is_preempted() can be used outside of preempt-disabled critical
> sections, yielding warnings such as:
> 
> BUG: using smp_processor_id() in preemptible [00000000] code: systemd-udevd/185
> caller is rwsem_spin_on_owner+0x1cc/0x2d0
> CPU: 1 PID: 185 Comm: systemd-udevd Not tainted 5.15.0-rc2+ #33
> Call Trace:
> [c000000012907ac0] [c000000000aa30a8] dump_stack_lvl+0xac/0x108 (unreliable)
> [c000000012907b00] [c000000001371f70] check_preemption_disabled+0x150/0x160
> [c000000012907b90] [c0000000001e0e8c] rwsem_spin_on_owner+0x1cc/0x2d0
> [c000000012907be0] [c0000000001e1408] rwsem_down_write_slowpath+0x478/0x9a0
> [c000000012907ca0] [c000000000576cf4] filename_create+0x94/0x1e0
> [c000000012907d10] [c00000000057ac08] do_symlinkat+0x68/0x1a0
> [c000000012907d70] [c00000000057ae18] sys_symlink+0x58/0x70
> [c000000012907da0] [c00000000002e448] system_call_exception+0x198/0x3c0
> [c000000012907e10] [c00000000000c54c] system_call_common+0xec/0x250
> 
> The result of vcpu_is_preempted() is always used speculatively, and the
> function does not access per-cpu resources in a (Linux) preempt-unsafe way.
> Use raw_smp_processor_id() to avoid such warnings, adding explanatory
> comments.
> 
> Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
> Fixes: ca3f969dcb11 ("powerpc/paravirt: Use is_kvm_guest() in vcpu_is_preempted()")

Looks good to me.

Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> ---
>  arch/powerpc/include/asm/paravirt.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
> index 39f173961f6a..eb7df559ae74 100644
> --- a/arch/powerpc/include/asm/paravirt.h
> +++ b/arch/powerpc/include/asm/paravirt.h
> @@ -110,7 +110,23 @@ static inline bool vcpu_is_preempted(int cpu)
> 
>  #ifdef CONFIG_PPC_SPLPAR
>  	if (!is_kvm_guest()) {
> -		int first_cpu = cpu_first_thread_sibling(smp_processor_id());
> +		int first_cpu;
> +
> +		/*
> +		 * The result of vcpu_is_preempted() is used in a
> +		 * speculative way, and is always subject to invalidation
> +		 * by events internal and external to Linux. While we can
> +		 * be called in preemptable context (in the Linux sense),
> +		 * we're not accessing per-cpu resources in a way that can
> +		 * race destructively with Linux scheduler preemption and
> +		 * migration, and callers can tolerate the potential for
> +		 * error introduced by sampling the CPU index without
> +		 * pinning the task to it. So it is permissible to use
> +		 * raw_smp_processor_id() here to defeat the preempt debug
> +		 * warnings that can arise from using smp_processor_id()
> +		 * in arbitrary contexts.
> +		 */
> +		first_cpu = cpu_first_thread_sibling(raw_smp_processor_id());
> 
>  		/*
>  		 * The PowerVM hypervisor dispatches VMs on a whole core
> -- 
> 2.31.1
> 

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks

[Michael Ellerman]

On Tue, 28 Sep 2021 16:41:45 -0500, Nathan Lynch wrote:
> Minor changes arising from discovering that this code throws warnings with
> DEBUG_PREEMPT kernels.
> 
> Changes since v1:
> * Additional commentary to (1) distinguish hypervisor dispatch and preempt
>   behavior from kernel scheduler preemption; and (2) more clearly justify
>   the use of raw_smp_processor_id().
> * Additional patch to update existing comments before making the functional
>   change.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc/paravirt: vcpu_is_preempted() commentary
      https://git.kernel.org/powerpc/c/799f9b51db688608b50e630a57bee5f699b268ca
[2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()
      https://git.kernel.org/powerpc/c/fda0eb220021a97c1d656434b9340ebf3fc4704a

cheers