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

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD <address@hidden>
> Sent: 22 August 2019 12:18
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; Stefano Stabellini <address@hidden>; address@hidden
> 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