From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH] pci: limit default numa node to used devices Date: Fri, 21 Jul 2017 18:37:05 +0300 Message-ID: <7842270.506WcGqqe5@xps> References: <20170721091119.15701-1-sergio.gonzalez.monroy@intel.com> <1688226.sl7kgNVU6i@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, nic@opencloud.tech To: Sergio Gonzalez Monroy Return-path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 1B76425A1 for ; Fri, 21 Jul 2017 17:37:10 +0200 (CEST) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 21/07/2017 18:03, Sergio Gonzalez Monroy: > On 21/07/2017 15:53, Thomas Monjalon wrote: > > The title and the text below should explain that you move > > the warning log from scan to probe, thanks to a temporary > > negative value. > > I thought that saying that I only check for devices managed by dpdk > explains the purpose, > and the patch itself shows the change from one file to another. It is obvious when you look carefully at the code, yes. I was just giving my help to better explain :) > > > 21/07/2017 12:11, Sergio Gonzalez Monroy: > >> Commit 8a04cb612589 ("pci: set default numa node for broken systems") > >> added logic to default to NUMA node 0 when sysfs numa_node information > >> was wrong or not available. > >> > >> Unfortunately there are many devices with wrong NUMA node information > >> that DPDK does not care about but still show warnings for them. > >> > >> Instead, only check for invalid NUMA node information for devices > >> managed by the DPDK. > >> > >> Signed-off-by: Sergio Gonzalez Monroy > > [...] > >> - if (eal_parse_sysfs_value(filename, &tmp) == 0 && > >> - tmp < RTE_MAX_NUMA_NODES) > >> + if (eal_parse_sysfs_value(filename, &tmp) == 0) > >> dev->device.numa_node = tmp; > > > > Why are you removing the check of the value? > > Are you going to accept invalid high values? > > This check was introduced on purpose by this commit: > > http://dpdk.org/commit/8a04cb6125 > > tmp is unsigned long type, so -1 is going to be a large number. Oh yes, I missed it was unsigned! > My understanding was that it was basically checking for -1 as numa_node. > > If we have valid numa_node greater than RTE_MAX_NUMA_NODES, defaulting > to 0 is not a good idea, is it? > > What I try to achieve with the patch is: > - if no numa_node avilable then parse is going to fail and we set -1. > - if numa_node is present but wrong, my understanding was that it would > be -1. All your explanations make sense when you realize that it is unsigned. I have one more question, Does it work to check for a negative value like this? if (dev->device.numa_node < 0)