* [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path
@ 2024-01-31 19:53 Maxime Coquelin
2024-02-06 10:29 ` David Marchand
2024-02-06 14:59 ` Maxime Coquelin
0 siblings, 2 replies; 4+ messages in thread
From: Maxime Coquelin @ 2024-01-31 19:53 UTC (permalink / raw)
To: dev, chenbox, david.marchand, bnemeth, echaudro; +Cc: Maxime Coquelin, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path
2024-01-31 19:53 [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path Maxime Coquelin
@ 2024-02-06 10:29 ` David Marchand
2024-02-06 13:57 ` Maxime Coquelin
2024-02-06 14:59 ` Maxime Coquelin
1 sibling, 1 reply; 4+ messages in thread
From: David Marchand @ 2024-02-06 10:29 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, chenbox, bnemeth, echaudro, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path
2024-02-06 10:29 ` David Marchand
@ 2024-02-06 13:57 ` Maxime Coquelin
0 siblings, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2024-02-06 13:57 UTC (permalink / raw)
To: David Marchand; +Cc: dev, chenbox, bnemeth, echaudro, stable
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>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path
2024-01-31 19:53 [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path Maxime Coquelin
2024-02-06 10:29 ` David Marchand
@ 2024-02-06 14:59 ` Maxime Coquelin
1 sibling, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2024-02-06 14:59 UTC (permalink / raw)
To: dev, chenbox, david.marchand, bnemeth, echaudro; +Cc: stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-06 14:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-31 19:53 [PATCH v2 1/2] vhost: fix memory leak in Virtio Tx split path Maxime Coquelin
2024-02-06 10:29 ` David Marchand
2024-02-06 13:57 ` Maxime Coquelin
2024-02-06 14:59 ` Maxime Coquelin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).