Re: [Qemu-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> -Original Message- > From: Anthony PERARD > Sent: 22 August 2019 14:18 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; Stefano Stabellini ; > xen-de...@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 > > > 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
Re: [Qemu-devel] [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 > > 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
Re: [Qemu-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> -Original Message- > From: Anthony PERARD > Sent: 22 August 2019 12:18 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; Stefano Stabellini ; > xen-de...@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
Re: [Qemu-devel] [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. Also, it's nice to avoid extra work. -- Anthony PERARD
Re: [Qemu-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> -Original Message-
> From: Anthony PERARD
> Sent: 22 August 2019 11:22
> To: Paul Durrant
> Cc: qemu-devel@nongnu.org; Stefano Stabellini ;
> xen-de...@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
> > > Sent: 21 August 2019 10:20
> > > To: qemu-devel@nongnu.org
> > > Cc: Anthony Perard ; Stefano Stabellini
> > > ; Paul
> > > Durrant ; xen-de...@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
> > > ---
> > > 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 th
Re: [Qemu-devel] [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
> > Sent: 21 August 2019 10:20
> > To: qemu-devel@nongnu.org
> > Cc: Anthony Perard ; Stefano Stabellini
> > ; Paul
> > Durrant ; xen-de...@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
> > ---
> > 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
Re: [Qemu-devel] [PATCH 2/2] xen-bus: Avoid rewriting identical values to xenstore
> -Original Message-
> From: Anthony PERARD
> Sent: 21 August 2019 10:20
> To: qemu-devel@nongnu.org
> Cc: Anthony Perard ; Stefano Stabellini
> ; Paul
> Durrant ; xen-de...@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
> ---
> 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 == XenbusStateInitiali
