* [PATCH v3 2/4] vmxnet3: Avoid segfault caused by vmxnet3_dev_rx_queue_setup.
2017-01-09 2:59 [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Nick Zhang
@ 2017-01-09 2:59 ` Nick Zhang
2017-01-09 2:59 ` [PATCH v3 3/4] vmxnet3: Avoid memory leak in vmxnet3_dev_tx_queue_setup Nick Zhang
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Nick Zhang @ 2017-01-09 2:59 UTC (permalink / raw)
To: yongwang; +Cc: ferruh.yigit, dev, Nick Zhang
When we config RX queue with 2048 RX queue size, and stop the
device, changed queue size to 4096 and then start the device,
there will be segment fault. We should allocate RX ring for
max possible number of hardware descriptors.
Signed-off-by: Nick Zhang <nic@opencloud.tech>
---
drivers/net/vmxnet3/vmxnet3_rxtx.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index e77374f..d5d7c33 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -977,8 +977,11 @@
comp_ring->next2proc = 0;
comp_ring->gen = VMXNET3_INIT_GEN;
- size = sizeof(struct Vmxnet3_RxDesc) * (ring0->size + ring1->size);
- size += sizeof(struct Vmxnet3_RxCompDesc) * comp_ring->size;
+ /* Allocate RX ring for max possible number of hardware descriptors. */
+ size = sizeof(struct Vmxnet3_RxDesc) *
+ (VMXNET3_RX_RING_MAX_SIZE * VMXNET3_RX_CMDRING_SIZE);
+ size += sizeof(struct Vmxnet3_RxCompDesc) *
+ (VMXNET3_RX_RING_MAX_SIZE * VMXNET3_RX_CMDRING_SIZE);
mz = ring_dma_zone_reserve(dev, "rxdesc", queue_idx, size, socket_id);
if (mz == NULL) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 3/4] vmxnet3: Avoid memory leak in vmxnet3_dev_tx_queue_setup.
2017-01-09 2:59 [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Nick Zhang
2017-01-09 2:59 ` [PATCH v3 2/4] vmxnet3: Avoid segfault caused by vmxnet3_dev_rx_queue_setup Nick Zhang
@ 2017-01-09 2:59 ` Nick Zhang
2017-01-09 2:59 ` [PATCH v3 4/4] vmxnet3: Avoid segfault caused by vmxnet3_dev_tx_queue_setup Nick Zhang
2017-01-17 20:15 ` [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Yong Wang
3 siblings, 0 replies; 8+ messages in thread
From: Nick Zhang @ 2017-01-09 2:59 UTC (permalink / raw)
To: yongwang; +Cc: ferruh.yigit, dev, Nick Zhang
This patch will check the "nb_desc" parameter for tx queue.
Tx vmxnet rings length should be between 512-4096. The patch
will release the txq and re-allocation it soon for different "nb_desc".
Signed-off-by: Nick Zhang <nic@opencloud.tech>
---
drivers/net/vmxnet3/vmxnet3_rxtx.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index d5d7c33..f00b3b9 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -828,6 +828,23 @@
return -EINVAL;
}
+ /* Tx vmxnet ring length should be between 512-4096 */
+ if (nb_desc < VMXNET3_DEF_TX_RING_SIZE) {
+ PMD_INIT_LOG(ERR, "VMXNET3 Tx Ring Size Min: %u",
+ VMXNET3_DEF_TX_RING_SIZE);
+ return -EINVAL;
+ } else if (nb_desc > VMXNET3_TX_RING_MAX_SIZE) {
+ PMD_INIT_LOG(ERR, "VMXNET3 Tx Ring Size Max: %u",
+ VMXNET3_TX_RING_MAX_SIZE);
+ return -EINVAL;
+ }
+
+ /* Free memory prior to re-allocation if needed... */
+ if (dev->data->tx_queues[queue_idx] != NULL) {
+ vmxnet3_dev_tx_queue_release(dev->data->tx_queues[queue_idx]);
+ dev->data->tx_queues[queue_idx] = NULL;
+ }
+
txq = rte_zmalloc("ethdev_tx_queue", sizeof(struct vmxnet3_tx_queue),
RTE_CACHE_LINE_SIZE);
if (txq == NULL) {
@@ -846,19 +863,8 @@
comp_ring = &txq->comp_ring;
data_ring = &txq->data_ring;
- /* Tx vmxnet ring length should be between 512-4096 */
- if (nb_desc < VMXNET3_DEF_TX_RING_SIZE) {
- PMD_INIT_LOG(ERR, "VMXNET3 Tx Ring Size Min: %u",
- VMXNET3_DEF_TX_RING_SIZE);
- return -EINVAL;
- } else if (nb_desc > VMXNET3_TX_RING_MAX_SIZE) {
- PMD_INIT_LOG(ERR, "VMXNET3 Tx Ring Size Max: %u",
- VMXNET3_TX_RING_MAX_SIZE);
- return -EINVAL;
- } else {
- ring->size = nb_desc;
- ring->size &= ~VMXNET3_RING_SIZE_MASK;
- }
+ ring->size = nb_desc;
+ ring->size &= ~VMXNET3_RING_SIZE_MASK;
comp_ring->size = data_ring->size = ring->size;
/* Tx vmxnet rings structure initialization*/
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v3 4/4] vmxnet3: Avoid segfault caused by vmxnet3_dev_tx_queue_setup.
2017-01-09 2:59 [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Nick Zhang
2017-01-09 2:59 ` [PATCH v3 2/4] vmxnet3: Avoid segfault caused by vmxnet3_dev_rx_queue_setup Nick Zhang
2017-01-09 2:59 ` [PATCH v3 3/4] vmxnet3: Avoid memory leak in vmxnet3_dev_tx_queue_setup Nick Zhang
@ 2017-01-09 2:59 ` Nick Zhang
2017-01-17 20:15 ` [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Yong Wang
3 siblings, 0 replies; 8+ messages in thread
From: Nick Zhang @ 2017-01-09 2:59 UTC (permalink / raw)
To: yongwang; +Cc: ferruh.yigit, dev, Nick Zhang
When we config TX queue with 2048 TX queue size, stop the device,
changed queue size to 4096 and then start the device, there will
be segment fault. We should allocate TX ring for max possible
number of hardware descriptors.
Signed-off-by: Nick Zhang <nic@opencloud.tech>
---
drivers/net/vmxnet3/vmxnet3_rxtx.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index f00b3b9..5f35a2e 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -874,9 +874,10 @@
comp_ring->next2proc = 0;
comp_ring->gen = VMXNET3_INIT_GEN;
- size = sizeof(struct Vmxnet3_TxDesc) * ring->size;
- size += sizeof(struct Vmxnet3_TxCompDesc) * comp_ring->size;
- size += sizeof(struct Vmxnet3_TxDataDesc) * data_ring->size;
+ /* Allocate Tx ring for max possible number of hardware descriptors. */
+ size = sizeof(struct Vmxnet3_TxDesc) * VMXNET3_TX_RING_MAX_SIZE;
+ size += sizeof(struct Vmxnet3_TxCompDesc) * VMXNET3_TX_RING_MAX_SIZE;
+ size += sizeof(struct Vmxnet3_TxDataDesc) * VMXNET3_TX_RING_MAX_SIZE;
mz = ring_dma_zone_reserve(dev, "txdesc", queue_idx, size, socket_id);
if (mz == NULL) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.
2017-01-09 2:59 [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Nick Zhang
` (2 preceding siblings ...)
2017-01-09 2:59 ` [PATCH v3 4/4] vmxnet3: Avoid segfault caused by vmxnet3_dev_tx_queue_setup Nick Zhang
@ 2017-01-17 20:15 ` Yong Wang
2017-01-18 1:37 ` nickcooper-zhangtonghao
3 siblings, 1 reply; 8+ messages in thread
From: Yong Wang @ 2017-01-17 20:15 UTC (permalink / raw)
To: Nick Zhang; +Cc: ferruh.yigit, dev
> -----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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.
2017-01-17 20:15 ` [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup Yong Wang
@ 2017-01-18 1:37 ` nickcooper-zhangtonghao
2017-01-18 2:05 ` Yong Wang
0 siblings, 1 reply; 8+ messages in thread
From: nickcooper-zhangtonghao @ 2017-01-18 1:37 UTC (permalink / raw)
To: Yong Wang; +Cc: ferruh.yigit, dev
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.
2017-01-18 1:37 ` nickcooper-zhangtonghao
@ 2017-01-18 2:05 ` Yong Wang
2017-01-18 2:41 ` nickcooper-zhangtonghao
0 siblings, 1 reply; 8+ messages in thread
From: Yong Wang @ 2017-01-18 2:05 UTC (permalink / raw)
To: nickcooper-zhangtonghao; +Cc: ferruh.yigit, dev
Any downside with free/reallocation now that memzone can be freed? Allocation with max ring size should work but is kind of wasteful in terms of memory usage and I assume this type of ring size change should not be a frequent operation.
From: nickcooper-zhangtonghao [mailto:nic@opencloud.tech]
Sent: Tuesday, January 17, 2017 5:37 PM
To: Yong Wang <yongwang@vmware.com>
Cc: ferruh.yigit@intel.com; dev@dpdk.org
Subject: Re: [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.
On Jan 18, 2017, at 4:15 AM, Yong Wang <yongwang@vmware.com<mailto:yongwang@vmware.com>> wrote:
-----Original Message-----
From: Nick Zhang [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<mailto: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.
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/4] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.
2017-01-18 2:05 ` Yong Wang
@ 2017-01-18 2:41 ` nickcooper-zhangtonghao
0 siblings, 0 replies; 8+ messages in thread
From: nickcooper-zhangtonghao @ 2017-01-18 2:41 UTC (permalink / raw)
To: Yong Wang; +Cc: ferruh.yigit, dev
> On Jan 18, 2017, at 10:05 AM, Yong Wang <yongwang@vmware.com> wrote:
>
> Any downside with free/reallocation now that memzone can be freed? Allocation with max ring size should work but is kind of wasteful in terms of memory usage and I assume this type of ring size change should not be a frequent operation.
Not free/realloc them anymore. I guess it is necessary to use the
allocation with max ring size. The seg fault really bother us.
and the app (e.g. ovs) may change it frequently when tuning performance.
^ permalink raw reply [flat|nested] 8+ messages in thread