Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: "Durrant, Paul" <pdurrant@amazon.co.uk>
To: "Roger Pau Monné" <roger.pau@citrix.com>
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:01:52 +0000
Message-ID: <e42fa35800f04b6f953e4af87f2c1a02@EX13D32EUC003.ant.amazon.com> (raw)
In-Reply-To: <20200220164839.GR4679@Air-de-Roger>

> -----Original Message-----
> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: 20 February 2020 16:49
> 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 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.
> 

Yes, that's needs consideration. I don’t think the same semantic can be suitable for both. E.g. in a xen-suspend we need to freeze with as little processing as possible to avoid dirtying RAM late in the migration cycle, and we know that in-flight data can wait. But in a transition to S4 we need to make sure that at least all the in-flight blkif requests get completed, since they probably contain bits of the guest's memory image and that's not going to get saved any other way.

  Paul
_______________________________________________
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é
2020-02-20 17:01                     ` Durrant, Paul [this message]
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=e42fa35800f04b6f953e4af87f2c1a02@EX13D32EUC003.ant.amazon.com \
    --to=pdurrant@amazon.co.uk \
    --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=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=roger.pau@citrix.com \
    --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