[dpdk-dev] [PATCH] vhost: stop device before updating public vring data

Wodkowski, PawelX pawelx.wodkowski at intel.com
Wed Mar 7 11:59:55 CET 2018


> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> Sent: Wednesday, March 7, 2018 11:09 AM
> To: Wodkowski, PawelX <pawelx.wodkowski at intel.com>; Kulasek, TomaszX
> <tomaszx.kulasek at intel.com>; yliu at fridaylinux.org
> Cc: Verkamp, Daniel <daniel.verkamp at intel.com>; Harris, James R
> <james.r.harris at intel.com>; dev at dpdk.org; Stojaczyk, DariuszX
> <dariuszx.stojaczyk at intel.com>
> Subject: Re: [dpdk-dev] [PATCH] vhost: stop device before updating public vring
> data
> 
> 
> 
> On 03/07/2018 10:16 AM, Wodkowski, PawelX wrote:
> >> -----Original Message-----
> >> From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> >> Sent: Tuesday, March 6, 2018 5:27 PM
> >> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>; yliu at fridaylinux.org
> >> Cc: Verkamp, Daniel <daniel.verkamp at intel.com>; Harris, James R
> >> <james.r.harris at intel.com>; Wodkowski, PawelX
> >> <pawelx.wodkowski at intel.com>; dev at dpdk.org; Stojaczyk, DariuszX
> >> <dariuszx.stojaczyk at intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH] vhost: stop device before updating public
> vring
> >> data
> >>
> >> Hi Tomasz,
> >>
> >> On 03/05/2018 05:11 PM, Tomasz Kulasek wrote:
> >>> For now DPDK assumes that callfd, kickfd and last_idx are being set just
> >>> once during vring initialization and device cannot be running while DPDK
> >>> receives SET_VRING_KICK, SET_VRING_CALL and SET_VRING_BASE
> messages.
> >>> However, that assumption is wrong. For Vhost SCSI messages might arrive
> >>> at any point of time, possibly multiple times, one after another.
> >>>
> >>> QEMU issues SET_VRING_CALL once during device initialization, then again
> >>> during device start. The second message will close previous callfd,
> >>> which is still being used by the user-implementation of vhost device.
> >>> This results in writing to invalid (closed) callfd.
> >>>
> >>> Other messages like SET_FEATURES, SET_VRING_ADDR etc also will
> change
> >>> internal state of VQ or device. To prevent race condition device should
> >>> also be stopped before updateing vring data.
> >>>
> >>> Signed-off-by: Dariusz Stojaczyk<dariuszx.stojaczyk at intel.com>
> >>> Signed-off-by: Pawel Wodkowski<pawelx.wodkowski at intel.com>
> >>> Signed-off-by: Tomasz Kulasek<tomaszx.kulasek at intel.com>
> >>> ---
> >>>    lib/librte_vhost/vhost_user.c | 40
> >> ++++++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 40 insertions(+)
> >>
> >> In last release, we have introduced a per-virtqueue lock to protect
> >> vring handling against asynchronous device changes.
> >>
> >> I think that would solve the issue you are facing, but you would need
> >> to export the VQs locking functions to the vhost-user lib API to be
> >> able to use it.
> >>
> >> I don't think your current patch is the right solution anyway, because
> >> it destroys the device in case we don't want it to remain alive, like
> >> set_log_base, or set_features when only the logging feature gets
> >> enabled.
> >
> > Please correct me if I can't see something obvious, but how this lock protect
> against eg
> > SET_MEM_TABLE message? Current flow you are thinking of is:
> >
> > DPDK vhost-user thread
> > 1.1. vhost_user_lock_all_queue_pairs()
> > 1.2. vhost_user_set_mem_table()
> > 1.3. vhost_user_unlock_all_queue_pairs()
> >
> > BACKEND: virito-net:
> > 2.1. rte_spinlock_lock(&vq->access_lock);
> > 2.2. Process vrings and copy all data
> > 2.3. rte_spinlock_unlock(&vq->access_lock);
> >
> > Yes, it will synchronize access to virtio_net structure but what if the BACKEND
> is in
> > zero copy mode and/or pass buffers to physical device? The request will
> > not end in 2.2 and you unmap the memory regions in the middle of request.
> > Even worse, the physical device will just abort the request but BACKEND can
> segfault
> > or write random memory because BACKEND try to use invalid memory
> address
> > (retrieved at request start).
> 
> Right, it doesn't work with zero-copy.
> 
> > To use this per-virtqueue lock:
> > 1. the lock need to be held from request start to the end - but this can starve
> DPDK
> > vhost-user thread as there might be many request on-the-fly and when one is
> done
> > the new one might be started.
> > 2. Becouse we don't know if something changed between requst start and
> request end
> > BACKEND need walk through all descriptors chain at the request end and do
> the
> > rte_vhost_gpa_to_vva() again.
> >
> > The SET_MEM_TABLE is most obvious message but the same is true for other
> like
> > VHOST_IOTLB_INVALIDATE or SET_FEATURES.
> 
> SET_FEATURE should never be sent as soon as the device is started,
> except to enable logging.
> 
> For VHOST_IOTLB_INVALIDATE, the solution might be to have a ref counter
> per entry, and to only remove it for the cache once it is zero and send
> the reply-ack tothe master once this is done. But the cost would be huge
> as with large entries, a lot of threads might increment/decrement the
> same variable so there will be contention.
> 
> For all other cases, like SET_MEM_TABLE, maybe the solution is to
> disable/enable all the queues using the existing ops.
> The application or library would have to take care that no guest buffers
> are in the wild before returning from the disable.
> 
> Do you think that would work?

What kind of ops can be used to reliably disable all queues and inform backend what
changed beside new_device/destroy_device? Those informations are very well hidden
inside vhost.c and vhost-user.c files.

I think we need new set of ops/callbacks in vhost_device_ops struct that let the backend
decide how to behave in response on certain messages like SET_MEM_TABLE.

Also I would propose to revert the a3688046995f "vhost: protect active rings from
async ring changes" and replace it by behavior of this patch. I see now that this patch
need to handle other cases like VHOST_USER_IOTLB_MSG but until we have new set of
callbacks I think this is only reliable and fast solution.

Pawel

> 
> Cheers,
> Maxime
> 
> > Pawel
> >
> >>
> >> Cheers,
> >> Maxime


More information about the dev mailing list