From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH v2 1/5] eal: Set numa node value for system which not support NUMA. Date: Thu, 5 Jan 2017 08:26:33 -0800 Message-ID: <20170105082633.48fc19df@xeon-e3> References: <1483617709-7088-1-git-send-email-nic@opencloud.tech> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org To: nickcooper-zhangtonghao Return-path: Received: from mail-pg0-f50.google.com (mail-pg0-f50.google.com [74.125.83.50]) by dpdk.org (Postfix) with ESMTP id AAA43D4E0 for ; Thu, 5 Jan 2017 17:26:42 +0100 (CET) Received: by mail-pg0-f50.google.com with SMTP id g1so202827240pgn.0 for ; Thu, 05 Jan 2017 08:26:42 -0800 (PST) In-Reply-To: <1483617709-7088-1-git-send-email-nic@opencloud.tech> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, 5 Jan 2017 04:01:45 -0800 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. > + * 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);