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 11:21:32 +0100 [thread overview]
Message-ID: <20190822102132.GJ1289@perard.uk.xensource.com> (raw)
In-Reply-To: <703d5a46d4c74eb4afd93d76b7341efc@AMSPEX02CL03.citrite.net>
On Wed, Aug 21, 2019 at 04:40:05PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > Sent: 21 August 2019 10:20
> > To: qemu-devel@nongnu.org
> > Cc: Anthony Perard <anthony.perard@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Paul
> > Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> > Subject: [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> >
> > When QEMU receive a xenstore watch event suggesting that the "state" or
> > "online" status of the frontend or the backend changed, it record this
> > in its own state but it also re-write the value back into xenstore even
> > so there were no changed. This trigger an unnecessary xenstore watch
> > event which QEMU will process again (and maybe the frontend as well).
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > hw/xen/xen-bus.c | 37 ++++++++++++++++++++++++-------------
> > 1 file changed, 24 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > index 982eca4533..c83f07424a 100644
> > --- a/hw/xen/xen-bus.c
> > +++ b/hw/xen/xen-bus.c
> > @@ -481,20 +481,27 @@ static int xen_device_backend_scanf(XenDevice *xendev, const char *key,
> > return rc;
> > }
> >
> > -void xen_device_backend_set_state(XenDevice *xendev,
> > - enum xenbus_state state)
> > +static bool xen_device_backend_record_state(XenDevice *xendev,
> > + enum xenbus_state state)
> > {
> > const char *type = object_get_typename(OBJECT(xendev));
> >
> > if (xendev->backend_state == state) {
> > - return;
> > + return false;
> > }
> >
> > trace_xen_device_backend_state(type, xendev->name,
> > xs_strstate(state));
> >
> > xendev->backend_state = state;
> > - xen_device_backend_printf(xendev, "state", "%u", state);
> > + return true;
> > +}
> > +
> > +void xen_device_backend_set_state(XenDevice *xendev,
> > + enum xenbus_state state)
> > +{
> > + if (xen_device_backend_record_state(xendev, state))
> > + xen_device_backend_printf(xendev, "state", "%u", state);
> > }
> >
> > enum xenbus_state xen_device_backend_get_state(XenDevice *xendev)
> > @@ -502,7 +509,8 @@ enum xenbus_state xen_device_backend_get_state(XenDevice *xendev)
> > return xendev->backend_state;
> > }
> >
> > -static void xen_device_backend_set_online(XenDevice *xendev, bool online)
> > +static void xen_device_backend_set_online(XenDevice *xendev, bool online,
> > + bool export)
> > {
> > const char *type = object_get_typename(OBJECT(xendev));
> >
> > @@ -513,7 +521,8 @@ static void xen_device_backend_set_online(XenDevice *xendev, bool online)
> > trace_xen_device_backend_online(type, xendev->name, online);
> >
> > xendev->backend_online = online;
> > - xen_device_backend_printf(xendev, "online", "%u", online);
> > + if (export)
> > + xen_device_backend_printf(xendev, "online", "%u", online);
> > }
> >
>
> Perhaps the behaviour of backend_set_state() and backend_set_online() could be the same? I.e. they both take an 'export' (or perhaps 'publish'?) parameter and only write xenstore if that is true. (I realise that would involve modifying xen-block to pass 'true' as the extra export/publish param, but I think it would be neater overall).
I've actually did it this way for backend_set_state() because the only
reason to update internal states without writing that state into
xenstore is because the xenstore state changed, so
{front,back}end_changed() are the only function that don't want/need to
write the new state into xenstore. I wanted to avoid misuse of the
extra export/publish param in future backend drivers.
As for frontend_set_state() and backend_set_online(), they are only used
in xen-bus.c, creating a new function didn't seems as needed.
I kind of think that maybe I should go further and also have
frontend_record_state() is it could be possible to have frontend drivers
in QEMU. (and maybe record_online so they all looks the same.)
So, would you prefer to have the extra param to *_set_*() that should be
"true" outside of *_changed(), or the extra functions like I did with
backend_{set,record}_state() ?
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-08-22 10:21 UTC|newest]
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 [this message]
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
2019-08-22 13:21 ` Paul Durrant
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=20190822102132.GJ1289@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).