linux-kernel - Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 21 Oct 2021 12:37:05 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     John Keeping <john@...anate.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-rt-users@...r.kernel.org, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH RT] PM: runtime: avoid retry loops on RT

On Wed, Oct 6, 2021 at 8:18 PM John Keeping <john@...anate.com> wrote:
>
> On Wed, 6 Oct 2021 19:05:50 +0200
> "Rafael J. Wysocki" <rafael@...nel.org> wrote:
>
> > On Tue, Oct 5, 2021 at 7:17 PM John Keeping <john@...anate.com> wrote:
> > >
> > > On Tue, 5 Oct 2021 18:38:27 +0200
> > > "Rafael J. Wysocki" <rafael@...nel.org> wrote:
> > >
> > > > On Tue, Oct 5, 2021 at 6:14 PM John Keeping <john@...anate.com> wrote:
> > > > >
> > > > > With PREEMPT_RT spin_unlock() is identical to spin_unlock_irq() so there
> > > > > is no reason to have a special case using the former.  Furthermore,
> > > > > spin_unlock() enables preemption meaning that a task in RESUMING or
> > > > > SUSPENDING state may be preempted by a higher priority task running
> > > > > pm_runtime_get_sync() leading to a livelock.
> > > > >
> > > > > Use the non-irq_safe path for all waiting so that the waiting task will
> > > > > block.
> > > > >
> > > > > Note that this changes only the waiting behaviour of irq_safe, other
> > > > > uses are left unchanged so that the parent device always remains active
> > > > > in the same way as !RT.
> > > > >
> > > > > Signed-off-by: John Keeping <john@...anate.com>
> > > >
> > > > So basically, the idea is that the irq_safe flag should have no effect
> > > > when CONFIG_PREEMPT_RT is set, right?
> > > >
> > > > Wouldn't it be cleaner to make it not present at all in that case?
> > >
> > > Yes, just replacing pm_runtime_irq_safe() with an empty function would
> > > also fix it, but I'm not sure if that will have unexpected effects from
> > > the parent device suspending/resuming, especially in terms of latency
> > > for handling interrupts.
> >
> > Well, the code as is doesn't work with CONFIG_PREEMPT_RT set anyway in general.
> >
> > Also this is not just pm_runtime_irq_safe(), but every access to this
> > flag (and there's more  of them than just the ones changed below).
> >
> > What about putting the flag under #ifdef CONFIG_PREEMPT_RT and
> > providing read/write accessor helpers for it that will be empty in
> > RT-enabled kernels?
>
> That's the other approach I considered, but there are really two things
> that irq_safe means here:
>
> 1) Call the suspend/resume hooks with interrupts disabled
>
> 2) Keep the parent device running and make other changes that allow (1)
>    on non-RT systems (for example in amba_pm_runtime_suspend() leave the
>    clock prepared when irq_safe is set, but unprepare it otherwise)
>
> In the approach of leaving the flag unset on PREEMPT_RT we solve the
> primary problem which is that (1) is irrelevant on RT, but that would
> also affect (2) and I'm not sure whether that's desirable or not.
>
> It's quite possible the answer is that the other changes don't matter
> and we should take the simpler approach of just removing irq_safe under
> CONFIG_PREEMPT_RT.  I'm becoming convinced that this is the right
> answer, but I'm not confident that I fully understand the wider
> ramifications.

The initial motivation for adding irq_safe was to allow interrupt
handlers of some devices to use PM-runtime, but in RT kernels that's
possible regardless IIUC, so I don't see a reason for having irq_safe
at all in that case.