Skip to contentSkip to conversation

eal: Set numa node value for system which not support NUMA.

PATCH Superseded nickcooper-zhangtonghao 4 participants 12 messages Jan 5, 2017 – Jan 9, 2017

Subsystems (2)

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

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

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.

---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_pci.c +7 -1
index 4350134..5dfdbe9 100644
@@ -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 */
Signature · 1 line
1.8.3.1
#1 lore raw

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

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

---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
drivers/net/vmxnet3/vmxnet3_rxtx.c +18 -12
index b109168..e77374f 100644
@@ -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;
 
Signature · 1 line
1.8.3.1
#2 lore raw

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

In reply to 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).

---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/net/vmxnet3/vmxnet3_rxtx.c +5 -2
index e77374f..d5d7c33 100644
@@ -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) {
Signature · 1 line
1.8.3.1
#3 lore raw

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

In reply to nickcooper-zhangtonghao

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

---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)
drivers/net/vmxnet3/vmxnet3_rxtx.c +19 -13
index d5d7c33..f00b3b9 100644
@@ -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*/
Signature · 1 line
1.8.3.1
#4 lore raw

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

In reply to 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.

---
 drivers/net/vmxnet3/vmxnet3_rxtx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/net/vmxnet3/vmxnet3_rxtx.c +4 -3
index f00b3b9..5f35a2e 100644
@@ -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) {
Signature · 1 line
1.8.3.1
#5 lore raw
In reply to nickcooper-zhangtonghao
Quoted 5 lines · 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.

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 <[email protected]>

Thanks,
ferruh

Quoted 24 lines
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_pci.c +7 -1
index 4350134..5dfdbe9 100644
@@ -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 */
#6 lore raw
Quoted 13 lines

-----Original Message-----
From: Ferruh Yigit [mailto:[email protected]]
Sent: Thursday, January 5, 2017 6:24 AM
To: nickcooper-zhangtonghao <[email protected]>; [email protected]; Yong Wang <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA.

Quoted 4 lines · 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.

Quoted 8 lines
Quoted 1 line

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 <[email protected]>

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.

Quoted 31 lines

Thanks,
ferruh

Review

Quoted 5 lines
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/lib/librte_eal/linuxapp/eal/eal_pci.c

lib/librte_eal/linuxapp/eal/eal_pci.c line 320

Quoted 9 lines

index 4350134..5dfdbe9 100644

lib/librte_eal/linuxapp/eal/eal_pci.c +1 -1
@@ -317,7 +317,13 @@
 			free(dev);
 			return -1;
 		}
-		dev->device.numa_node = tmp;
+		/* The NUMA node information for PCI devices provided

through

lib/librte_eal/linuxapp/eal/eal_pci.c line 321

Quoted 1 line
lib/librte_eal/linuxapp/eal/eal_pci.c +1 -0
+		 * sysfs is invalid for AMD Opteron(TM) Processor 62xx and

63xx

lib/librte_eal/linuxapp/eal/eal_pci.c line 322

Quoted 1 line
lib/librte_eal/linuxapp/eal/eal_pci.c +1 -0
+		 * on Red Hat Enterprise Linux 6, and VMs on some

hypervisors.

Quoted 8 lines
lib/librte_eal/linuxapp/eal/eal_pci.c +4 -0
+		 * 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 */
#7 lore raw
Quoted 4 lines · Yong Wang

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.

#8 lore raw
In reply to Ferruh Yigit

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

Thanks.
Nick

Quoted 11 lines · Ferruh Yigit

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 <[email protected] <mailto:[email protected]>>

Thanks,
ferruh

#9 lore raw
In reply to nickcooper-zhangtonghao
Quoted 5 lines · On Thu, 5 Jan 2017 04:01:45 -0800 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.

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).

#10 lore raw
In reply to nickcooper-zhangtonghao

Review

Quoted 8 lines · On Thu, 5 Jan 2017 04:01:45 -0800 nickcooper-zhangtonghao…
(quoted diff) +7 -0
+		/* 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:

lib/librte_eal/linuxapp/eal/eal_pci.c +3 -10
index 43501342..9f09cd98 100644
@@ -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);
#11 lore raw

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

My legal name is “Nick Zhang”. So,

Thanks.
Nick

Quoted 36 lines · Stephen Hemminger

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:

lib/librte_eal/linuxapp/eal/eal_pci.c +3 -10
index 43501342..9f09cd98 100644
@@ -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);