Re: [Qemu-devel] [PATCH] vhost-user spec: Clarify policy on setting log_base

[Michael S. Tsirkin]

On Tue, Nov 28, 2017 at 01:34:19PM -0500, Victor Kaplansky wrote:
> > From: "Michael S. Tsirkin" <address@hidden>
> > To: "Victor Kaplansky" <address@hidden>
> > Cc: address@hidden, "Maxime Coquelin" <address@hidden>, "Jason Wang" 
> > <address@hidden>
> > Sent: Tuesday, November 28, 2017 8:06:52 PM
> > Subject: Re: [PATCH] vhost-user spec: Clarify policy on setting log_base
> > 
> > On Tue, Nov 28, 2017 at 03:46:44PM +0200, Victor Kaplansky wrote:
> > > From: Victor Kaplansky <address@hidden>
> > > 
> > > If we allow qemu to change logging area after it was already established,
> > > it may require from the backend to acquire a lock on each access to
> > > the log_base, which has a potential quite a big performance hit.
> > > 
> > > Thus we would like to clarify in the spec, that qemu is not expected
> > > to resize or remap the logging area, and backend implementations
> > > can safely ignore subsequent requests to log_base modifications.
> > > 
> > > Signed-off-by: Victor Kaplansky <address@hidden>
> > > Suggested-by: Maxime Coquelin <address@hidden>
> > 
> > I'm not sure we can do this.
> > 
> > 
> > Log resizing as a result of memory hotplug might force
> > log base changes.
> 
> I agree. Memory hotplug during live migration will cause a
> bunch of problems not only to dirty page logging, but to a
> regular backend function, because memory regions have to be
> redefined before descriptors pointing to the new memory
> arrive a backend. Which means, that in DPDK we must to
> guard by some kind of synchronization mechanism all accesses
> to guest2qemu memory translation. This will have an impact
> on the backend performace even with light-weighted RCU.
> 
> So, I propose to handle memory hot plug by stopping rings,
> then changing memory regions and log_base, and then re-enabling
> rings.
> 
> What do you say?

Why not have backend do this internally when it gets
the log_base message?


> 
> > 
> > Backends need to use something like
> > rcu to avoid need for locking.
> > 
> > Apropos I wonder whether it's a bug that vhost_dev_start
> > calls vhost_set_log_base after starting rings.
> 
> In vhost-user backend before vhost_user_set_log_base is called for first
> time, log_base is initialized to zero. Which causes backend (DPDK) to
> skip logging. Thus theoretically setting log_base on an active ring,
> may cause to skip single (very first) dirty logging. In the reality,
> it has very small chances to happen, since sending the response from
> the backend to qemu by a socket takes much longer then a time gap
> between reading log_base variable and using it.

I suspect it's worth fixing though.


> 
> > 
> > Same question for the iotlb callback.
> > 
> > 
> > 
> > 
> > > ---
> > >  docs/interop/vhost-user.txt | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > > index 954771d0d8..7ab31e57ef 100644
> > > --- a/docs/interop/vhost-user.txt
> > > +++ b/docs/interop/vhost-user.txt
> > > @@ -257,6 +257,12 @@ Where addr is the guest physical address.
> > >  
> > >  Use atomic operations, as the log may be concurrently manipulated.
> > >  
> > > +Note that master is not expected to issue more than one
> > > VHOST_USER_SET_LOG_BASE
> > > +request before the rings are fully stopped by the master. Thus no
> > > modifications
> > > +to log_base address are allowed before the rings are restated and the
> > > client
> > > +can ignore all subsequent VHOST_USER_SET_LOG_BASE requests after the
> > > log_base
> > > +address has been established.
> > > +
> > >  Note that when logging modifications to the used ring (when
> > >  VHOST_VRING_F_LOG
> > >  is set for this ring), log_guest_addr should be used to calculate the log
> > >  offset: the write to first byte of the used ring is logged at this offset
> > >  from
> > > --
> > > 2.14.2
> >