From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id AD4B1A3160
for <public@inbox.dpdk.org>; Thu, 10 Oct 2019 07:16:02 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 5232F1C030;
Thu, 10 Oct 2019 07:16:01 +0200 (CEST)
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
by dpdk.org (Postfix) with ESMTP id 7B75E1BF78
for <dev@dpdk.org>; Thu, 10 Oct 2019 07:15:59 +0200 (CEST)
X-Amp-Result: UNKNOWN
X-Amp-Original-Verdict: FILE UNKNOWN
X-Amp-File-Uploaded: False
Received: from orsmga004.jf.intel.com ([10.7.209.38])
by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
09 Oct 2019 22:15:58 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.67,279,1566889200"; d="scan'208";a="345588518"
Received: from dpdk-virtio-tbie-2.sh.intel.com (HELO ___) ([10.67.104.74])
by orsmga004.jf.intel.com with ESMTP; 09 Oct 2019 22:15:56 -0700
Date: Thu, 10 Oct 2019 13:12:57 +0800
From: Tiwei Bie <tiwei.bie@intel.com>
To: Flavio Leitner <fbl@sysclose.org>
Cc: dev@dpdk.org, Ilya Maximets <i.maximets@ovn.org>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
Shahaf Shuler <shahafs@mellanox.com>,
David Marchand <david.marchand@redhat.com>,
Obrembski MichalX <michalx.obrembski@intel.com>,
Stokes Ian <ian.stokes@intel.com>
Message-ID: <20191010051257.GA24244@___>
References: <20191001221935.12140-1-fbl@sysclose.org>
<20191004201008.3981-1-fbl@sysclose.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20191004201008.3981-1-fbl@sysclose.org>
User-Agent: Mutt/1.9.4 (2018-02-28)
Subject: Re: [dpdk-dev] [PATCH v2] vhost: add support for large buffers.
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
[PATCH v2] vhost: add support for large buffers.
There is a warning reported by devtools/check-git-log.sh
The '.' at the end of the title should be dropped.
On Fri, Oct 04, 2019 at 05:10:08PM -0300, Flavio Leitner wrote:
> The rte_vhost_dequeue_burst supports two ways of dequeuing data.
> If the data fits into a buffer, then all data is copied and a
> single linear buffer is returned. Otherwise it allocates
> additional mbufs and chains them together to return a multiple
> segments mbuf.
>
> While that covers most use cases, it forces applications that
> need to work with larger data sizes to support multiple segments
> mbufs. The non-linear characteristic brings complexity and
> performance implications to the application.
>
> To resolve the issue, let the host provide during registration
> if attaching an external buffer to pktmbuf is supported and if
> only linear buffer are supported.
>
> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> ---
> doc/guides/prog_guide/vhost_lib.rst | 35 ++++++++++
> lib/librte_vhost/rte_vhost.h | 4 ++
> lib/librte_vhost/socket.c | 10 +++
> lib/librte_vhost/vhost.c | 22 ++++++
> lib/librte_vhost/vhost.h | 4 ++
> lib/librte_vhost/virtio_net.c | 103 ++++++++++++++++++++++++++--
> 6 files changed, 172 insertions(+), 6 deletions(-)
>
> - Changelog:
> V2:
> - Used rte_malloc() instead of another mempool as suggested by Shahaf.
> - Added the documentation section.
> - Using driver registration to negotiate the features.
> - OvS PoC code:
> https://github.com/fleitner/ovs/commit/8fc197c40b1d4fda331686a7b919e9e2b670dda7
>
> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index fc3ee4353..07e40e3c5 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -117,6 +117,41 @@ The following is an overview of some key Vhost API functions:
> Enabling this flag should only be done when the calling application does
> not pre-fault the guest shared memory, otherwise migration would fail.
>
> + - ``RTE_VHOST_USER_LINEARBUF_SUPPORT``
> +
> + Enabling this flag forces vhost dequeue function to only provide linear
> + pktmbuf (no multi-segmented pktmbuf).
> +
> + The vhost library by default provides a single pktmbuf for given a
> + packet, but if for some reason the data doesn't fit into a single
> + pktmbuf (e.g., TSO is enabled), the library will allocate additional
> + pktmbufs from the same mempool and chain them together to create a
> + multi-segmented pktmbuf.
> +
> + However, the vhost application needs to support multi-segmented format.
> + If the vhost application does not support that format and requires large
> + buffers to be dequeue, this flag should be enabled to force only linear
> + buffers (see RTE_VHOST_USER_EXTBUF_SUPPORT) or drop the packet.
> +
> + It is disabled by default.
> +
> + - ``RTE_VHOST_USER_EXTBUF_SUPPORT``
> +
> + Enabling this flag allows vhost dequeue function to allocate and attach
> + an external buffer to a pktmbuf if the pkmbuf doesn't provide enough
> + space to store all data.
> +
> + This is useful when the vhost application wants to support large packets
> + but doesn't want to increase the default mempool object size nor to
> + support multi-segmented mbufs (non-linear). In this case, a fresh buffer
> + is allocated using rte_malloc() which gets attached to a pktmbuf using
> + rte_pktmbuf_attach_extbuf().
> +
> + See RTE_VHOST_USER_LINEARBUF_SUPPORT as well to disable multi-segmented
> + mbufs for application that doesn't support chained mbufs.
> +
> + It is disabled by default.
> +
> * ``rte_vhost_driver_set_features(path, features)``
>
> This function sets the feature bits the vhost-user driver supports. The
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 19474bca0..b821b5df4 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -30,6 +30,10 @@ extern "C" {
> #define RTE_VHOST_USER_DEQUEUE_ZERO_COPY (1ULL << 2)
> #define RTE_VHOST_USER_IOMMU_SUPPORT (1ULL << 3)
> #define RTE_VHOST_USER_POSTCOPY_SUPPORT (1ULL << 4)
> +/* support mbuf with external buffer attached */
> +#define RTE_VHOST_USER_EXTBUF_SUPPORT (1ULL << 5)
> +/* support only linear buffers (no chained mbufs) */
> +#define RTE_VHOST_USER_LINEARBUF_SUPPORT (1ULL << 6)
>
> /** Protocol features. */
> #ifndef VHOST_USER_PROTOCOL_F_MQ
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 274988c4d..0ba610fda 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -40,6 +40,8 @@ struct vhost_user_socket {
> bool dequeue_zero_copy;
> bool iommu_support;
> bool use_builtin_virtio_net;
> + bool extbuf;
> + bool linearbuf;
>
> /*
> * The "supported_features" indicates the feature bits the
> @@ -232,6 +234,12 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
> if (vsocket->dequeue_zero_copy)
> vhost_enable_dequeue_zero_copy(vid);
>
> + if (vsocket->extbuf)
> + vhost_enable_extbuf(vid);
> +
> + if (vsocket->linearbuf)
> + vhost_enable_linearbuf(vid);
> +
> RTE_LOG(INFO, VHOST_CONFIG, "new device, handle is %d\n", vid);
>
> if (vsocket->notify_ops->new_connection) {
> @@ -870,6 +878,8 @@ rte_vhost_driver_register(const char *path, uint64_t flags)
> goto out_free;
> }
> vsocket->dequeue_zero_copy = flags & RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
> + vsocket->extbuf = flags & RTE_VHOST_USER_EXTBUF_SUPPORT;
> + vsocket->linearbuf = flags & RTE_VHOST_USER_LINEARBUF_SUPPORT;
It could be problematic to use dequeue_zero_copy with
above two features at the same time.
We may want to reject such combination.
>
> /*
> * Set the supported features correctly for the builtin vhost-user
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index cea44df8c..77457f538 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -605,6 +605,28 @@ vhost_set_builtin_virtio_net(int vid, bool enable)
> dev->flags &= ~VIRTIO_DEV_BUILTIN_VIRTIO_NET;
> }
>
> +void
> +vhost_enable_extbuf(int vid)
> +{
> + struct virtio_net *dev = get_device(vid);
> +
> + if (dev == NULL)
> + return;
> +
> + dev->extbuf = 1;
> +}
> +
> +void
> +vhost_enable_linearbuf(int vid)
> +{
> + struct virtio_net *dev = get_device(vid);
> +
> + if (dev == NULL)
> + return;
> +
> + dev->linearbuf = 1;
> +}
> +
> int
> rte_vhost_get_mtu(int vid, uint16_t *mtu)
> {
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 5131a97a3..0346bd118 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -302,6 +302,8 @@ struct virtio_net {
> rte_atomic16_t broadcast_rarp;
> uint32_t nr_vring;
> int dequeue_zero_copy;
> + int extbuf;
> + int linearbuf;
> struct vhost_virtqueue *virtqueue[VHOST_MAX_QUEUE_PAIRS * 2];
> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
> char ifname[IF_NAME_SZ];
> @@ -476,6 +478,8 @@ void vhost_attach_vdpa_device(int vid, int did);
> void vhost_set_ifname(int, const char *if_name, unsigned int if_len);
> void vhost_enable_dequeue_zero_copy(int vid);
> void vhost_set_builtin_virtio_net(int vid, bool enable);
> +void vhost_enable_extbuf(int vid);
> +void vhost_enable_linearbuf(int vid);
>
> struct vhost_device_ops const *vhost_driver_callback_get(const char *path);
>
> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
> index 5b85b832d..fca75161d 100644
> --- a/lib/librte_vhost/virtio_net.c
> +++ b/lib/librte_vhost/virtio_net.c
> @@ -2,6 +2,7 @@
> * Copyright(c) 2010-2016 Intel Corporation
> */
>
> +#include <assert.h>
> #include <stdint.h>
> #include <stdbool.h>
> #include <linux/virtio_net.h>
> @@ -1289,6 +1290,96 @@ get_zmbuf(struct vhost_virtqueue *vq)
> return NULL;
> }
>
> +static void
> +virtio_dev_extbuf_free(void *addr __rte_unused, void *opaque)
> +{
> + rte_free(opaque);
> +}
> +
> +static int
> +virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint16_t size)
> +{
> + struct rte_mbuf_ext_shared_info *shinfo;
> + uint16_t buf_len;
It also includes RTE_PKTMBUF_HEADROOM and sizeof(*shinfo).
Is uint16_t big enough?
> + rte_iova_t iova;
> + void *buf;
> +
> + shinfo = NULL;
> + buf_len = size + RTE_PKTMBUF_HEADROOM;
> +
> + /* Try to use pkt buffer to store shinfo to reduce the amount of memory
> + * required, otherwise store shinfo in the new buffer.
> + */
> + if (rte_pktmbuf_tailroom(pkt) > sizeof(*shinfo))
> + shinfo = rte_pktmbuf_mtod(pkt,
> + struct rte_mbuf_ext_shared_info *);
> + else {
> + if (unlikely(buf_len + sizeof(shinfo) > UINT16_MAX)) {
Should be sizeof(*shinfo)
> + RTE_LOG(ERR, VHOST_DATA,
> + "buffer size exceeded maximum.\n");
> + return -ENOSPC;
> + }
> +
> + buf_len += sizeof(shinfo);
Ditto.
> + }
> +
> + buf = rte_malloc(NULL, buf_len, RTE_CACHE_LINE_SIZE);
> + if (unlikely(buf == NULL)) {
> + RTE_LOG(ERR, VHOST_DATA,
> + "Failed to allocate memory for mbuf.\n");
Looks better to just have 3 tabs before the string.
> + return -ENOMEM;
> + }
> +
> + /* initialize shinfo */
> + if (shinfo) {
> + shinfo->free_cb = virtio_dev_extbuf_free;
> + shinfo->fcb_opaque = buf;
> + rte_mbuf_ext_refcnt_set(shinfo, 1);
> + } else {
> + shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len,
> + virtio_dev_extbuf_free, buf);
> + assert(shinfo);
As a library, we shouldn't abort the process.
We can just return the error.
> + }
> +
> + iova = rte_mempool_virt2iova(buf);
Should be rte_malloc_virt2iova().
> + rte_pktmbuf_attach_extbuf(pkt, buf, iova, buf_len, shinfo);
> + rte_pktmbuf_reset_headroom(pkt);
> + assert(pkt->ol_flags == EXT_ATTACHED_MBUF);
Ditto.
> +
> + return 0;
> +}
> +
> +/*
> + * Allocate a host supported pktmbuf.
> + */
> +static __rte_always_inline struct rte_mbuf *
> +virtio_dev_pktmbuf_alloc(struct virtio_net *dev, struct rte_mempool *mp,
> + uint16_t data_len)
> +{
> + struct rte_mbuf *pkt = rte_pktmbuf_alloc(mp);
> +
> + if (unlikely(pkt == NULL))
> + return NULL;
> +
> + if (rte_pktmbuf_tailroom(pkt) >= data_len)
> + return pkt;
> +
> + /* attach an external buffer if supported */
> + if (dev->extbuf && !virtio_dev_extbuf_alloc(pkt, data_len))
> + return pkt;
> +
> + /* check if chained buffers are allowed */
> + if (!dev->linearbuf)
> + return pkt;
> +
> + /* Data doesn't fit into the buffer and the host supports
> + * only linear buffers
> + */
> + rte_pktmbuf_free(pkt);
> +
> + return NULL;
> +}
> +
> static __rte_noinline uint16_t
> virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
> struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts, uint16_t count)
> @@ -1343,21 +1434,21 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
> for (i = 0; i < count; i++) {
> struct buf_vector buf_vec[BUF_VECTOR_MAX];
> uint16_t head_idx;
> - uint32_t dummy_len;
> + uint32_t buf_len;
> uint16_t nr_vec = 0;
> int err;
>
> if (unlikely(fill_vec_buf_split(dev, vq,
> vq->last_avail_idx + i,
> &nr_vec, buf_vec,
> - &head_idx, &dummy_len,
> + &head_idx, &buf_len,
> VHOST_ACCESS_RO) < 0))
> break;
>
> if (likely(dev->dequeue_zero_copy == 0))
> update_shadow_used_ring_split(vq, head_idx, 0);
>
> - pkts[i] = rte_pktmbuf_alloc(mbuf_pool);
> + pkts[i] = virtio_dev_pktmbuf_alloc(dev, mbuf_pool, buf_len);
> if (unlikely(pkts[i] == NULL)) {
> RTE_LOG(ERR, VHOST_DATA,
> "Failed to allocate memory for mbuf.\n");
> @@ -1451,14 +1542,14 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
> for (i = 0; i < count; i++) {
> struct buf_vector buf_vec[BUF_VECTOR_MAX];
> uint16_t buf_id;
> - uint32_t dummy_len;
> + uint32_t buf_len;
> uint16_t desc_count, nr_vec = 0;
> int err;
>
> if (unlikely(fill_vec_buf_packed(dev, vq,
> vq->last_avail_idx, &desc_count,
> buf_vec, &nr_vec,
> - &buf_id, &dummy_len,
> + &buf_id, &buf_len,
> VHOST_ACCESS_RO) < 0))
> break;
>
> @@ -1466,7 +1557,7 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
> update_shadow_used_ring_packed(vq, buf_id, 0,
> desc_count);
>
> - pkts[i] = rte_pktmbuf_alloc(mbuf_pool);
> + pkts[i] = virtio_dev_pktmbuf_alloc(dev, mbuf_pool, buf_len);
> if (unlikely(pkts[i] == NULL)) {
> RTE_LOG(ERR, VHOST_DATA,
> "Failed to allocate memory for mbuf.\n");
> --
> 2.20.1
>