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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 22, 2019 at 12:25:44PM +0100, Paul Durrant wrote:
> > From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel