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: sta...@dpdk.org
> Cc: f...@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 <sivaprasad.tumm...@intel.com>
> ---
> 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
