diff mbox series

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

Message ID 20240131195309.2808015-1-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series [v2,1/2] vhost: fix memory leak in Virtio Tx split path |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Maxime Coquelin Jan. 31, 2024, 7:53 p.m. UTC 
  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(-)

Comments

David Marchand Feb. 6, 2024, 10:29 a.m. UTC | #1 
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>
Maxime Coquelin Feb. 6, 2024, 1:57 p.m. UTC | #2 
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>
> 
>
Maxime Coquelin Feb. 6, 2024, 2:59 p.m. UTC | #3 
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
diff mbox series

Patch

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