[PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[nickcooper-zhangtonghao]

The NUMA node information for PCI devices provided through
sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
on Red Hat Enterprise Linux 6, and VMs on some hypervisors.

Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 4350134..5dfdbe9 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -317,7 +317,13 @@
 			free(dev);
 			return -1;
 		}
-		dev->device.numa_node = tmp;
+		/* The NUMA node information for PCI devices provided through
+		 * sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
+		 * on Red Hat Enterprise Linux 6, and VMs on some hypervisors.
+		 * In the upstream linux kernel, the numa_node is an integer,
+		 * which data type is int, not unsigned long.
+		 */
+		dev->device.numa_node = (int)tmp > 0 ? (int)tmp : 0;
 	}
 
 	/* parse resources */
-- 
1.8.3.1

[PATCH v2 2/5] vmxnet3: Avoid memory leak in vmxnet3_dev_rx_queue_setup.

[nickcooper-zhangtonghao]

This patch will check the "nb_desc" parameter for rx queue,
release the rxq and re-allocation it soon.

Signed-off-by: nickcooper-zhangtonghao <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]);
+		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

[PATCH v2 3/5] vmxnet3: Avoid segfault caused by vmxnet3_dev_rx_queue_setup.

[nickcooper-zhangtonghao]

We should allocate RX ring for max possible number of hardware
descriptors. If we config RX queue with 2048 RX queue size,
and 4096 soon, there will be segment fault when calling other
ethernet API (e.g. rte_eth_dev_start).

Signed-off-by: nickcooper-zhangtonghao <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

[PATCH v2 4/5] vmxnet3: Avoid memory leak in vmxnet3_dev_tx_queue_setup.

[nickcooper-zhangtonghao]

This patch will check the "nb_desc" parameter for tx queue,
release the txq and re-allocation it soon.

Signed-off-by: nickcooper-zhangtonghao <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

[PATCH v2 5/5] vmxnet3: Avoid segfault caused by vmxnet3_dev_tx_queue_setup.

[nickcooper-zhangtonghao]

We should allocate Tx ring for max possible mumber of hardware descriptors.
If we config Tx queue with 2048 Tx queue size, and 4096 soon,
there will be segment fault.

Signed-off-by: nickcooper-zhangtonghao <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

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[Ferruh Yigit]

On 1/5/2017 12:01 PM, nickcooper-zhangtonghao wrote:
> The NUMA node information for PCI devices provided through
> sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
> on Red Hat Enterprise Linux 6, and VMs on some hypervisors.
> 
> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>

Hi nickcooper-zhangtonghao,

The patches in the patchset are individual patches, right? Is there any
dependency between them?

And CC'ed vmxnet3 driver maintainer: Yong Wang <yongwang@vmware.com>

Thanks,
ferruh

> ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 4350134..5dfdbe9 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -317,7 +317,13 @@
>  			free(dev);
>  			return -1;
>  		}
> -		dev->device.numa_node = tmp;
> +		/* The NUMA node information for PCI devices provided through
> +		 * sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
> +		 * on Red Hat Enterprise Linux 6, and VMs on some hypervisors.
> +		 * In the upstream linux kernel, the numa_node is an integer,
> +		 * which data type is int, not unsigned long.
> +		 */
> +		dev->device.numa_node = (int)tmp > 0 ? (int)tmp : 0;
>  	}
>  
>  	/* parse resources */
> 

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[Stephen Hemminger]

On Thu,  5 Jan 2017 04:01:45 -0800
nickcooper-zhangtonghao <nic@opencloud.tech> wrote:

> The NUMA node information for PCI devices provided through
> sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
> on Red Hat Enterprise Linux 6, and VMs on some hypervisors.
> 
> Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>

That name seems a bit unusual. Is that your legal name or a partnership?

The Signed-off-by line has legal significance and it is important that
you use your legal name. Please use the name you would use when
signing a legal document (like buying a house or renting).

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[Stephen Hemminger]

On Thu,  5 Jan 2017 04:01:45 -0800
nickcooper-zhangtonghao <nic@opencloud.tech> wrote:

> +		/* The NUMA node information for PCI devices provided through
> +		 * sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
> +		 * on Red Hat Enterprise Linux 6, and VMs on some hypervisors.
> +		 * In the upstream linux kernel, the numa_node is an integer,
> +		 * which data type is int, not unsigned long.
> +		 */
> +		dev->device.numa_node = (int)tmp > 0 ? (int)tmp : 0;
>  	}

It is good to see more checking for valid values.  I suspect that other systems
may have the same problem.  My preference would to have the code comment generic
and to have the precise details of about where this was observed in the commit
log. 

The following would do same thing but be simpler:

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 43501342..9f09cd98 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -306,19 +306,12 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
 			dev->max_vfs = (uint16_t)tmp;
 	}
 
-	/* get numa node */
+	/* get numa node, default to 0 if not present */
 	snprintf(filename, sizeof(filename), "%s/numa_node",
 		 dirname);
