* [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks
@ 2021-09-28 21:41 Nathan Lynch
2021-09-28 21:41 ` [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary Nathan Lynch
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Nathan Lynch @ 2021-09-28 21:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: srikar, npiggin
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary
2021-09-28 21:41 [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Nathan Lynch
@ 2021-09-28 21:41 ` Nathan Lynch
2021-09-29 7:30 ` Srikar Dronamraju
2021-09-28 21:41 ` [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted() Nathan Lynch
2021-10-11 12:06 ` [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Nathan Lynch @ 2021-09-28 21:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: srikar, npiggin
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()
2021-09-28 21:41 [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Nathan Lynch
2021-09-28 21:41 ` [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary Nathan Lynch
@ 2021-09-28 21:41 ` Nathan Lynch
2021-09-29 7:30 ` Srikar Dronamraju
2021-10-11 12:06 ` [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Nathan Lynch @ 2021-09-28 21:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: srikar, npiggin
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary
2021-09-28 21:41 ` [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary Nathan Lynch
@ 2021-09-29 7:30 ` Srikar Dronamraju
0 siblings, 0 replies; 6+ messages in thread
From: Srikar Dronamraju @ 2021-09-29 7:30 UTC (permalink / raw)
To: Nathan Lynch; +Cc: linuxppc-dev, npiggin
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted()
2021-09-28 21:41 ` [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted() Nathan Lynch
@ 2021-09-29 7:30 ` Srikar Dronamraju
0 siblings, 0 replies; 6+ messages in thread
From: Srikar Dronamraju @ 2021-09-29 7:30 UTC (permalink / raw)
To: Nathan Lynch; +Cc: linuxppc-dev, npiggin
* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks
2021-09-28 21:41 [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Nathan Lynch
2021-09-28 21:41 ` [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary Nathan Lynch
2021-09-28 21:41 ` [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted() Nathan Lynch
@ 2021-10-11 12:06 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2021-10-11 12:06 UTC (permalink / raw)
To: linuxppc-dev, Nathan Lynch; +Cc: srikar, npiggin
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-11 12:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-28 21:41 [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Nathan Lynch
2021-09-28 21:41 ` [PATCH v2 1/2] powerpc/paravirt: vcpu_is_preempted() commentary Nathan Lynch
2021-09-29 7:30 ` Srikar Dronamraju
2021-09-28 21:41 ` [PATCH v2 2/2] powerpc/paravirt: correct preempt debug splat in vcpu_is_preempted() Nathan Lynch
2021-09-29 7:30 ` Srikar Dronamraju
2021-10-11 12:06 ` [PATCH v2 0/2] powerpc/paravirt: vcpu_is_preempted() tweaks Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).