DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
Cc: dev@dpdk.org, nic@opencloud.tech
Subject: Re: [dpdk-dev] [PATCH] pci: limit default numa node to used devices
Date: Fri, 21 Jul 2017 18:37:05 +0300	[thread overview]
Message-ID: <7842270.506WcGqqe5@xps> (raw)
In-Reply-To: <d0b81ffa-e75c-7b51-17da-27b04b949536@intel.com>

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 <sergio.gonzalez.monroy@intel.com>
> > [...]
> >> -	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)

  reply	other threads:[~2017-07-21 15:37 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-21  9:11 Sergio Gonzalez Monroy
2017-07-21 14:53 ` Thomas Monjalon
2017-07-21 15:03   ` Sergio Gonzalez Monroy
2017-07-21 15:37     ` Thomas Monjalon [this message]
2017-07-21 15:47       ` Sergio Gonzalez Monroy
2017-07-21 16:26         ` Thomas Monjalon
2017-07-21 16:29 ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7842270.506WcGqqe5@xps \
    --to=thomas@monjalon.net \
    --cc=dev@dpdk.org \
    --cc=nic@opencloud.tech \
    --cc=sergio.gonzalez.monroy@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).