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

[Victor Kaplansky]

> 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?


> 
> 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.


> 
> 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
>