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 14: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 12:25:44PM +0100, Paul Durrant wrote:
> > > From: Anthony PERARD <address@hidden>
> > > 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