Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 20, 2020 at 08:54:36AM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of
> > Roger Pau Monné
> > Sent: 20 February 2020 08:39
> > To: Agarwal, Anchal <anchalag@xxxxxxxxxx>
> > Cc: Valentin, Eduardo <eduval@xxxxxxxxxx>; len.brown@xxxxxxxxx;
> > peterz@xxxxxxxxxxxxx; benh@xxxxxxxxxxxxxxxxxxx; x86@xxxxxxxxxx; linux-
> > mm@xxxxxxxxx; pavel@xxxxxx; hpa@xxxxxxxxx; tglx@xxxxxxxxxxxxx;
> > sstabellini@xxxxxxxxxx; fllinden@xxxxxxxxxx; Kamata, Munehisa
> > <kamatam@xxxxxxxxxx>; mingo@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> > Singh, Balbir <sblbir@xxxxxxxxxx>; axboe@xxxxxxxxx;
> > konrad.wilk@xxxxxxxxxx; bp@xxxxxxxxx; boris.ostrovsky@xxxxxxxxxx;
> > jgross@xxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> > rjw@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; vkuznets@xxxxxxxxxx;
> > davem@xxxxxxxxxxxxx; Woodhouse, David <dwmw@xxxxxxxxxxxx>
> > Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks
> > for PM suspend and hibernation
> > 
> > Thanks for this work, please see below.
> > 
> > On Wed, Feb 19, 2020 at 06:04:24PM +0000, Anchal Agarwal wrote:
> > > On Tue, Feb 18, 2020 at 10:16:11AM +0100, Roger Pau Monné wrote:
> > > > On Mon, Feb 17, 2020 at 11:05:53PM +0000, Anchal Agarwal wrote:
> > > > > On Mon, Feb 17, 2020 at 11:05:09AM +0100, Roger Pau Monné wrote:
> > > > > > On Fri, Feb 14, 2020 at 11:25:34PM +0000, Anchal Agarwal wrote:
> > > > > Quiescing the queue seemed a better option here as we want to make
> > sure ongoing
> > > > > requests dispatches are totally drained.
> > > > > I should accept that some of these notion is borrowed from how nvme
> > freeze/unfreeze
> > > > > is done although its not apple to apple comparison.
> > > >
> > > > That's fine, but I would still like to requests that you use the same
> > > > logic (as much as possible) for both the Xen and the PM initiated
> > > > suspension.
> > > >
> > > > So you either apply this freeze/unfreeze to the Xen suspension (and
> > > > drop the re-issuing of requests on resume) or adapt the same approach
> > > > as the Xen initiated suspension. Keeping two completely different
> > > > approaches to suspension / resume on blkfront is not suitable long
> > > > term.
> > > >
> > > I agree with you on overhaul of xen suspend/resume wrt blkfront is a
> > good
> > > idea however, IMO that is a work for future and this patch series should
> > > not be blocked for it. What do you think?
> > 
> > It's not so much that I think an overhaul of suspend/resume in
> > blkfront is needed, it's just that I don't want to have two completely
> > different suspend/resume paths inside blkfront.
> > 
> > So from my PoV I think the right solution is to either use the same
> > code (as much as possible) as it's currently used by Xen initiated
> > suspend/resume, or to also switch Xen initiated suspension to use the
> > newly introduced code.
> > 
> > Having two different approaches to suspend/resume in the same driver
> > is a recipe for disaster IMO: it adds complexity by forcing developers
> > to take into account two different suspend/resume approaches when
> > there's no need for it.
> 
> I disagree. S3 or S4 suspend/resume (or perhaps we should call them power 
> state transitions to avoid confusion) are quite different from Xen 
> suspend/resume.
> Power state transitions ought to be, and indeed are, visible to the software 
> running inside the guest. Applications, as well as drivers, can receive 
> notification and take whatever action they deem appropriate.
> Xen suspend/resume OTOH is used when a guest is migrated and the code should 
> go to all lengths possible to make any software running inside the guest 
> (other than Xen specific enlightened code, such as PV drivers) completely 
> unaware that anything has actually happened.

So from what you say above PM state transitions are notified to all
drivers, and Xen suspend/resume is only notified to PV drivers, and
here we are speaking about blkfront which is a PV driver, and should
get notified in both cases. So I'm unsure why the same (or at least
very similar) approach can't be used in both cases.

The suspend/resume approach proposed by this patch is completely
different than the one used by a xenbus initiated suspend/resume, and
I don't see a technical reason that warrants this difference.

I'm not saying that the approach used here is wrong, it's just that I
don't see the point in having two different ways to do suspend/resume
in the same driver, unless there's a technical reason for it, which I
don't think has been provided.

I would be fine with switching xenbus initiated suspend/resume to also
use the approach proposed here: freeze the queues and drain the shared
rings before suspending.

> So, whilst it may be possible to use common routines to, for example, 
> re-establish PV frontend/backend communication, PV frontend code should be 
> acutely aware of the circumstances they are operating in. I can cite example 
> code in the Windows PV driver, which have supported guest S3/S4 power state 
> transitions since day 1.

Hm, please bear with me, as I'm not sure I fully understand. Why isn't
the current suspend/resume logic suitable for PM transitions?

As said above, I'm happy to switch xenbus initiated suspend/resume to
use the logic in this patch, but unless there's a technical reason for
it I don't see why blkfront should have two completely different
approaches to suspend/resume depending on whether it's a PM or a
xenbus state change.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel