[dpdk-stable] [dpdk-dev] [PATCH v4] vhost_user: protect active rings from async ring changes
Stephen Hemminger
stephen at networkplumber.org
Wed Dec 20 20:06:16 CET 2017
On Wed, 20 Dec 2017 16:37:52 +0200
Victor Kaplansky <victork at redhat.com> 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 at redhat.com>
> ---
>
> 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 | 25 +++++++++++++--
> lib/librte_vhost/vhost.c | 1 +
> lib/librte_vhost/vhost_user.c | 71 +++++++++++++++++++++++++++++++++++++++++++
> lib/librte_vhost/virtio_net.c | 28 ++++++++++++++---
> 4 files changed, 119 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 1cc81c17..f3e43e95 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -108,12 +108,14 @@ struct vhost_virtqueue {
>
> /* Backend value to determine if device should started/stopped */
> int backend;
> + int enabled;
> + int access_ok;
> + rte_spinlock_t access_lock;
Maybe these int's should be bool?
> +
> /* Used to notify the guest (trigger interrupt) */
> int callfd;
> /* Currently unused as polling mode is enabled */
> int kickfd;
> - int enabled;
> - int access_ok;
>
> /* Physical address of used ring, for logging */
> uint64_t log_guest_addr;
> @@ -302,6 +304,25 @@ vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
> vhost_log_write(dev, vq->log_guest_addr + offset, len);
> }
>
> +static __rte_always_inline int
> +vhost_user_access_trylock(struct vhost_virtqueue *vq)
> +{
> + return rte_spinlock_trylock(&vq->access_lock);
> +}
> +
> +static __rte_always_inline void
> +vhost_user_access_lock(struct vhost_virtqueue *vq)
> +{
> + rte_spinlock_lock(&vq->access_lock);
> +}
> +
> +static __rte_always_inline void
> +vhost_user_access_unlock(struct vhost_virtqueue *vq)
> +{
> + rte_spinlock_unlock(&vq->access_lock);
> +}
> +
> +
Wrapping locking inline's adds nothing and makes life harder
for static analysis tools.
The bigger problem is that doing locking on all enqueue/dequeue
can have a visible performance impact. Did you measure that?
Could you invent an RCUish mechanism using compiler barriers?
More information about the stable
mailing list