Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Anthony PERARD <anthony.perard@citrix.com>
To: Paul Durrant <Paul.Durrant@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
Date: Thu, 22 Aug 2019 14:18:02 +0100
Message-ID: <20190822124427.GL1289@perard.uk.xensource.com> (raw)
In-Reply-To: <40872481b00f4dbeb82ab540e99121a6@AMSPEX02CL03.citrite.net>

On Thu, Aug 22, 2019 at 12:25:44PM +0100, Paul Durrant wrote:
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > Sent: 22 August 2019 12:18
> > 
> > On Thu, Aug 22, 2019 at 11:36:32AM +0100, Paul Durrant wrote:
> > > But, now I look at the code again without your patch applied I don't actually see the problem it is
> > trying to fix. The functions xen_device_[back|front]end_set_state return early if the state being set
> > matches the existing state and hence never get to the line where the state is written to xenstore.
> > 
> > Let's see:
> >     * step 1 (initial states in xenstore and QEMU)
> >         xenstore/frontend/state = 4
> >         xendev->frontend_state = 4
> >     * step 2 (frontend changes state in xenstore)
> >         xenstore/frontend/state = 5
> >     * step 3 (watch event received by QEMU)
> >         xen_device_frontend_changed()
> >             state = read(xenstore/frontend/state) (state=5)
> >             xen_device_frontend_set_state(state)
> >                 xendev->frontend_state != state  (4!=5)
> >                     xendev->frontend_state = state
> >                     xenstore/frontend/state = state
> >     * step 4
> >         # watch event triggers xen_device_frontend_changed() again but
> >         # this time xendev->frontend_state == xenstore/frontend_state
> > 
> > This is how QEMU writes to xenstore an identical value.
> > 
> > That behavior might be an issue if the frontend changes the value after
> > QEMU have read it but before QEMU writes it again.
> 
> Ah, ok, so the problem is actually limited to frontend state because that is written by both frontend and backend, so whether QEMU writes an updated frontend state to xenstore needs to be controlled. It's only called in two places xen_device_frontend_changed() and xen_device_realize(). The write to xenstore should be avoided in the former case, but not the latter. So adding a 'publish' boolean and using that to determine whether the write to xenstore is done seems like the right approach. But I don't think any change is needed to xen_device_backend_set_online() or xen_device_backend_set_state(), is it?

I guess it's not that much of a issue for backend_set_*(), the double
write would only happen when the toolstack try to tear down the backend,
so it would happen only once.

Alright, I'll only change frontend_set_state() and use 'publish'.

-- 
Anthony PERARD

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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20190821092020.17952-1-anthony.perard@citrix.com>
2019-08-21  9:20 ` [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset Anthony PERARD
2019-08-21  9:36   ` Paul Durrant
2019-08-22  9:50     ` Anthony PERARD
2019-08-22  9:59       ` Paul Durrant
2019-08-22 15:01         ` Anthony PERARD
2019-08-21  9:20 ` [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore Anthony PERARD
2019-08-21 15:40   ` Paul Durrant
2019-08-22 10:21     ` Anthony PERARD
2019-08-22 10:36       ` Paul Durrant
2019-08-22 11:17         ` Anthony PERARD
2019-08-22 11:25           ` Paul Durrant
2019-08-22 13:18             ` Anthony PERARD [this message]
2019-08-22 13:21               ` Paul Durrant

Reply instructions:

You may reply publically 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=20190822124427.GL1289@perard.uk.xensource.com \
    --to=anthony.perard@citrix.com \
    --cc=Paul.Durrant@citrix.com \
    --cc=qemu-devel@nongnu.org \
    --cc=sstabellini@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 xen-devel@archiver.kernel.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