From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH] vhost_user: protect active rings from async ring changes Date: Wed, 6 Dec 2017 22:11:31 +0800 Message-ID: <20171206141131.GC17112@yliu-dev> References: <20171206135329.652-1-vkaplans@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, tiwei.bie@intel.com, jianfeng.tan@intel.com, stable@dpdk.org, jfreiman@redhat.com, Maxime Coquelin To: Victor Kaplansky Return-path: Content-Disposition: inline In-Reply-To: <20171206135329.652-1-vkaplans@redhat.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Dec 06, 2017 at 03:55:49PM +0200, 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 segfauls 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 divece > 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. Hi, Thanks for the patch! Firstly, I didn't see your SoB. I'm also more interested to know do you see any performance penalty? > --- > lib/librte_vhost/vhost.h | 1 + > lib/librte_vhost/vhost_user.c | 44 +++++++++++++++++++++++++++++++++++++++++++ > lib/librte_vhost/virtio_net.c | 8 ++++++++ > 3 files changed, 53 insertions(+) > > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > index 1cc81c17..812aeccd 100644 > --- a/lib/librte_vhost/vhost.h > +++ b/lib/librte_vhost/vhost.h > @@ -137,6 +137,7 @@ struct vhost_virtqueue { > TAILQ_HEAD(, vhost_iotlb_entry) iotlb_list; > int iotlb_cache_nr; > TAILQ_HEAD(, vhost_iotlb_entry) iotlb_pending_list; > + rte_spinlock_t active_lock; The indentation is broken. > } __rte_cache_aligned; > @@ -1356,6 +1361,8 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id, > if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) > vhost_user_iotlb_rd_unlock(vq); > > + rte_spinlock_unlock(&vq->active_lock); Ditto. --yliu