Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Roger Pau Monné" <roger.pau@citrix.com>
To: "Durrant, Paul" <pdurrant@amazon.co.uk>
Cc: "Valentin, Eduardo" <eduval@amazon.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"benh@kernel.crashing.org" <benh@kernel.crashing.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	"pavel@ucw.cz" <pavel@ucw.cz>, "hpa@zytor.com" <hpa@zytor.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>,
	"fllinden@amaozn.com" <fllinden@amaozn.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Singh, Balbir" <sblbir@amazon.com>,
	"len.brown@intel.com" <len.brown@intel.com>,
	"jgross@suse.com" <jgross@suse.com>,
	"konrad.wilk@oracle.com" <konrad.wilk@oracle.com>,
	"Agarwal, Anchal" <anchalag@amazon.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"boris.ostrovsky@oracle.com" <boris.ostrovsky@oracle.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>,
	"Kamata, Munehisa" <kamatam@amazon.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"Woodhouse, David" <dwmw@amazon.co.uk>
Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation
Date: Thu, 20 Feb 2020 17:48:39 +0100
Message-ID: <20200220164839.GR4679@Air-de-Roger> (raw)
In-Reply-To: <c9662397256a4568a5cc7d70a84940e5@EX13D32EUC003.ant.amazon.com>

On Thu, Feb 20, 2020 at 04:23:13PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Roger Pau Monné <roger.pau@citrix.com>
> > Sent: 20 February 2020 15:45
> > To: Durrant, Paul <pdurrant@amazon.co.uk>
> > Cc: Agarwal, Anchal <anchalag@amazon.com>; Valentin, Eduardo
> > <eduval@amazon.com>; len.brown@intel.com; peterz@infradead.org;
> > benh@kernel.crashing.org; x86@kernel.org; linux-mm@kvack.org;
> > pavel@ucw.cz; hpa@zytor.com; tglx@linutronix.de; sstabellini@kernel.org;
> > fllinden@amaozn.com; Kamata, Munehisa <kamatam@amazon.com>;
> > mingo@redhat.com; xen-devel@lists.xenproject.org; Singh, Balbir
> > <sblbir@amazon.com>; axboe@kernel.dk; konrad.wilk@oracle.com;
> > bp@alien8.de; boris.ostrovsky@oracle.com; jgross@suse.com;
> > netdev@vger.kernel.org; linux-pm@vger.kernel.org; rjw@rjwysocki.net;
> > linux-kernel@vger.kernel.org; vkuznets@redhat.com; davem@davemloft.net;
> > Woodhouse, David <dwmw@amazon.co.uk>
> > Subject: Re: [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks
> > for PM suspend and hibernation
> > 
> > On Thu, Feb 20, 2020 at 08:54:36AM +0000, Durrant, Paul wrote:
> > > > -----Original Message-----
> > > > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of
> > > > Roger Pau Monné
> > > > Sent: 20 February 2020 08:39
> > > > To: Agarwal, Anchal <anchalag@amazon.com>
> > > > Cc: Valentin, Eduardo <eduval@amazon.com>; len.brown@intel.com;
> > > > peterz@infradead.org; benh@kernel.crashing.org; x86@kernel.org; linux-
> > > > mm@kvack.org; pavel@ucw.cz; hpa@zytor.com; tglx@linutronix.de;
> > > > sstabellini@kernel.org; fllinden@amaozn.com; Kamata, Munehisa
> > > > <kamatam@amazon.com>; mingo@redhat.com; xen-
> > devel@lists.xenproject.org;
> > > > Singh, Balbir <sblbir@amazon.com>; axboe@kernel.dk;
> > > > konrad.wilk@oracle.com; bp@alien8.de; boris.ostrovsky@oracle.com;
> > > > jgross@suse.com; netdev@vger.kernel.org; linux-pm@vger.kernel.org;
> > > > rjw@rjwysocki.net; linux-kernel@vger.kernel.org; vkuznets@redhat.com;
> > > > davem@davemloft.net; Woodhouse, David <dwmw@amazon.co.uk>
> > > > 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.
> >
> 
> Within an individual PV driver it may well be ok to use common mechanisms for connecting to the backend but issues will arise if any subsequent action is visible to the guest. E.g. a network frontend needs to issue gratuitous ARPs without anything else in the network stack (or monitoring the network stack) knowing that it has happened. 
>  
> > 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.
> 
> The technical justification is that the driver needs to know what kind of suspend or resume it is doing, so that it doesn't do the wrong thing. There may also be differences in the state of the system e.g. in Windows, at least some of the resume-from-xen-suspend code runs with interrupts disabled (which is necessary to make sure enough state is restored before things become visible to other kernel code).
> 
> > 
> > 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.
> > 
> 
> I think abstracting away at the xenbus level to some degree is probably feasible, but some sort of flag should be passed to the individual drivers so they know what circumstances they are operating under.
> 
> > > 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?
> > 
> 
> I don’t know the details for Linux but it may well be to do with assumptions made about the system e.g. the ability to block waiting for something to happen on another CPU (which may have already been quiesced in a PM context).
> 
> > 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.
> > 
> 
> Hopefully what I said above illustrates why it may not be 100% common.

Yes, that's fine. I don't expect it to be 100% common (as I guess
that the hooks will have different prototypes), but I expect
that routines can be shared, and that the approach taken can be the
same.

For example one necessary difference will be that xenbus initiated
suspend won't close the PV connection, in case suspension fails. On PM
suspend you seem to always close the connection beforehand, so you
will always have to re-negotiate on resume even if suspension failed.

What I'm mostly worried about is the different approach to ring
draining. Ie: either xenbus is changed to freeze the queues and drain
the shared rings, or PM uses the already existing logic of not
flushing the rings an re-issuing in-flight requests on resume.

Thanks, Roger.

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

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 23:21 [Xen-devel] [RFC RESEND PATCH v3 00/12] Enable PM hibernation on guest VMs Anchal Agarwal
2020-02-14 23:22 ` [Xen-devel] [RFC PATCH v3 01/12] xen/manage: keep track of the on-going suspend mode Anchal Agarwal
2020-02-14 23:23 ` [Xen-devel] [RFC PATCH v3 02/12] xenbus: add freeze/thaw/restore callbacks support Anchal Agarwal
2020-02-14 23:23 ` [Xen-devel] [RFC PATCH v3 03/12] x86/xen: Introduce new function to map HYPERVISOR_shared_info on Resume Anchal Agarwal
2020-02-14 23:24 ` [Xen-devel] [RFC PATCH v3 04/12] x86/xen: add system core suspend and resume callbacks Anchal Agarwal
2020-02-14 23:24 ` [Xen-devel] [RFC PATCH v3 05/12] xen-netfront: add callbacks for PM suspend and hibernation support Anchal Agarwal
2020-02-14 23:25 ` [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal
2020-02-17 10:05   ` Roger Pau Monné
2020-02-17 23:05     ` Anchal Agarwal
2020-02-18  9:16       ` Roger Pau Monné
2020-02-19 18:04         ` Anchal Agarwal
2020-02-20  8:39           ` Roger Pau Monné
2020-02-20  8:54             ` Durrant, Paul
2020-02-20 15:45               ` Roger Pau Monné
2020-02-20 16:23                 ` Durrant, Paul
2020-02-20 16:48                   ` Roger Pau Monné [this message]
2020-02-20 17:01                     ` Durrant, Paul
2020-02-21  0:49                       ` Anchal Agarwal
2020-02-21  9:47                         ` Roger Pau Monné
2020-02-21  9:22                       ` Roger Pau Monné
2020-02-21  9:56                         ` Durrant, Paul
2020-02-21 10:21                           ` Roger Pau Monné
2020-02-21 10:33                             ` Durrant, Paul
2020-02-21 11:51                               ` Roger Pau Monné
2020-02-21 14:24   ` Roger Pau Monné
2020-03-06 18:40     ` Anchal Agarwal
2020-03-09  9:54       ` Roger Pau Monné
     [not found]         ` <FA688A68-5372-4757-B075-A69A45671CB9@amazon.com>
     [not found]           ` <20200312090435.GK24449@Air-de-Roger.citrite.net>
2020-03-13 17:21             ` Anchal Agarwal
2020-02-14 23:25 ` [Xen-devel] [RFC PATCH v3 07/12] genirq: Shutdown irq chips in suspend/resume during hibernation Anchal Agarwal
2020-03-06 23:03   ` Thomas Gleixner
2020-03-09 22:37     ` [Xen-devel] [EXTERNAL][RFC " Anchal Agarwal
2020-02-14 23:26 ` [Xen-devel] [RFC PATCH v3 08/12] xen/time: introduce xen_{save, restore}_steal_clock Anchal Agarwal
2020-02-14 23:27 ` [Xen-devel] [RFC PATCH v3 09/12] x86/xen: save and restore steal clock Anchal Agarwal
2020-02-14 23:27 ` [Xen-devel] [RFC PATCH v3 10/12] xen: Introduce wrapper for save/restore sched clock offset Anchal Agarwal
2020-02-14 23:27 ` [Xen-devel] [RFC PATCH v3 11/12] xen: Update sched clock offset to avoid system instability in hibernation Anchal Agarwal
2020-02-14 23:28 ` [Xen-devel] [RFC PATCH v3 12/12] PM / hibernate: update the resume offset on SNAPSHOT_SET_SWAP_AREA Anchal Agarwal
  -- strict thread matches above, loose matches on Subject: below --
2020-02-12 22:32 [Xen-devel] [RFC PATCH v3 06/12] xen-blkfront: add callbacks for PM suspend and hibernation Anchal Agarwal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200220164839.GR4679@Air-de-Roger \
    --to=roger.pau@citrix.com \
    --cc=anchalag@amazon.com \
    --cc=axboe@kernel.dk \
    --cc=benh@kernel.crashing.org \
    --cc=boris.ostrovsky@oracle.com \
    --cc=bp@alien8.de \
    --cc=davem@davemloft.net \
    --cc=dwmw@amazon.co.uk \
    --cc=eduval@amazon.com \
    --cc=fllinden@amaozn.com \
    --cc=hpa@zytor.com \
    --cc=jgross@suse.com \
    --cc=kamatam@amazon.com \
    --cc=konrad.wilk@oracle.com \
    --cc=len.brown@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=pdurrant@amazon.co.uk \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=sblbir@amazon.com \
    --cc=sstabellini@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=vkuznets@redhat.com \
    --cc=x86@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git