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 1B76425A1 for ; Fri, 21 Jul 2017 17:37:10 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 4424C20931; Fri, 21 Jul 2017 11:37:10 -0400 (EDT) Received: from frontend2 ([10.202.2.161]) by compute1.internal (MEProxy); Fri, 21 Jul 2017 11:37:10 -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=ly5PWTKRk8+cgtf VYozOJQSwaEgtkIO738UaU71WftA=; b=YvPh5TjsoYDB5RBg9FqQri/nszEp9kC HI4vUJHu3qaM5MlrRSlBTnodVDypYXkp7agBbKjpV/qQNn6G+PaAxmpqdr6gN0ZA wDTgXO44YJ5pI/kCJQdAdgBqzvyjJgbc/fGkaxRBG9feqXP33OrLlxnPx0HK8unL JrzYAwXjC5k4= 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=ly5PWTKRk8+cgtfVYozOJQSwaEgtkIO738UaU71WftA=; b=hfu3uqO/ X7Csv1o5OqZ5rGom3cjyo82VFatZkKk/sDRAJ+3RtGPCFFkCs/LctgZuZLxLqxlD YiZqoX+iSmZOxB/7MCTVOnmagBgMfA1za8FwY6xTwqRKaiSUTf7Wj56SuQkZp36T sfipQS6a5hJFT5hnWaFV7rke1NkmWSAG/WCF1HAdCT4c79dZ6WhK1wM/TOgE3LMB OKcV1BYF5lGuQ141Bpju53d4cfVaDGjQLFhcX180cKlfMcCYCTDJfC+LQVNJo8HP /5zyNBKTDcVP1WtaFR40IaKdkNcnWROt2OxDnHoRTmjokQg4eIXQHJnPSOcWKZwD 2hMZT3fHOLG0sw== X-ME-Sender: X-Sasl-enc: LFXs4S794OH/0M+ZXiM8oAd8+zO3giFEPK2sPmVrzJa5 1500651429 Received: from xps.localnet (unknown [37.169.141.147]) by mail.messagingengine.com (Postfix) with ESMTPA id 99D6B240AF; Fri, 21 Jul 2017 11:37:09 -0400 (EDT) From: Thomas Monjalon To: Sergio Gonzalez Monroy Cc: dev@dpdk.org, nic@opencloud.tech Date: Fri, 21 Jul 2017 18:37:05 +0300 Message-ID: <7842270.506WcGqqe5@xps> In-Reply-To: References: <20170721091119.15701-1-sergio.gonzalez.monroy@intel.com> <1688226.sl7kgNVU6i@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 15:37:11 -0000 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)