* [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT
@ 2021-11-17 18:37 John Keeping
2021-11-17 18:53 ` Rafael J. Wysocki
0 siblings, 1 reply; 3+ messages in thread
From: John Keeping @ 2021-11-17 18:37 UTC (permalink / raw)
To: linux-pm
Cc: John Keeping, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman,
linux-kernel
With PREEMPT_RT the cpu_relax() loops in rpm_suspend and rpm_resume can
cause unbounded latency if they preempt an asynchronous suspend. The
main scenario where this can happen is when a realtime thread resumes a
device while it is asynchronously suspending on a worker thread.
I'm not convinced this can actually happen in the rpm_suspend case, or
at least it's a lot less likely for a synchronous suspend to run at the
same time as an asynchronous suspend, but both functions are updated
here for symmetry.
For devices setting power.irq_safe, it is possible that RPM functions
will be called with a spinlock held (for example in
pl330_issue_pending()). This means a normal call to schedule() can't be
used, but to avoid the priority inversion it is necessary to wait and
schedule. schedule_rtlock() is only available when CONFIG_PREEMPT_RT is
defined, so even though the logic is correct without any preprocessor
guards around schedule_rtlock(), they are necessary for compilation.
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: John Keeping <john@metanate.com>
---
Changes since v1:
- Use schedule_rtlock() instead of schedule() for PREEMPT_RT & irq_safe
- Rewritten commit description
drivers/base/power/runtime.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index f3de7bfc7f5b..fdf461bfae8c 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -596,7 +596,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
goto out;
}
- if (dev->power.irq_safe) {
+ if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
spin_unlock(&dev->power.lock);
cpu_relax();
@@ -614,7 +614,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
spin_unlock_irq(&dev->power.lock);
- schedule();
+#ifdef CONFIG_PREEMPT_RT
+ if (dev->power.irq_safe)
+ schedule_rtlock();
+ else
+#endif
+ schedule();
spin_lock_irq(&dev->power.lock);
}
@@ -779,7 +784,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
goto out;
}
- if (dev->power.irq_safe) {
+ if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
spin_unlock(&dev->power.lock);
cpu_relax();
@@ -798,7 +803,12 @@ static int rpm_resume(struct device *dev, int rpmflags)
spin_unlock_irq(&dev->power.lock);
- schedule();
+#ifdef CONFIG_PREEMPT_RT
+ if (dev->power.irq_safe)
+ schedule_rtlock();
+ else
+#endif
+ schedule();
spin_lock_irq(&dev->power.lock);
}
--
2.34.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT
2021-11-17 18:37 [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT John Keeping
@ 2021-11-17 18:53 ` Rafael J. Wysocki
2021-11-17 19:25 ` John Keeping
0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2021-11-17 18:53 UTC (permalink / raw)
To: John Keeping
Cc: Linux PM, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman,
Linux Kernel Mailing List
On Wed, Nov 17, 2021 at 7:37 PM John Keeping <john@metanate.com> wrote:
>
> With PREEMPT_RT the cpu_relax() loops in rpm_suspend and rpm_resume can
> cause unbounded latency if they preempt an asynchronous suspend. The
> main scenario where this can happen is when a realtime thread resumes a
> device while it is asynchronously suspending on a worker thread.
>
> I'm not convinced this can actually happen in the rpm_suspend case, or
> at least it's a lot less likely for a synchronous suspend to run at the
> same time as an asynchronous suspend, but both functions are updated
> here for symmetry.
>
> For devices setting power.irq_safe, it is possible that RPM functions
> will be called with a spinlock held (for example in
> pl330_issue_pending()). This means a normal call to schedule() can't be
> used, but to avoid the priority inversion it is necessary to wait and
> schedule. schedule_rtlock() is only available when CONFIG_PREEMPT_RT is
> defined, so even though the logic is correct without any preprocessor
> guards around schedule_rtlock(), they are necessary for compilation.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
> Changes since v1:
> - Use schedule_rtlock() instead of schedule() for PREEMPT_RT & irq_safe
> - Rewritten commit description
>
> drivers/base/power/runtime.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index f3de7bfc7f5b..fdf461bfae8c 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -596,7 +596,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> goto out;
> }
>
> - if (dev->power.irq_safe) {
> + if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
Please add a helper to avoid code duplication related to this (even
though there is a small amount of it).
> spin_unlock(&dev->power.lock);
>
> cpu_relax();
> @@ -614,7 +614,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>
> spin_unlock_irq(&dev->power.lock);
>
> - schedule();
> +#ifdef CONFIG_PREEMPT_RT
> + if (dev->power.irq_safe)
> + schedule_rtlock();
> + else
> +#endif
Same here, and please use the #ifdet inside the helper.
> + schedule();
>
> spin_lock_irq(&dev->power.lock);
> }
> @@ -779,7 +784,7 @@ static int rpm_resume(struct device *dev, int rpmflags)
> goto out;
> }
>
> - if (dev->power.irq_safe) {
> + if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
> spin_unlock(&dev->power.lock);
>
> cpu_relax();
> @@ -798,7 +803,12 @@ static int rpm_resume(struct device *dev, int rpmflags)
>
> spin_unlock_irq(&dev->power.lock);
>
> - schedule();
> +#ifdef CONFIG_PREEMPT_RT
> + if (dev->power.irq_safe)
> + schedule_rtlock();
> + else
> +#endif
> + schedule();
>
> spin_lock_irq(&dev->power.lock);
> }
> --
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT
2021-11-17 18:53 ` Rafael J. Wysocki
@ 2021-11-17 19:25 ` John Keeping
0 siblings, 0 replies; 3+ messages in thread
From: John Keeping @ 2021-11-17 19:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, Thomas Gleixner, Peter Zijlstra, Ingo Molnar,
Pavel Machek, Len Brown, Greg Kroah-Hartman,
Linux Kernel Mailing List
On Wed, 17 Nov 2021 19:53:47 +0100
"Rafael J. Wysocki" <rafael@kernel.org> wrote:
> On Wed, Nov 17, 2021 at 7:37 PM John Keeping <john@metanate.com> wrote:
> >
> > With PREEMPT_RT the cpu_relax() loops in rpm_suspend and rpm_resume can
> > cause unbounded latency if they preempt an asynchronous suspend. The
> > main scenario where this can happen is when a realtime thread resumes a
> > device while it is asynchronously suspending on a worker thread.
> >
> > I'm not convinced this can actually happen in the rpm_suspend case, or
> > at least it's a lot less likely for a synchronous suspend to run at the
> > same time as an asynchronous suspend, but both functions are updated
> > here for symmetry.
> >
> > For devices setting power.irq_safe, it is possible that RPM functions
> > will be called with a spinlock held (for example in
> > pl330_issue_pending()). This means a normal call to schedule() can't be
> > used, but to avoid the priority inversion it is necessary to wait and
> > schedule. schedule_rtlock() is only available when CONFIG_PREEMPT_RT is
> > defined, so even though the logic is correct without any preprocessor
> > guards around schedule_rtlock(), they are necessary for compilation.
> >
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> > Changes since v1:
> > - Use schedule_rtlock() instead of schedule() for PREEMPT_RT & irq_safe
> > - Rewritten commit description
> >
> > drivers/base/power/runtime.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index f3de7bfc7f5b..fdf461bfae8c 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -596,7 +596,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> > goto out;
> > }
> >
> > - if (dev->power.irq_safe) {
> > + if (dev->power.irq_safe && !IS_ENABLED(CONFIG_PREEMPT_RT)) {
>
> Please add a helper to avoid code duplication related to this (even
> though there is a small amount of it).
Ack. I'd like some feedback on the schedule_rtlock() approach from the
scheduler & RT people, so I'll wait a bit before sending a v3.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-17 19:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 18:37 [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT John Keeping
2021-11-17 18:53 ` Rafael J. Wysocki
2021-11-17 19:25 ` John Keeping
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).