diff mbox series

[2/2] net/virtio: fix Tx completed mbufs leak on device stop

Message ID 20210818141329.2842686-2-andrew.rybchenko@oktetlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [1/2] net/virtio: fix Tx cleanup functions to have same signature |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing fail Testing issues
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Andrew Rybchenko Aug. 18, 2021, 2:13 p.m. UTC 
  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

Maxime Coquelin Sept. 13, 2021, 3:41 p.m. UTC | #1 
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);
>  
>
Andrew Rybchenko Sept. 15, 2021, 9:16 a.m. UTC | #2 
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);
>>  
>>
diff mbox series

Patch

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];
+		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);