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

[Anthony PERARD <anthony.perard@citrix.com>]

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