[Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Anthony PERARD]

When a frontend want to reset its state and the backend one, it start
with setting "Closing", then wait for the backend (QEMU) to do the same.

But when QEMU is setting "Closing" to its state, it trigger an event
(xenstore watch) that re-execute xen_device_backend_changed() and set
the backend state to "Closed". QEMU should wait for the frontend to
set "Closed" before doing the same.

Before setting "Closed" to the backend_state, we are also going to
check if the frontend was responsible for the transition to "Closing".

Fixes: b6af8926fb858c4f1426e5acb2cfc1f0580ec98a
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
Cc: qemu-stable@nongnu.org
---
 hw/xen/xen-bus.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index e40500242d..982eca4533 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -540,9 +540,11 @@ static void xen_device_backend_changed(void *opaque)
     /*
      * If the toolstack (or unplug request callback) has set the backend
      * state to Closing, but there is no active frontend (i.e. the
-     * state is not Connected) then set the backend state to Closed.
+     * state is not Connected or Closing) then set the backend state
+     * to Closed.
      */
     if (xendev->backend_state == XenbusStateClosing &&
+        xendev->frontend_state != XenbusStateClosing &&
         xendev->frontend_state != XenbusStateConnected) {
         xen_device_backend_set_state(xendev, XenbusStateClosed);
     }
-- 
Anthony PERARD


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

Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Paul Durrant]

> -----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>; qemu-stable@nongnu.org; Stefano Stabellini
> <sstabellini@kernel.org>; Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Subject: [PATCH 1/2] xen-bus: Fix backend state transition on device reset
> 
> When a frontend want to reset its state and the backend one, it start
> with setting "Closing", then wait for the backend (QEMU) to do the same.
> 
> But when QEMU is setting "Closing" to its state, it trigger an event
> (xenstore watch) that re-execute xen_device_backend_changed() and set
> the backend state to "Closed". QEMU should wait for the frontend to
> set "Closed" before doing the same.
> 
> Before setting "Closed" to the backend_state, we are also going to
> check if the frontend was responsible for the transition to "Closing".
> 
> Fixes: b6af8926fb858c4f1426e5acb2cfc1f0580ec98a
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> Cc: qemu-stable@nongnu.org
> ---
>  hw/xen/xen-bus.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> index e40500242d..982eca4533 100644
> --- a/hw/xen/xen-bus.c
> +++ b/hw/xen/xen-bus.c
> @@ -540,9 +540,11 @@ static void xen_device_backend_changed(void *opaque)
>      /*
>       * If the toolstack (or unplug request callback) has set the backend
>       * state to Closing, but there is no active frontend (i.e. the
> -     * state is not Connected) then set the backend state to Closed.
> +     * state is not Connected or Closing) then set the backend state
> +     * to Closed.
>       */
>      if (xendev->backend_state == XenbusStateClosing &&
> +        xendev->frontend_state != XenbusStateClosing &&
>          xendev->frontend_state != XenbusStateConnected) {
>          xen_device_backend_set_state(xendev, XenbusStateClosed);

Actually, I wonder whether it is better to 'whitelist' here? AFAIK the only valid frontend states whether the backend should set itself 'closed' are 'closed' (i.e. the frontend is finished) and 'initialising' (the frontend was never there).

  Paul

>      }
> --
> Anthony PERARD


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

Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Anthony PERARD]

