From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 12BC6271 for ; Fri, 21 Jul 2017 18:26:21 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id ADC4620231; Fri, 21 Jul 2017 12:26:20 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Fri, 21 Jul 2017 12:26:20 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=MX4wkfp2jNXXe3e f2cBYYhlq3kl5M5M28Iw6N4IrHfg=; b=VH5oLW+tx0fjsPK6YXFO+1SCnFoD9Wj B/1LWNYiodK7dIOUCb5dH/nfeOTJqeqWNQ0PvYuyRPvX7nQceoCMfN7ligzevxpE 9+Wvn7Q/5jKjd64vcP4UWzvbz/XdVqhn3hlKfn5dd8we3xKKMSSL/J9cJPCqH/TB ae/lnmqmrt8s= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=MX4wkfp2jNXXe3ef2cBYYhlq3kl5M5M28Iw6N4IrHfg=; b=XtA6Cld6 1ci7oaK0KVmVfA4pBrFbS6v9rK0vPTb0hrnnfN50mqalq2dKCTLBtbnBU89SAIP4 M4GrzNJW07lpmlmtI/by9x8otpH/Ozkg6qdcHmL1WacBfZkwPeSbZirZgQ+a/hA+ 7bXibtwU3BIUMSEefEo0U6MpfYzBXPuFqielNM13belYosWtWbw/XYQfpXECcS7H HI7uz451/DDaeB/sLijeWH2ajlw5yTUJgs7+RNccnuqV+IS1crI+mh50dYsVuhc9 /xJmoqJQ+SH9aOJdwILA3Hzm9+77a5uOJpPhAs2byV/TdrOZ4jY2XfhbI6r08kbA YI+uPj3+qCHWnw== X-ME-Sender: X-Sasl-enc: dQPUd/piw5n87mMaEWvIfeg4cvOBCjF5FHbJZjtu7OhG 1500654380 Received: from xps.localnet (unknown [37.163.5.152]) by mail.messagingengine.com (Postfix) with ESMTPA id 04E40240AF; Fri, 21 Jul 2017 12:26:20 -0400 (EDT) From: Thomas Monjalon To: Sergio Gonzalez Monroy Cc: dev@dpdk.org, nic@opencloud.tech Date: Fri, 21 Jul 2017 19:26:11 +0300 Message-ID: <1682729.KlDdaL09u8@xps> In-Reply-To: References: <20170721091119.15701-1-sergio.gonzalez.monroy@intel.com> <7842270.506WcGqqe5@xps> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] pci: limit default numa node to used devices X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Jul 2017 16:26:21 -0000 21/07/2017 18:47, Sergio Gonzalez Monroy: > On 21/07/2017 16:37, Thomas Monjalon wrote: > > 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 :) > > Just giving my view of the commit message > If you think it can be improve,by all meansfeel free to change it :) > > >>> 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) > > numa_node is signed int type in struct rte_device, so it should work. OK, sorry I overlooked :)