eal: Set numa node value for system which not support NUMA.
Subsystems (2)
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
@@ -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
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
@@ -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
PATCH v2 3/5 vmxnet3: Avoid segfault caused by vmxnet3_dev_rx_queue_setup.
In reply to nickcooper-zhangtonghaoWe 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
@@ -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
PATCH v2 4/5 vmxnet3: Avoid memory leak in vmxnet3_dev_tx_queue_setup.
In reply to nickcooper-zhangtonghaoThis 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
@@ -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
PATCH v2 5/5 vmxnet3: Avoid segfault caused by vmxnet3_dev_tx_queue_setup.
In reply to nickcooper-zhangtonghaoWe 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
@@ -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
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.
Signed-off-by nickcooper-zhangtonghao <[email protected]>
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@@ -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 */
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
Signed-off-by nickcooper-zhangtonghao <[email protected]>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,
ferruhReview
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.clib/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 */
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.
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
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.
Signed-off-by nickcooper-zhangtonghao <[email protected]>
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).
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
@@ -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);
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@@ -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);