On Wed, Aug 21, 2019 at 10:36:40AM +0100, Paul Durrant wrote:
> > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > index e40500242d..982eca4533 100644
> > --- a/hw/xen/xen-bus.c
> > +++ b/hw/xen/xen-bus.c
> > @@ -540,9 +540,11 @@ static void xen_device_backend_changed(void *opaque)
> >      /*
> >       * If the toolstack (or unplug request callback) has set the backend
> >       * state to Closing, but there is no active frontend (i.e. the
> > -     * state is not Connected) then set the backend state to Closed.
> > +     * state is not Connected or Closing) then set the backend state
> > +     * to Closed.
> >       */
> >      if (xendev->backend_state == XenbusStateClosing &&
> > +        xendev->frontend_state != XenbusStateClosing &&
> >          xendev->frontend_state != XenbusStateConnected) {
> >          xen_device_backend_set_state(xendev, XenbusStateClosed);
> 
> Actually, I wonder whether it is better to 'whitelist' here? AFAIK the only valid frontend states whether the backend should set itself 'closed' are 'closed' (i.e. the frontend is finished) and 'initialising' (the frontend was never there).

Let's see, what are the reason backend=Closing?
    - frontend changed to Closing (because it wants to disconnect)
    - toolstack(libxl) or QEMU(unplug request) set the state to Closing,
      but also online to 0.

What should the backend do in both case:
    - frontend alive: backend should wait
        frontend state might be InitWait, Initialised, Connected,
        Closing.
    - frontend not existing or disconnected: backend can skip waiting
      and go to the next step, Closed.
        frontend might be Initialising, Closed.
        But there are also Unknown, Reconfiguring and Reconfigured which
        are probably errors.

So, the whitelist with Closed and Initialising is a good start, but what
about the Unknown state? (QEMU doesn't have backends were the state
Reconfigur* is possible, so they can be mapped to Unknown for now).

Cheers,

-- 
Anthony PERARD

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

Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 22 August 2019 10:51
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-stable@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 1/2] xen-bus: Fix backend state transition on device reset
> 
> On Wed, Aug 21, 2019 at 10:36:40AM +0100, Paul Durrant wrote:
> > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > > index e40500242d..982eca4533 100644
> > > --- a/hw/xen/xen-bus.c
> > > +++ b/hw/xen/xen-bus.c
> > > @@ -540,9 +540,11 @@ static void xen_device_backend_changed(void *opaque)
> > >      /*
> > >       * If the toolstack (or unplug request callback) has set the backend
> > >       * state to Closing, but there is no active frontend (i.e. the
> > > -     * state is not Connected) then set the backend state to Closed.
> > > +     * state is not Connected or Closing) then set the backend state
> > > +     * to Closed.
> > >       */
> > >      if (xendev->backend_state == XenbusStateClosing &&
> > > +        xendev->frontend_state != XenbusStateClosing &&
> > >          xendev->frontend_state != XenbusStateConnected) {
> > >          xen_device_backend_set_state(xendev, XenbusStateClosed);
> >
> > Actually, I wonder whether it is better to 'whitelist' here? AFAIK the only valid frontend states
> whether the backend should set itself 'closed' are 'closed' (i.e. the frontend is finished) and
> 'initialising' (the frontend was never there).
> 
> Let's see, what are the reason backend=Closing?
>     - frontend changed to Closing (because it wants to disconnect)
>     - toolstack(libxl) or QEMU(unplug request) set the state to Closing,
>       but also online to 0.
> 
> What should the backend do in both case:
>     - frontend alive: backend should wait
>         frontend state might be InitWait, Initialised, Connected,
>         Closing.
>     - frontend not existing or disconnected: backend can skip waiting
>       and go to the next step, Closed.
>         frontend might be Initialising, Closed.
>         But there are also Unknown, Reconfiguring and Reconfigured which
>         are probably errors.
> 
> So, the whitelist with Closed and Initialising is a good start, but what
> about the Unknown state? (QEMU doesn't have backends were the state
> Reconfigur* is possible, so they can be mapped to Unknown for now).

I guess we should consider Unknown (basically a missing xenstore state key) to mean either an admin, or the frontend has screwed up or is malicious so I think we just close down the backend straight away. So maybe listing InitWait, Initialised, Connected, and Closing as frontend states that are 'good' (i.e. we wait in anticipation of the frontend eventually getting to Closed) and then say all other states result in immediate close of the backend. Probably worth having a helper function for saying whether a state is good or not.

  Cheers,

    Paul

> 
> Cheers,
> 
> --
> Anthony PERARD

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

Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset

[Anthony PERARD]

On Thu, Aug 22, 2019 at 10:59:38AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > Sent: 22 August 2019 10:51
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: qemu-devel@nongnu.org; qemu-stable@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>; xen-
> > devel@lists.xenproject.org
> > Subject: Re: [PATCH 1/2] xen-bus: Fix backend state transition on device reset
> > 
> > On Wed, Aug 21, 2019 at 10:36:40AM +0100, Paul Durrant wrote:
> > > > diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
> > > > index e40500242d..982eca4533 100644
> > > > --- a/hw/xen/xen-bus.c
> > > > +++ b/hw/xen/xen-bus.c
> > > > @@ -540,9 +540,11 @@ static void xen_device_backend_changed(void *opaque)
> > > >      /*
> > > >       * If the toolstack (or unplug request callback) has set the backend
> > > >       * state to Closing, but there is no active frontend (i.e. the
> > > > -     * state is not Connected) then set the backend state to Closed.
> > > > +     * state is not Connected or Closing) then set the backend state
> > > > +     * to Closed.
> > > >       */
> > > >      if (xendev->backend_state == XenbusStateClosing &&
> > > > +        xendev->frontend_state != XenbusStateClosing &&
> > > >          xendev->frontend_state != XenbusStateConnected) {
> > > >          xen_device_backend_set_state(xendev, XenbusStateClosed);
> > >
> > > Actually, I wonder whether it is better to 'whitelist' here? AFAIK the only valid frontend states
> > whether the backend should set itself 'closed' are 'closed' (i.e. the frontend is finished) and
> > 'initialising' (the frontend was never there).
> > 
> > Let's see, what are the reason backend=Closing?
> >     - frontend changed to Closing (because it wants to disconnect)
> >     - toolstack(libxl) or QEMU(unplug request) set the state to Closing,
> >       but also online to 0.
> > 
> > What should the backend do in both case:
> >     - frontend alive: backend should wait
> >         frontend state might be InitWait, Initialised, Connected,
> >         Closing.
> >     - frontend not existing or disconnected: backend can skip waiting
> >       and go to the next step, Closed.
> >         frontend might be Initialising, Closed.
> >         But there are also Unknown, Reconfiguring and Reconfigured which
> >         are probably errors.
> > 
> > So, the whitelist with Closed and Initialising is a good start, but what
> > about the Unknown state? (QEMU doesn't have backends were the state
> > Reconfigur* is possible, so they can be mapped to Unknown for now).
> 
> I guess we should consider Unknown (basically a missing xenstore state key) to mean either an admin, or the frontend has screwed up or is malicious so I think we just close down the backend straight away. So maybe listing InitWait, Initialised, Connected, and Closing as frontend states that are 'good' (i.e. we wait in anticipation of the frontend eventually getting to Closed) and then say all other states result in immediate close of the backend. Probably worth having a helper function for saying whether a state is good or not.

Sounds good, but I'll use "active" instead of "good" to name the helper
as that feels more accurate to me. Also "active" is already used in the
comment. I'll name the new helper xen_device_state_is_active().

Thanks,

-- 
Anthony PERARD

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

[Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Anthony PERARD]

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);
 }
 
 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);
 
     /*
      * 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);
 
     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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Paul Durrant]

> -----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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Anthony PERARD]

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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 22 August 2019 11:22
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> 
> 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.)
> 

I guess I don't like the term 'record'... I'd really like to stick with 'set'...

> 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() ?
> 

...so I prefer the extra param.

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.

  Paul

> Thanks,
> 
> --
> Anthony PERARD

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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Anthony PERARD]

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.

Also, it's nice to avoid extra work.

-- 
Anthony PERARD

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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 22 August 2019 12:18
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> 
> 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?

  Paul

> 
> Also, it's nice to avoid extra work.
> 
> --
> Anthony PERARD

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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Anthony PERARD]

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

Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 22 August 2019 14:18
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> 
> 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'.

Thanks :-)

  Paul

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