Messages in this thread | | | Date | Wed, 17 Nov 2021 19:25:20 +0000 | From | John Keeping <> | Subject | Re: [PATCH v2] PM: runtime: avoid priority inversion on PREEMPT_RT |
| |
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.
| |