From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 19587A0C47; Tue, 6 Jul 2021 16:08:55 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F1A074128B; Tue, 6 Jul 2021 16:08:54 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id AA9684120E for ; Tue, 6 Jul 2021 16:08:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1625580532; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+RivA8vWYvNMV8Wv7PJWeXzNFg5MifpSFb7X/lebcUw=; b=OWlCm52gB0GXrGcSGbU3tfcGCM8BM7/vij9Apk181nPPWfiYG+NvcKZRmCwtmg+QdyGUun 97zJt5pBKR98SC/sgB3t1e8bOAC2rCpFWL0LUB7YW1vo13khNnYzvXBEEj35TZSvjdPK2v KKoQIrDCgGUm+JNmmRkyW6Op6qlPy6I= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-435-_lyN-RwJOMmapiMeKwtz9A-1; Tue, 06 Jul 2021 10:08:51 -0400 X-MC-Unique: _lyN-RwJOMmapiMeKwtz9A-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 44E5919251B0; Tue, 6 Jul 2021 14:08:50 +0000 (UTC) Received: from [10.36.110.36] (unknown [10.36.110.36]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 462EC5D6AD; Tue, 6 Jul 2021 14:08:39 +0000 (UTC) To: Cheng Jiang , Chenbo.Xia@intel.com Cc: dev@dpdk.org, jiayu.hu@intel.com, yvonnex.yang@intel.com References: <20210602042802.31943-1-cheng1.jiang@intel.com> <20210615141513.16163-1-cheng1.jiang@intel.com> <20210615141513.16163-2-cheng1.jiang@intel.com> From: Maxime Coquelin Message-ID: <7a71639e-b9a3-8362-cc1e-8f86179d4c0e@redhat.com> Date: Tue, 6 Jul 2021 16:08:38 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210615141513.16163-2-cheng1.jiang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/3] vhost: add unsafe API to drain pkts in async vhost X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list 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 6/15/21 4:15 PM, Cheng Jiang wrote: > Applications need to stop DMA transfers and finish all the in-flight > pkts when in VM memory hot-plug case and async vhost is used. This > patch is to provide an unsafe API to drain in-flight pkts which are > submitted to DMA engine in vhost async data path. > > Signed-off-by: Cheng Jiang > --- > lib/vhost/rte_vhost_async.h | 22 +++++++++ > lib/vhost/version.map | 3 ++ > lib/vhost/virtio_net.c | 90 +++++++++++++++++++++++++++---------- > 3 files changed, 92 insertions(+), 23 deletions(-) > > diff --git a/lib/vhost/rte_vhost_async.h b/lib/vhost/rte_vhost_async.h > index 6faa31f5ad..041f40cf04 100644 > --- a/lib/vhost/rte_vhost_async.h > +++ b/lib/vhost/rte_vhost_async.h > @@ -193,4 +193,26 @@ __rte_experimental > uint16_t rte_vhost_poll_enqueue_completed(int vid, uint16_t queue_id, > struct rte_mbuf **pkts, uint16_t count); > > +/** > + * This function checks async completion status and empty all pakcets > + * for a specific vhost device queue. Packets which are inflight will > + * be returned in an array. > + * > + * @note This function does not perform any locking > + * > + * @param vid > + * id of vhost device to enqueue data > + * @param queue_id > + * queue id to enqueue data > + * @param pkts > + * blank array to get return packet pointer > + * @param count > + * size of the packet array > + * @return > + * num of packets returned > + */ > +__rte_experimental > +uint16_t rte_vhost_drain_queue_thread_unsafe(int vid, uint16_t queue_id, > + struct rte_mbuf **pkts, uint16_t count); > + > #endif /* _RTE_VHOST_ASYNC_H_ */ > diff --git a/lib/vhost/version.map b/lib/vhost/version.map > index 9103a23cd4..f480f188af 100644 > --- a/lib/vhost/version.map > +++ b/lib/vhost/version.map > @@ -79,4 +79,7 @@ EXPERIMENTAL { > > # added in 21.05 > rte_vhost_get_negotiated_protocol_features; > + > + # added in 21.08 > + rte_vhost_drain_queue_thread_unsafe; > }; > diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c > index 8da8a86a10..793510974a 100644 > --- a/lib/vhost/virtio_net.c > +++ b/lib/vhost/virtio_net.c > @@ -2082,36 +2082,18 @@ write_back_completed_descs_packed(struct vhost_virtqueue *vq, > } while (nr_left > 0); > } > > -uint16_t rte_vhost_poll_enqueue_completed(int vid, uint16_t queue_id, > +static __rte_always_inline uint16_t > +vhost_poll_enqueue_completed(struct virtio_net *dev, uint16_t queue_id, > struct rte_mbuf **pkts, uint16_t count) > { > - struct virtio_net *dev = get_device(vid); > struct vhost_virtqueue *vq; > uint16_t n_pkts_cpl = 0, n_pkts_put = 0, n_descs = 0, n_buffers = 0; > uint16_t start_idx, pkts_idx, vq_size; > struct async_inflight_info *pkts_info; > uint16_t from, i; > > - if (!dev) > - return 0; > - > - VHOST_LOG_DATA(DEBUG, "(%d) %s\n", dev->vid, __func__); > - if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->nr_vring))) { > - VHOST_LOG_DATA(ERR, "(%d) %s: invalid virtqueue idx %d.\n", > - dev->vid, __func__, queue_id); > - return 0; > - } > - > vq = dev->virtqueue[queue_id]; > > - if (unlikely(!vq->async_registered)) { > - VHOST_LOG_DATA(ERR, "(%d) %s: async not registered for queue id %d.\n", > - dev->vid, __func__, queue_id); > - return 0; > - } > - > - rte_spinlock_lock(&vq->access_lock); > - > pkts_idx = vq->async_pkts_idx % vq->size; > pkts_info = vq->async_pkts_info; > vq_size = vq->size; > @@ -2119,14 +2101,14 @@ uint16_t rte_vhost_poll_enqueue_completed(int vid, uint16_t queue_id, > vq_size, vq->async_pkts_inflight_n); > > if (count > vq->async_last_pkts_n) > - n_pkts_cpl = vq->async_ops.check_completed_copies(vid, > + n_pkts_cpl = vq->async_ops.check_completed_copies(dev->vid, > queue_id, 0, count - vq->async_last_pkts_n); > n_pkts_cpl += vq->async_last_pkts_n; > > n_pkts_put = RTE_MIN(count, n_pkts_cpl); > if (unlikely(n_pkts_put == 0)) { > vq->async_last_pkts_n = n_pkts_cpl; > - goto done; > + return 0; > } > > if (vq_is_packed(dev)) { > @@ -2165,12 +2147,74 @@ uint16_t rte_vhost_poll_enqueue_completed(int vid, uint16_t queue_id, > vq->last_async_desc_idx_split += n_descs; > } > > -done: > + return n_pkts_put; > +} > + > +uint16_t rte_vhost_poll_enqueue_completed(int vid, uint16_t queue_id, > + struct rte_mbuf **pkts, uint16_t count) > +{ > + struct virtio_net *dev = get_device(vid); > + struct vhost_virtqueue *vq; > + uint16_t n_pkts_put = 0; > + > + if (!dev) > + return 0; > + > + VHOST_LOG_DATA(DEBUG, "(%d) %s\n", dev->vid, __func__); > + if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->nr_vring))) { > + VHOST_LOG_DATA(ERR, "(%d) %s: invalid virtqueue idx %d.\n", > + dev->vid, __func__, queue_id); > + return 0; > + } > + > + vq = dev->virtqueue[queue_id]; > + > + if (unlikely(!vq->async_registered)) { > + VHOST_LOG_DATA(ERR, "(%d) %s: async not registered for queue id %d.\n", > + dev->vid, __func__, queue_id); > + return 0; > + } > + > + rte_spinlock_lock(&vq->access_lock); > + > + n_pkts_put = vhost_poll_enqueue_completed(dev, queue_id, pkts, count); > + > rte_spinlock_unlock(&vq->access_lock); > > return n_pkts_put; > } > > +uint16_t rte_vhost_drain_queue_thread_unsafe(int vid, uint16_t queue_id, > + struct rte_mbuf **pkts, uint16_t count) > +{ > + struct virtio_net *dev = get_device(vid); > + struct vhost_virtqueue *vq; > + uint16_t n_pkts = count; > + > + if (!dev) > + return 0; > + > + VHOST_LOG_DATA(DEBUG, "(%d) %s\n", dev->vid, __func__); > + if (unlikely(!is_valid_virt_queue_idx(queue_id, 0, dev->nr_vring))) { > + VHOST_LOG_DATA(ERR, "(%d) %s: invalid virtqueue idx %d.\n", > + dev->vid, __func__, queue_id); > + return 0; > + } > + > + vq = dev->virtqueue[queue_id]; > + > + if (unlikely(!vq->async_registered)) { > + VHOST_LOG_DATA(ERR, "(%d) %s: async not registered for queue id %d.\n", > + dev->vid, __func__, queue_id); > + return 0; > + } > + > + while (count) > + count -= vhost_poll_enqueue_completed(dev, queue_id, pkts, count); I think we may want to improve the sync_ops so that .check_completed_copies() returns an int. If for some reason the DMA driver callback fails, we would poll forever. Looking more into the code, I see that ioat_check_completed_copies_cb() an return -1 (whereas it should return an unint32_t). It would lead to undefined behaviour if the failure would happen. The IOAT driver needs to be fixed, and also the callback prototype and its handling. > + > + return n_pkts; > +} > + > static __rte_always_inline uint32_t > virtio_dev_rx_async_submit(struct virtio_net *dev, uint16_t queue_id, > struct rte_mbuf **pkts, uint32_t count, >