From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH v5] vhost_user: protect active rings from async ring changes Date: Wed, 17 Jan 2018 18:14:31 +0100 Message-ID: <5988bf83-7532-5fcd-11d6-7964c7f84fe9@redhat.com> References: <20180117154925-mutt-send-email-victork@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: stable@dpdk.org, Jens Freimann , Yuanhan Liu , Tiwei Bie , "Tan, Jianfeng" , Stephen Hemminger To: Victor Kaplansky , dev@dpdk.org Return-path: In-Reply-To: <20180117154925-mutt-send-email-victork@redhat.com> Content-Language: en-US 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 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 > --- > 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