From: "Tan, Jianfeng" <jianfeng.tan@intel.com>
To: Victor Kaplansky <vkaplans@redhat.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"yliu@fridaylinux.org" <yliu@fridaylinux.org>,
"Bie, Tiwei" <tiwei.bie@intel.com>
Cc: "stable@dpdk.org" <stable@dpdk.org>,
"jfreiman@redhat.com" <jfreiman@redhat.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>
Subject: Re: [dpdk-stable] [PATCH] vhost_user: protect active rings from async ring changes
Date: Thu, 7 Dec 2017 09:33:27 +0000 [thread overview]
Message-ID: <ED26CBA2FAD1BF48A8719AEF02201E36513789F8@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <20171206135329.652-1-vkaplans@redhat.com>
> -----Original Message-----
> From: Victor Kaplansky [mailto:vkaplans@redhat.com]
> Sent: Wednesday, December 6, 2017 9:56 PM
> To: dev@dpdk.org; yliu@fridaylinux.org; Bie, Tiwei; Tan, Jianfeng;
> vkaplans@redhat.com
> Cc: stable@dpdk.org; jfreiman@redhat.com; Maxime Coquelin
> Subject: [PATCH] vhost_user: protect active rings from async ring changes
>
> 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
segfauls ->segfaults?
> 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
Another typo: 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.
Also wonder how much overhead it brings.
Instead of locking each vring, can we just, waiting a while (10us for example) after call destroy_device() callback so that every PMD thread has enough time to skip out the criterial area?
> ---
> 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;
> } __rte_cache_aligned;
>
> /* Old kernels have no such macros defined */
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index f4c7ce46..02a3a7d3 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1190,6 +1190,46 @@ vhost_user_check_and_alloc_queue_pair(struct
> virtio_net *dev, VhostUserMsg *msg)
> return alloc_vring_queue(dev, vring_idx);
> }
>
> +static void
> +vhost_user_lock_all_queue_pairs(struct virtio_net *dev)
> +{
> + unsigned int i = 0;
> + unsigned int vq_num = 0;
> +
> + while (vq_num < dev->nr_vring) {
> + struct vhost_virtqueue *vq = dev->virtqueue[i];
> +
> + if (!vq) {
> + i++;
> + continue;
> + }
> +
> + rte_spinlock_lock(&vq->active_lock);
> + vq_num++;
> + i++;
> + }
> +}
> +
> +static void
> +vhost_user_unlock_all_queue_pairs(struct virtio_net *dev)
> +{
> + unsigned int i = 0;
> + unsigned int vq_num = 0;
> +
> + while (vq_num < dev->nr_vring) {
> + struct vhost_virtqueue *vq = dev->virtqueue[i];
> +
> + if (!vq) {
> + i++;
> + continue;
> + }
> +
> + rte_spinlock_unlock(&vq->active_lock);
> + vq_num++;
> + i++;
> + }
> +}
> +
> int
> vhost_user_msg_handler(int vid, int fd)
> {
> @@ -1241,6 +1281,8 @@ vhost_user_msg_handler(int vid, int fd)
> return -1;
> }
>
> + vhost_user_lock_all_queue_pairs(dev);
> +
> switch (msg.request.master) {
> case VHOST_USER_GET_FEATURES:
> msg.payload.u64 = vhost_user_get_features(dev);
> @@ -1342,6 +1384,8 @@ vhost_user_msg_handler(int vid, int fd)
>
> }
>
> + vhost_user_unlock_all_queue_pairs(dev);
> +
> if (msg.flags & VHOST_USER_NEED_REPLY) {
> msg.payload.u64 = !!ret;
> msg.size = sizeof(msg.payload.u64);
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 6fee16e5..de7e38bb 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -44,6 +44,7 @@
> #include <rte_udp.h>
> #include <rte_sctp.h>
> #include <rte_arp.h>
> +#include <rte_spinlock.h>
>
> #include "iotlb.h"
> #include "vhost.h"
> @@ -1180,9 +1181,12 @@ rte_vhost_dequeue_burst(int vid, uint16_t
> queue_id,
> }
>
> vq = dev->virtqueue[queue_id];
> +
Remove above blank line.
> if (unlikely(vq->enabled == 0))
> return 0;
>
> + rte_spinlock_lock(&vq->active_lock);
> +
> vq->batch_copy_nb_elems = 0;
>
> if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))
> @@ -1240,6 +1244,7 @@ rte_vhost_dequeue_burst(int vid, uint16_t
> queue_id,
> if (rarp_mbuf == NULL) {
> RTE_LOG(ERR, VHOST_DATA,
> "Failed to allocate memory for mbuf.\n");
> + rte_spinlock_unlock(&vq->active_lock);
> return 0;
> }
>
> @@ -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);
> +
> if (unlikely(rarp_mbuf != NULL)) {
> /*
> * Inject it to the head of "pkts" array, so that switch's mac
> @@ -1366,5 +1373,6 @@ rte_vhost_dequeue_burst(int vid, uint16_t
> queue_id,
> i += 1;
> }
>
> +
Remove above blank line.
> return i;
> }
> --
> Victor
next prev parent reply other threads:[~2017-12-07 9:33 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-06 13:55 Victor Kaplansky
2017-12-06 14:11 ` Yuanhan Liu
2017-12-07 9:33 ` Tan, Jianfeng [this message]
2017-12-07 10:02 ` Maxime Coquelin
2017-12-08 2:14 ` Tan, Jianfeng
2017-12-08 8:35 ` Maxime Coquelin
2017-12-08 8:51 ` Tan, Jianfeng
2017-12-08 10:11 ` Maxime Coquelin
2017-12-12 5:25 ` Tan, Jianfeng
2017-12-12 8:41 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ED26CBA2FAD1BF48A8719AEF02201E36513789F8@SHSMSX103.ccr.corp.intel.com \
--to=jianfeng.tan@intel.com \
--cc=dev@dpdk.org \
--cc=jfreiman@redhat.com \
--cc=maxime.coquelin@redhat.com \
--cc=stable@dpdk.org \
--cc=tiwei.bie@intel.com \
--cc=vkaplans@redhat.com \
--cc=yliu@fridaylinux.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).