* [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset [not found] <20190821092020.17952-1-anthony.perard@citrix.com> @ 2019-08-21 9:20 ` Anthony PERARD 2019-08-21 9:36 ` Paul Durrant 2019-08-21 9:20 ` [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore Anthony PERARD 1 sibling, 1 reply; 13+ messages in thread From: Anthony PERARD @ 2019-08-21 9:20 UTC (permalink / raw) To: qemu-devel Cc: Anthony PERARD, xen-devel, Stefano Stabellini, qemu-stable, Paul Durrant 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset 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 0 siblings, 1 reply; 13+ messages in thread From: Paul Durrant @ 2019-08-21 9:36 UTC (permalink / raw) To: Anthony Perard, qemu-devel Cc: Anthony Perard, xen-devel, Stefano Stabellini, qemu-stable > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset 2019-08-21 9:36 ` Paul Durrant @ 2019-08-22 9:50 ` Anthony PERARD 2019-08-22 9:59 ` Paul Durrant 0 siblings, 1 reply; 13+ messages in thread From: Anthony PERARD @ 2019-08-22 9:50 UTC (permalink / raw) To: Paul Durrant; +Cc: xen-devel, Stefano Stabellini, qemu-devel, qemu-stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset 2019-08-22 9:50 ` Anthony PERARD @ 2019-08-22 9:59 ` Paul Durrant 2019-08-22 15:01 ` Anthony PERARD 0 siblings, 1 reply; 13+ messages in thread From: Paul Durrant @ 2019-08-22 9:59 UTC (permalink / raw) To: Anthony Perard; +Cc: xen-devel, Stefano Stabellini, qemu-devel, qemu-stable > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 1/2] xen-bus: Fix backend state transition on device reset 2019-08-22 9:59 ` Paul Durrant @ 2019-08-22 15:01 ` Anthony PERARD 0 siblings, 0 replies; 13+ messages in thread From: Anthony PERARD @ 2019-08-22 15:01 UTC (permalink / raw) To: Paul Durrant; +Cc: xen-devel, Stefano Stabellini, qemu-devel, qemu-stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore [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:20 ` Anthony PERARD 2019-08-21 15:40 ` Paul Durrant 1 sibling, 1 reply; 13+ messages in thread From: Anthony PERARD @ 2019-08-21 9:20 UTC (permalink / raw) To: qemu-devel; +Cc: Anthony PERARD, xen-devel, Stefano Stabellini, Paul Durrant 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 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 0 siblings, 1 reply; 13+ messages in thread From: Paul Durrant @ 2019-08-21 15:40 UTC (permalink / raw) To: Anthony Perard, qemu-devel; +Cc: Anthony Perard, xen-devel, Stefano Stabellini > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 2019-08-21 15:40 ` Paul Durrant @ 2019-08-22 10:21 ` Anthony PERARD 2019-08-22 10:36 ` Paul Durrant 0 siblings, 1 reply; 13+ messages in thread From: Anthony PERARD @ 2019-08-22 10:21 UTC (permalink / raw) To: Paul Durrant; +Cc: xen-devel, Stefano Stabellini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 2019-08-22 10:21 ` Anthony PERARD @ 2019-08-22 10:36 ` Paul Durrant 2019-08-22 11:17 ` Anthony PERARD 0 siblings, 1 reply; 13+ messages in thread From: Paul Durrant @ 2019-08-22 10:36 UTC (permalink / raw) To: Anthony Perard; +Cc: xen-devel, Stefano Stabellini, qemu-devel > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 2019-08-22 10:36 ` Paul Durrant @ 2019-08-22 11:17 ` Anthony PERARD 2019-08-22 11:25 ` Paul Durrant 0 siblings, 1 reply; 13+ messages in thread From: Anthony PERARD @ 2019-08-22 11:17 UTC (permalink / raw) To: Paul Durrant; +Cc: xen-devel, Stefano Stabellini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 2019-08-22 11:17 ` Anthony PERARD @ 2019-08-22 11:25 ` Paul Durrant 2019-08-22 13:18 ` Anthony PERARD 0 siblings, 1 reply; 13+ messages in thread From: Paul Durrant @ 2019-08-22 11:25 UTC (permalink / raw) To: Anthony Perard; +Cc: xen-devel, Stefano Stabellini, qemu-devel > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 2019-08-22 11:25 ` Paul Durrant @ 2019-08-22 13:18 ` Anthony PERARD 2019-08-22 13:21 ` Paul Durrant 0 siblings, 1 reply; 13+ messages in thread From: Anthony PERARD @ 2019-08-22 13:18 UTC (permalink / raw) To: Paul Durrant; +Cc: xen-devel, Stefano Stabellini, qemu-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore 2019-08-22 13:18 ` Anthony PERARD @ 2019-08-22 13:21 ` Paul Durrant 0 siblings, 0 replies; 13+ messages in thread From: Paul Durrant @ 2019-08-22 13:21 UTC (permalink / raw) To: Anthony Perard; +Cc: xen-devel, Stefano Stabellini, qemu-devel > -----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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-08-22 15:02 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [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 2019-08-22 13:21 ` Paul Durrant
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).