[PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path

[PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path

[Maxime Coquelin]

When vIOMMU is enabled and Virtio device is bound to kernel
driver in guest, rte_vhost_dequeue_burst() will often return
early because of IOTLB misses.

This patch fixes a mbuf leak occurring in this case.

Fixes: 242695f6122a ("vhost: allocate and free packets in bulk in Tx split")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: David Marchand <david.marchand@redhat.com>
---

Changes in v2:
==============
- Fix descriptors leak (David)
- Rebased on top of next-virtio

---
 lib/vhost/virtio_net.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index c738b7edc9..9951842b9f 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -3104,7 +3104,6 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 {
 	uint16_t i;
 	uint16_t avail_entries;
-	uint16_t dropped = 0;
 	static bool allocerr_warned;
 
 	/*
@@ -3143,11 +3142,8 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 		update_shadow_used_ring_split(vq, head_idx, 0);
 
-		if (unlikely(buf_len <= dev->vhost_hlen)) {
-			dropped += 1;
-			i++;
+		if (unlikely(buf_len <= dev->vhost_hlen))
 			break;
-		}
 
 		buf_len -= dev->vhost_hlen;
 
@@ -3164,8 +3160,6 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 					buf_len, mbuf_pool->name);
 				allocerr_warned = true;
 			}
-			dropped += 1;
-			i++;
 			break;
 		}
 
@@ -3176,27 +3170,21 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 				VHOST_DATA_LOG(dev->ifname, ERR, "failed to copy desc to mbuf.");
 				allocerr_warned = true;
 			}
-			dropped += 1;
-			i++;
 			break;
 		}
-
 	}
 
-	if (dropped)
-		rte_pktmbuf_free_bulk(&pkts[i - 1], count - i + 1);
+	if (unlikely(count != i))
+		rte_pktmbuf_free_bulk(&pkts[i], count - i);
 
-	vq->last_avail_idx += i;
-
-	do_data_copy_dequeue(vq);
-	if (unlikely(i < count))
-		vq->shadow_used_idx = i;
 	if (likely(vq->shadow_used_idx)) {
+		vq->last_avail_idx += vq->shadow_used_idx;
+		do_data_copy_dequeue(vq);
 		flush_shadow_used_ring_split(dev, vq);
 		vhost_vring_call_split(dev, vq);
 	}
 
-	return (i - dropped);
+	return i;
 }
 
 __rte_noinline
-- 
2.43.0

[PATCH v2 2/2] vhost: add new mbuf allocation failure statistic

[Maxime Coquelin]

This patch introduces a new, per virtqueue, mbuf allocation
failure statistic. It can be useful to troubleshoot packets
drops due to insufficient mempool size or memory leaks.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost.c      |  1 +
 lib/vhost/vhost.h      |  1 +
 lib/vhost/virtio_net.c | 17 +++++++++++++----
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 5912a42979..ac71d17784 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -55,6 +55,7 @@ static const struct vhost_vq_stats_name_off vhost_vq_stat_strings[] = {
 	{"iotlb_misses",           offsetof(struct vhost_virtqueue, stats.iotlb_misses)},
 	{"inflight_submitted",     offsetof(struct vhost_virtqueue, stats.inflight_submitted)},
 	{"inflight_completed",     offsetof(struct vhost_virtqueue, stats.inflight_completed)},
+	{"mbuf_alloc_failed",      offsetof(struct vhost_virtqueue, stats.mbuf_alloc_failed)},
 };
 
 #define VHOST_NB_VQ_STATS RTE_DIM(vhost_vq_stat_strings)
diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 16fe6d4ddb..82ea25b3e1 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -156,6 +156,7 @@ struct virtqueue_stats {
 	uint64_t iotlb_misses;
 	uint64_t inflight_submitted;
 	uint64_t inflight_completed;
+	uint64_t mbuf_alloc_failed;
 	uint64_t guest_notifications_suppressed;
 	/* Counters below are atomic, and should be incremented as such. */
 	RTE_ATOMIC(uint64_t) guest_notifications;
diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 9951842b9f..1359c5fb1f 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2996,6 +2996,7 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		if (mbuf_avail == 0) {
 			cur = rte_pktmbuf_alloc(mbuf_pool);
 			if (unlikely(cur == NULL)) {
+				vq->stats.mbuf_alloc_failed++;
 				VHOST_DATA_LOG(dev->ifname, ERR,
 					"failed to allocate memory for mbuf.");
 				goto error;
@@ -3123,8 +3124,10 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	count = RTE_MIN(count, avail_entries);
 	VHOST_DATA_LOG(dev->ifname, DEBUG, "about to dequeue %u buffers", count);
 
-	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts, count))
+	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts, count)) {
+		vq->stats.mbuf_alloc_failed += count;
 		return 0;
+	}
 
 	for (i = 0; i < count; i++) {
 		struct buf_vector buf_vec[BUF_VECTOR_MAX];
@@ -3494,8 +3497,10 @@ virtio_dev_tx_packed(struct virtio_net *dev,
 {
 	uint32_t pkt_idx = 0;
 
-	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts, count))
+	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts, count)) {
+		vq->stats.mbuf_alloc_failed += count;
 		return 0;
+	}
 
 	do {
 		rte_prefetch0(&vq->desc_packed[vq->last_avail_idx]);
@@ -3745,8 +3750,10 @@ virtio_dev_tx_async_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	count = RTE_MIN(count, avail_entries);
 	VHOST_DATA_LOG(dev->ifname, DEBUG, "about to dequeue %u buffers", count);
 
-	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts_prealloc, count))
+	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts_prealloc, count)) {
+		vq->stats.mbuf_alloc_failed += count;
 		goto out;
+	}
 
 	for (pkt_idx = 0; pkt_idx < count; pkt_idx++) {
 		uint16_t head_idx = 0;
@@ -4035,8 +4042,10 @@ virtio_dev_tx_async_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	async_iter_reset(async);
 
-	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts_prealloc, count))
+	if (rte_pktmbuf_alloc_bulk(mbuf_pool, pkts_prealloc, count)) {
+		vq->stats.mbuf_alloc_failed += count;
 		goto out;
+	}
 
 	do {
 		struct rte_mbuf *pkt = pkts_prealloc[pkt_idx];
-- 
2.43.0

Re: [PATCH v2 2/2] vhost: add new mbuf allocation failure statistic

[David Marchand]

On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch introduces a new, per virtqueue, mbuf allocation
> failure statistic. It can be useful to troubleshoot packets
> drops due to insufficient mempool size or memory leaks.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Having a stat for such situation will be useful.

I just have one comment, though it is not really related to this change itself.

[snip]

> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 9951842b9f..1359c5fb1f 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2996,6 +2996,7 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>                 if (mbuf_avail == 0) {
>                         cur = rte_pktmbuf_alloc(mbuf_pool);
>                         if (unlikely(cur == NULL)) {
> +                               vq->stats.mbuf_alloc_failed++;
>                                 VHOST_DATA_LOG(dev->ifname, ERR,
>                                         "failed to allocate memory for mbuf.");

This error log here is scary as it means the datapath can be slowed
down for each multisegment mbuf in the event of a mbuf (maybe
temporary) shortage.
Besides no other mbuf allocation in the vhost library datapath
generates such log.

I would remove it, probably in a separate patch.
WDYT?


>                                 goto error;


-- 
David Marchand

Re: [PATCH v2 2/2] vhost: add new mbuf allocation failure statistic

[Maxime Coquelin]

On 2/1/24 09:10, David Marchand wrote:
> On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch introduces a new, per virtqueue, mbuf allocation
>> failure statistic. It can be useful to troubleshoot packets
>> drops due to insufficient mempool size or memory leaks.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Having a stat for such situation will be useful.
> 
> I just have one comment, though it is not really related to this change itself.
> 
> [snip]
> 
>> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
>> index 9951842b9f..1359c5fb1f 100644
>> --- a/lib/vhost/virtio_net.c
>> +++ b/lib/vhost/virtio_net.c
>> @@ -2996,6 +2996,7 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>>                  if (mbuf_avail == 0) {
>>                          cur = rte_pktmbuf_alloc(mbuf_pool);
>>                          if (unlikely(cur == NULL)) {
>> +                               vq->stats.mbuf_alloc_failed++;
>>                                  VHOST_DATA_LOG(dev->ifname, ERR,
>>                                          "failed to allocate memory for mbuf.");
> 
> This error log here is scary as it means the datapath can be slowed
> down for each multisegment mbuf in the event of a mbuf (maybe
> temporary) shortage.
> Besides no other mbuf allocation in the vhost library datapath
> generates such log.
> 
> I would remove it, probably in a separate patch.
> WDYT?

Agree, we should not have such log in the datapath.
And now that we have the stat, it is even less useful.

Regards,
Maxime

> 
>>                                  goto error;
> 
> 

Re: [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path

[David Marchand]

On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> When vIOMMU is enabled and Virtio device is bound to kernel
> driver in guest, rte_vhost_dequeue_burst() will often return
> early because of IOTLB misses.

In theory, we can hit this issue with a dpdk pmd too, as long as the
vIOMMU is in use.
But the consequence would be a "really small" leak which does not have
the same impact as what was seen with the kernel driver which
maps/unmaps pages associated with virtio-net skb way more often :-).
So maybe rephrase this part emphasizing on the kernel case like:

"""
When vIOMMU is enabled, rte_vhost_dequeue_burst() can return early
because of IOTLB misses.
Such IOTLB misses are especially frequent when a Virtio device is
bound to a kernel driver in guest.
"""

>
> This patch fixes a mbuf leak occurring in this case.
>
> Fixes: 242695f6122a ("vhost: allocate and free packets in bulk in Tx split")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH v2 2/2] vhost: add new mbuf allocation failure statistic

[David Marchand]

On Thu, Feb 1, 2024 at 9:29 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 2/1/24 09:10, David Marchand wrote:
> > On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> This patch introduces a new, per virtqueue, mbuf allocation
> >> failure statistic. It can be useful to troubleshoot packets
> >> drops due to insufficient mempool size or memory leaks.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > Having a stat for such situation will be useful.
> >
> > I just have one comment, though it is not really related to this change itself.
> >
> > [snip]
> >
> >> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> >> index 9951842b9f..1359c5fb1f 100644
> >> --- a/lib/vhost/virtio_net.c
> >> +++ b/lib/vhost/virtio_net.c
> >> @@ -2996,6 +2996,7 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
> >>                  if (mbuf_avail == 0) {
> >>                          cur = rte_pktmbuf_alloc(mbuf_pool);
> >>                          if (unlikely(cur == NULL)) {
> >> +                               vq->stats.mbuf_alloc_failed++;
> >>                                  VHOST_DATA_LOG(dev->ifname, ERR,
> >>                                          "failed to allocate memory for mbuf.");
> >
> > This error log here is scary as it means the datapath can be slowed
> > down for each multisegment mbuf in the event of a mbuf (maybe
> > temporary) shortage.
> > Besides no other mbuf allocation in the vhost library datapath
> > generates such log.
> >
> > I would remove it, probably in a separate patch.
> > WDYT?
>
> Agree, we should not have such log in the datapath.
> And now that we have the stat, it is even less useful.

Ok, nevertheless, you can add my:
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path

[Maxime Coquelin]

On 2/6/24 11:29, David Marchand wrote:
> On Wed, Jan 31, 2024 at 8:53 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> When vIOMMU is enabled and Virtio device is bound to kernel
>> driver in guest, rte_vhost_dequeue_burst() will often return
>> early because of IOTLB misses.
> 
> In theory, we can hit this issue with a dpdk pmd too, as long as the
> vIOMMU is in use.
> But the consequence would be a "really small" leak which does not have
> the same impact as what was seen with the kernel driver which
> maps/unmaps pages associated with virtio-net skb way more often :-).
> So maybe rephrase this part emphasizing on the kernel case like:
> 
> """
> When vIOMMU is enabled, rte_vhost_dequeue_burst() can return early
> because of IOTLB misses.
> Such IOTLB misses are especially frequent when a Virtio device is
> bound to a kernel driver in guest.
> """

Thanks, I agree with your suggestion, Virtio PMD is indeed also
impacted.

>>
>> This patch fixes a mbuf leak occurring in this case.
>>
>> Fixes: 242695f6122a ("vhost: allocate and free packets in bulk in Tx split")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> 

Re: [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path

[Maxime Coquelin]

On 1/31/24 20:53, Maxime Coquelin wrote:
> When vIOMMU is enabled and Virtio device is bound to kernel
> driver in guest, rte_vhost_dequeue_burst() will often return
> early because of IOTLB misses.
> 
> This patch fixes a mbuf leak occurring in this case.
> 
> Fixes: 242695f6122a ("vhost: allocate and free packets in bulk in Tx split")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> 
> Changes in v2:
> ==============
> - Fix descriptors leak (David)
> - Rebased on top of next-virtio
> 
> ---
>   lib/vhost/virtio_net.c | 24 ++++++------------------
>   1 file changed, 6 insertions(+), 18 deletions(-)
> 

Applied to next-virtio tree.

Thanks,
Maxime

Re: [PATCH v2 2/2] vhost: add new mbuf allocation failure statistic

[Maxime Coquelin]

On 1/31/24 20:53, Maxime Coquelin wrote:
> This patch introduces a new, per virtqueue, mbuf allocation
> failure statistic. It can be useful to troubleshoot packets
> drops due to insufficient mempool size or memory leaks.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>   lib/vhost/vhost.c      |  1 +
>   lib/vhost/vhost.h      |  1 +
>   lib/vhost/virtio_net.c | 17 +++++++++++++----
>   3 files changed, 15 insertions(+), 4 deletions(-)
> 

Applied to next-virtio tree.

Thanks,
Maxime