diff mbox

[dpdk-dev,v3,1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.

Message ID 1483930780-7064-1-git-send-email-nic@opencloud.tech (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

nickcooper-zhangtonghao Jan. 9, 2017, 2:59 a.m. UTC 
  This patch will check the "nb_desc" parameter for rx queue.
Rx vmxnet rings length should be between 128-4096.
The patch will release the rxq and re-allocation it soon
for different "nb_desc".

Signed-off-by: Nick Zhang <nic@opencloud.tech>
---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Yong Wang Jan. 17, 2017, 8:15 p.m. UTC | #1 
> -----Original Message-----

> From: Nick Zhang [mailto:nic@opencloud.tech]

> Sent: Sunday, January 8, 2017 7:00 PM

> To: Yong Wang <yongwang@vmware.com>

> Cc: ferruh.yigit@intel.com; dev@dpdk.org; Nick Zhang <nic@opencloud.tech>

> Subject: [PATCH v3 1/4] vmxnet3: Avoid memory leak in

> vmxnet3_dev_rx_queue_setup.

> 

> This patch will check the "nb_desc" parameter for rx queue.

> Rx vmxnet rings length should be between 128-4096.

> The patch will release the rxq and re-allocation it soon

> for different "nb_desc".

> 

> Signed-off-by: Nick Zhang <nic@opencloud.tech>

> ---

>  drivers/net/vmxnet3/vmxnet3_rxtx.c | 30 ++++++++++++++++++------------

>  1 file changed, 18 insertions(+), 12 deletions(-)

> 

> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c

> b/drivers/net/vmxnet3/vmxnet3_rxtx.c

> index b109168..e77374f 100644

> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c

> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c

> @@ -926,6 +926,21 @@

> 

>  	PMD_INIT_FUNC_TRACE();

> 

> +	/* Rx vmxnet rings length should be between 128-4096 */

> +	if (nb_desc < VMXNET3_DEF_RX_RING_SIZE) {

> +		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Min: 128");

> +		return -EINVAL;

> +	} else if (nb_desc > VMXNET3_RX_RING_MAX_SIZE) {

> +		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Max: 4096");

> +		return -EINVAL;

> +	}

> +

> +	/* Free memory prior to re-allocation if needed. */

> +	if (dev->data->rx_queues[queue_idx] != NULL) {

> +		vmxnet3_dev_rx_queue_release(dev->data->rx_queues[queue_idx]);


Currently vmxnet3_dev_rx_queue_release() does not free device ring memory.  As a result, the same device ring memory allocated based on the previous descriptor size will be used and that should also explain why you are observing seg fault with an increased ring size.  If you handle the device ring memory free in vmxnet3_dev_rx_queue_release(), I think the pre-allocation of ring with max size will not be needed any more.

> +		dev->data->rx_queues[queue_idx] = NULL;

> +	}

> +

>  	rxq = rte_zmalloc("ethdev_rx_queue", sizeof(struct vmxnet3_rx_queue),

>  			  RTE_CACHE_LINE_SIZE);

>  	if (rxq == NULL) {

> @@ -946,18 +961,9 @@

>  	ring1 = &rxq->cmd_ring[1];

>  	comp_ring = &rxq->comp_ring;

> 

> -	/* Rx vmxnet rings length should be between 256-4096 */

> -	if (nb_desc < VMXNET3_DEF_RX_RING_SIZE) {

> -		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Min: 256");

> -		return -EINVAL;

> -	} else if (nb_desc > VMXNET3_RX_RING_MAX_SIZE) {

> -		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Max: 4096");

> -		return -EINVAL;

> -	} else {

> -		ring0->size = nb_desc;

> -		ring0->size &= ~VMXNET3_RING_SIZE_MASK;

> -		ring1->size = ring0->size;

> -	}

> +	ring0->size = nb_desc;

> +	ring0->size &= ~VMXNET3_RING_SIZE_MASK;

> +	ring1->size = ring0->size;

> 

>  	comp_ring->size = ring0->size + ring1->size;

> 

> --

> 1.8.3.1

> 

>
nickcooper-zhangtonghao Jan. 18, 2017, 1:37 a.m. UTC | #2 
> On Jan 18, 2017, at 4:15 AM, Yong Wang <yongwang@vmware.com> wrote:
> 
>> -----Original Message-----
>> From: Nick Zhang [mailto:nic@opencloud.tech <mailto:nic@opencloud.tech>]
>> Sent: Sunday, January 8, 2017 7:00 PM
>> To: Yong Wang <yongwang@vmware.com <mailto:yongwang@vmware.com>>
>> Cc: ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>; dev@dpdk.org <mailto:dev@dpdk.org>; Nick Zhang <nic@opencloud.tech <mailto:nic@opencloud.tech>>
>> Subject: [PATCH v3 1/4] vmxnet3: Avoid memory leak in
>> vmxnet3_dev_rx_queue_setup.
>> 
>> This patch will check the "nb_desc" parameter for rx queue.
>> Rx vmxnet rings length should be between 128-4096.
>> The patch will release the rxq and re-allocation it soon
>> for different "nb_desc".
>> 
>> Signed-off-by: Nick Zhang <nic@opencloud.tech>
>> ---
>> drivers/net/vmxnet3/vmxnet3_rxtx.c | 30 ++++++++++++++++++------------
>> 1 file changed, 18 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> index b109168..e77374f 100644
>> --- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> +++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
>> @@ -926,6 +926,21 @@
>> 
>> 	PMD_INIT_FUNC_TRACE();
>> 
>> +	/* Rx vmxnet rings length should be between 128-4096 */
>> +	if (nb_desc < VMXNET3_DEF_RX_RING_SIZE) {
>> +		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Min: 128");
>> +		return -EINVAL;
>> +	} else if (nb_desc > VMXNET3_RX_RING_MAX_SIZE) {
>> +		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Max: 4096");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Free memory prior to re-allocation if needed. */
>> +	if (dev->data->rx_queues[queue_idx] != NULL) {
>> +		vmxnet3_dev_rx_queue_release(dev->data->rx_queues[queue_idx]);
> 
> Currently vmxnet3_dev_rx_queue_release() does not free device ring memory.  As a result, the same device ring memory allocated based on the previous descriptor size will be used and that should also explain why you are observing seg fault with an increased ring size. If you handle the device ring memory free in vmxnet3_dev_rx_queue_release(), I think the pre-allocation of ring with max size will not be needed any more.

Yes, we should not free the pre-allocation of ring, but alloc only once ring with max size. I will submit v4.
Thank you.
diff mbox

Patch

diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index b109168..e77374f 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -926,6 +926,21 @@ 
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* Rx vmxnet rings length should be between 128-4096 */
+	if (nb_desc < VMXNET3_DEF_RX_RING_SIZE) {
+		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Min: 128");
+		return -EINVAL;
+	} else if (nb_desc > VMXNET3_RX_RING_MAX_SIZE) {
+		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Max: 4096");
+		return -EINVAL;
+	}
+
+	/* Free memory prior to re-allocation if needed. */
+	if (dev->data->rx_queues[queue_idx] != NULL) {
+		vmxnet3_dev_rx_queue_release(dev->data->rx_queues[queue_idx]);
+		dev->data->rx_queues[queue_idx] = NULL;
+	}
+
 	rxq = rte_zmalloc("ethdev_rx_queue", sizeof(struct vmxnet3_rx_queue),
 			  RTE_CACHE_LINE_SIZE);
 	if (rxq == NULL) {
@@ -946,18 +961,9 @@ 
 	ring1 = &rxq->cmd_ring[1];
 	comp_ring = &rxq->comp_ring;
 
-	/* Rx vmxnet rings length should be between 256-4096 */
-	if (nb_desc < VMXNET3_DEF_RX_RING_SIZE) {
-		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Min: 256");
-		return -EINVAL;
-	} else if (nb_desc > VMXNET3_RX_RING_MAX_SIZE) {
-		PMD_INIT_LOG(ERR, "VMXNET3 Rx Ring Size Max: 4096");
-		return -EINVAL;
-	} else {
-		ring0->size = nb_desc;
-		ring0->size &= ~VMXNET3_RING_SIZE_MASK;
-		ring1->size = ring0->size;
-	}
+	ring0->size = nb_desc;
+	ring0->size &= ~VMXNET3_RING_SIZE_MASK;
+	ring1->size = ring0->size;
 
 	comp_ring->size = ring0->size + ring1->size;