Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Durrant <Paul.Durrant@citrix.com>
To: Anthony Perard <anthony.perard@citrix.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: Anthony Perard <anthony.perard@citrix.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
Date: Wed, 21 Aug 2019 15:40:05 +0000
Message-ID: <703d5a46d4c74eb4afd93d76b7341efc@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <20190821092020.17952-3-anthony.perard@citrix.com>

> -----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).
 
>  static void xen_device_backend_changed(void *opaque)
> @@ -529,13 +538,13 @@ static void xen_device_backend_changed(void *opaque)
>          state = XenbusStateUnknown;
>      }
> 
> -    xen_device_backend_set_state(xendev, state);
> +    xen_device_backend_record_state(xendev, state);
> 
>      if (xen_device_backend_scanf(xendev, "online", "%u", &online) != 1) {
>          online = 0;
>      }
> 
> -    xen_device_backend_set_online(xendev, !!online);
> +    xen_device_backend_set_online(xendev, !!online, false);
> 

You could then pass 'false' here in both cases.

>      /*
>       * If the toolstack (or unplug request callback) has set the backend
> @@ -683,7 +692,8 @@ int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
>  }
> 
>  static void xen_device_frontend_set_state(XenDevice *xendev,
> -                                          enum xenbus_state state)
> +                                          enum xenbus_state state,
> +                                          bool export)
>  {
>      const char *type = object_get_typename(OBJECT(xendev));
> 
> @@ -695,7 +705,8 @@ static void xen_device_frontend_set_state(XenDevice *xendev,
>                                      xs_strstate(state));
> 
>      xendev->frontend_state = state;
> -    xen_device_frontend_printf(xendev, "state", "%u", state);
> +    if (export)
> +        xen_device_frontend_printf(xendev, "state", "%u", state);
>  }
> 
>  static void xen_device_frontend_changed(void *opaque)
> @@ -711,7 +722,7 @@ static void xen_device_frontend_changed(void *opaque)
>          state = XenbusStateUnknown;
>      }
> 
> -    xen_device_frontend_set_state(xendev, state);
> +    xen_device_frontend_set_state(xendev, state, false);
> 
>      if (state == XenbusStateInitialising &&
>          xendev->backend_state == XenbusStateClosed &&
> @@ -1146,7 +1157,7 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
>                                xendev->frontend_id);
>      xen_device_backend_printf(xendev, "hotplug-status", "connected");
> 
> -    xen_device_backend_set_online(xendev, true);
> +    xen_device_backend_set_online(xendev, true, true);
>      xen_device_backend_set_state(xendev, XenbusStateInitWait);
> 
>      xen_device_frontend_printf(xendev, "backend", "%s",
> @@ -1154,7 +1165,7 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
>      xen_device_frontend_printf(xendev, "backend-id", "%u",
>                                 xenbus->backend_id);
> 
> -    xen_device_frontend_set_state(xendev, XenbusStateInitialising);
> +    xen_device_frontend_set_state(xendev, XenbusStateInitialising, true);
> 

And similarly pass 'true' here for all three cases.

What do you think?

  Paul

>      xendev->exit.notify = xen_device_exit;
>      qemu_add_exit_notifier(&xendev->exit);
> --
> 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 [this message]
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
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=703d5a46d4c74eb4afd93d76b7341efc@AMSPEX02CL03.citrite.net \
    --to=paul.durrant@citrix.com \
    --cc=anthony.perard@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
	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