> -----Original Message----- > From: Anthony PERARD <anthony.per...@citrix.com> > Sent: 22 August 2019 14:18 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: qemu-devel@nongnu.org; Stefano Stabellini <sstabell...@kernel.org>; > 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 <anthony.per...@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