-	if (access(filename, R_OK) != 0) {
-		/* if no NUMA support, set default to 0 */
-		dev->device.numa_node = 0;
-	} else {
-		if (eal_parse_sysfs_value(filename, &tmp) < 0) {
-			free(dev);
-			return -1;
-		}
+	if (eal_parse_sysfs_value(filename, &tmp) == 0 &&
+	    tmp < RTE_MAX_NUMA_NODES)
 		dev->device.numa_node = tmp;
-	}
 
 	/* parse resources */
 	snprintf(filename, sizeof(filename), "%s/resource", dirname);

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[Yong Wang]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Thursday, January 5, 2017 6:24 AM
> To: nickcooper-zhangtonghao <nic@opencloud.tech>; dev@dpdk.org; Yong
> Wang <yongwang@vmware.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/5] eal: Set numa node value for system
> which not support NUMA.
> 
> On 1/5/2017 12:01 PM, nickcooper-zhangtonghao wrote:
> > The NUMA node information for PCI devices provided through
> > sysfs is invalid for AMD Opteron(TM) Processor 62xx and 63xx
> > on Red Hat Enterprise Linux 6, and VMs on some hypervisors.
> >
> > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech>
> 
> Hi nickcooper-zhangtonghao,
> 
> The patches in the patchset are individual patches, right? Is there any
> dependency between them?
> 
> And CC'ed vmxnet3 driver maintainer: Yong Wang <yongwang@vmware.com>

Can you add the exact steps to reproduce the vmxnet3 issues to help the review and the verification. My guess is that you have stopped the device, changed some ring parameters (to something larger than the previous settings) and restarted the device. Such info should be included into the commit description in addition to just saying what the patch does.

> Thanks,
> ferruh
> 
> > ---
> >  lib/librte_eal/linuxapp/eal/eal_pci.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > index 4350134..5dfdbe9 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > @@ -317,7 +317,13 @@
> >  			free(dev);
> >  			return -1;
> >  		}
> > -		dev->device.numa_node = tmp;
> > +		/* The NUMA node information for PCI devices provided
> through
> > +		 * sysfs is invalid for AMD Opteron(TM) Processor 62xx and
> 63xx
> > +		 * on Red Hat Enterprise Linux 6, and VMs on some
> hypervisors.
> > +		 * In the upstream linux kernel, the numa_node is an integer,
> > +		 * which data type is int, not unsigned long.
> > +		 */
> > +		dev->device.numa_node = (int)tmp > 0 ? (int)tmp : 0;
> >  	}
> >
> >  	/* parse resources */
> >

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[nickcooper-zhangtonghao]

> On Jan 6, 2017, at 8:01 AM, Yong Wang <yongwang@vmware.com> wrote:
> 
> Can you add the exact steps to reproduce the vmxnet3 issues to help the review and the verification. My guess is that you have stopped the device, changed some ring parameters (to something larger than the previous settings) and restarted the device.


Thanks for your reply. Your guess is right. I run the openvswitch+dpdk with vmxnet3. 
First，I set the nb_desc of rx queue to 2048, and then stop the device， change nb_desc to 4096.
When I start the device again, I get the openvswitch crash caused by dpdk. Writing a sample program based on dpdk, I also get a crash.
The e1000 dpdk driver allocates RX ring for max possible mumber of hardware descriptors. I guess vmxnet3 should be in the same case.
I will submit v3.

Thanks. 

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[nickcooper-zhangtonghao]

Thanks for your reply. The patch you submitted is better. Thanks for your improvement.

My legal name is “Nick Zhang”. So,

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


Thanks.
Nick

> On Jan 6, 2017, at 12:26 AM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> It is good to see more checking for valid values.  I suspect that other systems
> may have the same problem.  My preference would to have the code comment generic
> and to have the precise details of about where this was observed in the commit
> log. 
> 
> The following would do same thing but be simpler:
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 43501342..9f09cd98 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -306,19 +306,12 @@ pci_scan_one(const char *dirname, uint16_t domain, uint8_t bus,
> 			dev->max_vfs = (uint16_t)tmp;
> 	}
> 
> -	/* get numa node */
> +	/* get numa node, default to 0 if not present */
> 	snprintf(filename, sizeof(filename), "%s/numa_node",
> 		 dirname);
> -	if (access(filename, R_OK) != 0) {
> -		/* if no NUMA support, set default to 0 */
> -		dev->device.numa_node = 0;
> -	} else {
> -		if (eal_parse_sysfs_value(filename, &tmp) < 0) {
> -			free(dev);
> -			return -1;
> -		}
> +	if (eal_parse_sysfs_value(filename, &tmp) == 0 &&
> +	    tmp < RTE_MAX_NUMA_NODES)
> 		dev->device.numa_node = tmp;
> -	}
> 
> 	/* parse resources */
> 	snprintf(filename, sizeof(filename), "%s/resource", dirname);

Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

[nickcooper-zhangtonghao]

I submitted the patches for first time. 
The first one is an individual patch for eal, others is for vmxnet3 interdependently.


Thanks.
Nick

> On Jan 5, 2017, at 10:23 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> Hi nickcooper-zhangtonghao,
> 
> The patches in the patchset are individual patches, right? Is there any
> dependency between them?
> 
> And CC'ed vmxnet3 driver maintainer: Yong Wang <yongwang@vmware.com <mailto:yongwang@vmware.com>>
> 
> Thanks,
> ferruh