Re: [dpdk-dev] [PATCH v5] vhost_user: protect active rings from async ring changes

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 01/17/2018 02:49 PM, Victor Kaplansky wrote:
> When performing live migration or memory hot-plugging,
> the changes to the device and vrings made by message handler
> done independently from vring usage by PMD threads.
> 
> This causes for example segfaults during live-migration
> with MQ enable, but in general virtually any request
> sent by qemu changing the state of device can cause
> problems.
> 
> These patches fixes all above issues by adding a spinlock
> to every vring and requiring message handler to start operation
> only after ensuring that all PMD threads related to the device
> are out of critical section accessing the vring data.
> 
> Each vring has its own lock in order to not create contention
> between PMD threads of different vrings and to prevent
> performance degradation by scaling queue pair number.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1450680
> 
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
> v5:
>   o get rid of spinlock wrapping functions in vhost.h
> 
> v4:
> 
>   o moved access_unlock before accessing enable flag and
>     access_unlock after iommu_unlock consistently.
>   o cosmetics: removed blank line.
>   o the access_lock variable moved to be in the same
>     cache line with enable and access_ok flags.
>   o dequeue path is now guarded with trylock and returning
>     zero if unsuccessful.
>   o GET_VRING_BASE operation is not guarded by access lock
>     to avoid deadlock with device_destroy. See the comment
>     in the code.
>   o Fixed error path exit from enqueue and dequeue carefully
>     unlocking access and iommu locks as appropriate.
> 
> v3:
>     o Added locking to enqueue flow.
>     o Enqueue path guarded as well as dequeue path.
>     o Changed name of active_lock.
>     o Added initialization of guarding spinlock.
>     o Reworked functions skimming over all virt-queues.
>     o Performance measurements done by Maxime Coquelin shows
>       no degradation in bandwidth and throughput.
>     o Spelling.
>     o Taking lock only on set operations.
>     o IOMMU messages are not guarded by access lock.
> 
> v2:
>     o Fixed checkpatch complains.
>     o Added Signed-off-by.
>     o Refined placement of guard to exclude IOMMU messages.
>     o TODO: performance degradation measurement.
> 
>   lib/librte_vhost/vhost.h      |  6 ++--
>   lib/librte_vhost/vhost.c      |  1 +
>   lib/librte_vhost/vhost_user.c | 70 +++++++++++++++++++++++++++++++++++++++++++
>   lib/librte_vhost/virtio_net.c | 28 ++++++++++++++---
>   4 files changed, 99 insertions(+), 6 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>