On Wed, 20 Dec 2017 16:37:52 +0200 Victor Kaplansky <vict...@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 <vict...@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?
