[2/2] net/virtio: fix Tx completed mbufs leak on device stop
Checks
Commit Message
From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Free Tx completed mbufs on device stop. Not completed Tx mbufs cannot be
freed since they are still in use.
Fixes: c1f86306a02 ("virtio: add new driver")
Cc: stable@dpdk.org
Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
drivers/net/virtio/virtio_ethdev.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
Comments
On 8/18/21 4:13 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>
> Free Tx completed mbufs on device stop. Not completed Tx mbufs cannot be
> freed since they are still in use.
>
> Fixes: c1f86306a02 ("virtio: add new driver")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> drivers/net/virtio/virtio_ethdev.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index e58085a2c9..ed3fefee7c 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -2393,6 +2393,34 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
> PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
> }
>
> +static void
> +virtio_tx_completed_cleanup(struct rte_eth_dev *dev)
> +{
> + struct virtio_hw *hw = dev->data->dev_private;
> + struct virtqueue *vq;
> + int qidx;
> + void (*xmit_cleanup)(struct virtqueue *vq, uint16_t nb_used);
> +
> + if (virtio_with_packed_queue(hw)) {
> + if (hw->use_vec_tx)
> + xmit_cleanup = &virtio_xmit_cleanup_inorder_packed;
> + else if (virtio_with_feature(hw, VIRTIO_F_IN_ORDER))
> + xmit_cleanup = &virtio_xmit_cleanup_inorder_packed;
> + else
> + xmit_cleanup = &virtio_xmit_cleanup_normal_packed;
> + } else {
> + if (hw->use_inorder_tx)
> + xmit_cleanup = &virtio_xmit_cleanup_inorder;
> + else
> + xmit_cleanup = &virtio_xmit_cleanup;
> + }
> +
> + for (qidx = 0; qidx < hw->max_queue_pairs; qidx++) {
> + vq = hw->vqs[2 * qidx + VTNET_SQ_TQ_QUEUE_IDX];
Maybe add a check to ensure that vq is non-NULL since it is dereferenced
later without checking.
> + xmit_cleanup(vq, virtqueue_nused(vq));
> + }
> +}
> +
> /*
> * Stop device: disable interrupt and mark link down
> */
> @@ -2411,6 +2439,8 @@ virtio_dev_stop(struct rte_eth_dev *dev)
> goto out_unlock;
> hw->started = 0;
>
> + virtio_tx_completed_cleanup(dev);
> +
> if (intr_conf->lsc || intr_conf->rxq) {
> virtio_intr_disable(dev);
>
>
On 9/13/21 6:41 PM, Maxime Coquelin wrote:
>
>
> On 8/18/21 4:13 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Free Tx completed mbufs on device stop. Not completed Tx mbufs cannot be
>> freed since they are still in use.
>>
>> Fixes: c1f86306a02 ("virtio: add new driver")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>> drivers/net/virtio/virtio_ethdev.c | 30 ++++++++++++++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index e58085a2c9..ed3fefee7c 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -2393,6 +2393,34 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
>> PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
>> }
>>
>> +static void
>> +virtio_tx_completed_cleanup(struct rte_eth_dev *dev)
>> +{
>> + struct virtio_hw *hw = dev->data->dev_private;
>> + struct virtqueue *vq;
>> + int qidx;
>> + void (*xmit_cleanup)(struct virtqueue *vq, uint16_t nb_used);
>> +
>> + if (virtio_with_packed_queue(hw)) {
>> + if (hw->use_vec_tx)
>> + xmit_cleanup = &virtio_xmit_cleanup_inorder_packed;
>> + else if (virtio_with_feature(hw, VIRTIO_F_IN_ORDER))
>> + xmit_cleanup = &virtio_xmit_cleanup_inorder_packed;
>> + else
>> + xmit_cleanup = &virtio_xmit_cleanup_normal_packed;
>> + } else {
>> + if (hw->use_inorder_tx)
>> + xmit_cleanup = &virtio_xmit_cleanup_inorder;
>> + else
>> + xmit_cleanup = &virtio_xmit_cleanup;
>> + }
>> +
>> + for (qidx = 0; qidx < hw->max_queue_pairs; qidx++) {
>> + vq = hw->vqs[2 * qidx + VTNET_SQ_TQ_QUEUE_IDX];
>
> Maybe add a check to ensure that vq is non-NULL since it is dereferenced
> later without checking.
Good idea, I'll send v2.
>
>> + xmit_cleanup(vq, virtqueue_nused(vq));
>> + }
>> +}
>> +
>> /*
>> * Stop device: disable interrupt and mark link down
>> */
>> @@ -2411,6 +2439,8 @@ virtio_dev_stop(struct rte_eth_dev *dev)
>> goto out_unlock;
>> hw->started = 0;
>>
>> + virtio_tx_completed_cleanup(dev);
>> +
>> if (intr_conf->lsc || intr_conf->rxq) {
>> virtio_intr_disable(dev);
>>
>>
@@ -2393,6 +2393,34 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
PMD_INIT_LOG(DEBUG, "%d mbufs freed", mbuf_num);
}
+static void
+virtio_tx_completed_cleanup(struct rte_eth_dev *dev)
+{
+ struct virtio_hw *hw = dev->data->dev_private;
+ struct virtqueue *vq;
+ int qidx;
+ void (*xmit_cleanup)(struct virtqueue *vq, uint16_t nb_used);
+
+ if (virtio_with_packed_queue(hw)) {
+ if (hw->use_vec_tx)
+ xmit_cleanup = &virtio_xmit_cleanup_inorder_packed;
+ else if (virtio_with_feature(hw, VIRTIO_F_IN_ORDER))
+ xmit_cleanup = &virtio_xmit_cleanup_inorder_packed;
+ else
+ xmit_cleanup = &virtio_xmit_cleanup_normal_packed;
+ } else {
+ if (hw->use_inorder_tx)
+ xmit_cleanup = &virtio_xmit_cleanup_inorder;
+ else
+ xmit_cleanup = &virtio_xmit_cleanup;
+ }
+
+ for (qidx = 0; qidx < hw->max_queue_pairs; qidx++) {
+ vq = hw->vqs[2 * qidx + VTNET_SQ_TQ_QUEUE_IDX];
+ xmit_cleanup(vq, virtqueue_nused(vq));
+ }
+}
+
/*
* Stop device: disable interrupt and mark link down
*/
@@ -2411,6 +2439,8 @@ virtio_dev_stop(struct rte_eth_dev *dev)
goto out_unlock;
hw->started = 0;
+ virtio_tx_completed_cleanup(dev);
+
if (intr_conf->lsc || intr_conf->rxq) {
virtio_intr_disable(dev);