From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4A9CCA04AE; Mon, 4 May 2020 21:32:47 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9252F1D529; Mon, 4 May 2020 21:32:46 +0200 (CEST) Received: from sysclose.org (smtp.sysclose.org [69.164.214.230]) by dpdk.org (Postfix) with ESMTP id 1476B1D149; Mon, 4 May 2020 21:32:45 +0200 (CEST) Received: from localhost (unknown [45.71.104.119]) by sysclose.org (Postfix) with ESMTPSA id A50DD3058; Mon, 4 May 2020 19:33:17 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 sysclose.org A50DD3058 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sysclose.org; s=201903; t=1588620797; bh=Tb2xQ0uIZFcPXF5cAvHI81pi+Bm76aIYKCTUPE4hL4Y=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=N/UbzmUVOUoTfJJDvCh6dTpQvJvjJZ/cl1WPeQwTJP63jUfyecJYMbFoXpCi1gQL5 ziVuUeo3I12Ou3e9qucuq9/Lt4hPdAWLG+8pEvN436AqiK+E4Ef1KKE7aTHtg9BMTz WINIKyrGjVacQzFUFgduJclz7MFfdbk9angFolqFdIxa/fLJthb9iq39oxd1HnuJRN Zi6zqblPgQhmtCYDOIhoReosY69RVGKL1xxOaUGAp/XfXCD5IiQgemPwOdAUB/DWbG qcJVwpLxGhVeteLz0o0oVYU2f80vCC4MX4qo+LjdzCQI/kuFzify5LTbnj+VtKuvvb qZLM7Q2Q7aHqA== Date: Mon, 4 May 2020 16:32:40 -0300 From: Flavio Leitner To: Sivaprasad Tummala Cc: Maxime Coquelin , Zhihong Wang , Xiaolong Ye , dev@dpdk.org, stable@dpdk.org Message-ID: <20200504193240.GA92333@p50.lan> References: <20200428095203.64935-1-Sivaprasad.Tummala@intel.com> <20200504171118.93782-1-Sivaprasad.Tummala@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200504171118.93782-1-Sivaprasad.Tummala@intel.com> Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix mbuf alloc failure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 Mon, May 04, 2020 at 10:41:17PM +0530, Sivaprasad Tummala wrote: > vhost buffer allocation is successful for packets that fit > into a linear buffer. If it fails, vhost library is expected > to drop the current packet and skip to the next. > > The patch fixes the error scenario by skipping to next packet. > Note: Drop counters are not currently supported. > > Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer") > Cc: stable@dpdk.org > Cc: fbl@sysclose.org > > --- > v2: > * fixed review comments - Maxime Coquelin > * fixed mbuf alloc errors for packed virtqueues - Maxime Coquelin > * fixed mbuf copy errors - Flavio Leitner > > Signed-off-by: Sivaprasad Tummala > --- > lib/librte_vhost/virtio_net.c | 50 ++++++++++++++++++++++++++--------- > 1 file changed, 37 insertions(+), 13 deletions(-) > > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c > index 1fc30c681..764c514fd 100644 > --- a/lib/librte_vhost/virtio_net.c > +++ b/lib/librte_vhost/virtio_net.c > @@ -1674,6 +1674,7 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq, > { > uint16_t i; > uint16_t free_entries; > + uint16_t dropped = 0; > > if (unlikely(dev->dequeue_zero_copy)) { > struct zcopy_mbuf *zmbuf, *next; > @@ -1737,13 +1738,31 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq, > update_shadow_used_ring_split(vq, head_idx, 0); > > pkts[i] = virtio_dev_pktmbuf_alloc(dev, mbuf_pool, buf_len); > - if (unlikely(pkts[i] == NULL)) > + if (unlikely(pkts[i] == NULL)) { > + /* > + * mbuf allocation fails for jumbo packets when external > + * buffer allocation is not allowed and linear buffer > + * is required. Drop this packet. > + */ > +#ifdef RTE_LIBRTE_VHOST_DEBUG > + VHOST_LOG_DATA(ERR, > + "Failed to allocate memory for mbuf. Packet dropped!\n"); > +#endif That message is useful to spot that missing packets that happens once in a while, so we should be able to see it even in production without debug enabled. However, we can't let it flood the log. I am not sure if librte eal has this functionality, but if not you could limit by using a static bool: static bool allocerr_warned = false; if (allocerr_warned) { VHOST_LOG_DATA(ERR, "Failed to allocate memory for mbuf. Packet dropped!\n"); allocerr_warned = true; } > + dropped += 1; > + i++; > break; > + } > > err = copy_desc_to_mbuf(dev, vq, buf_vec, nr_vec, pkts[i], > mbuf_pool); > if (unlikely(err)) { > rte_pktmbuf_free(pkts[i]); > +#ifdef RTE_LIBRTE_VHOST_DEBUG > + VHOST_LOG_DATA(ERR, > + "Failed to copy desc to mbuf. Packet dropped!\n"); > +#endif Same here. > + dropped += 1; > + i++; > break; > } > > @@ -1753,6 +1772,8 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq, > zmbuf = get_zmbuf(vq); > if (!zmbuf) { > rte_pktmbuf_free(pkts[i]); > + dropped += 1; > + i++; > break; > } > zmbuf->mbuf = pkts[i]; > @@ -1782,7 +1803,7 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq, > } > } > > - return i; > + return (i - dropped); > } > > static __rte_always_inline int > @@ -1946,21 +1967,24 @@ virtio_dev_tx_single_packed(struct virtio_net *dev, > struct rte_mbuf **pkts) > { > > - uint16_t buf_id, desc_count; > + uint16_t buf_id, desc_count = 0; > + int ret; > > - if (vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id, > - &desc_count)) > - return -1; > + ret = vhost_dequeue_single_packed(dev, vq, mbuf_pool, pkts, &buf_id, > + &desc_count); > > - if (virtio_net_is_inorder(dev)) > - vhost_shadow_dequeue_single_packed_inorder(vq, buf_id, > - desc_count); > - else > - vhost_shadow_dequeue_single_packed(vq, buf_id, desc_count); > + if (likely(desc_count > 0)) { The vhost_dequeue_single_packed() could return -1 with desc_count > 0 and this change doesn't handle that. Thanks, fbl > + if (virtio_net_is_inorder(dev)) > + vhost_shadow_dequeue_single_packed_inorder(vq, buf_id, > + desc_count); > + else > + vhost_shadow_dequeue_single_packed(vq, buf_id, > + desc_count); > > - vq_inc_last_avail_packed(vq, desc_count); > + vq_inc_last_avail_packed(vq, desc_count); > + } > > - return 0; > + return ret; > } > > static __rte_always_inline int > -- > 2.17.1 > -- fbl