On 2019/9/2 15:25, Peter Zijlstra wrote: > On Mon, Sep 02, 2019 at 01:46:51PM +0800, Yunsheng Lin wrote: >> On 2019/9/1 0:12, Peter Zijlstra wrote: > >>> 1) because even it is not set, the device really does belong to a node. >>> It is impossible a device will have magic uniform access to memory when >>> CPUs cannot. >> >> So it means dev_to_node() will return either NUMA_NO_NODE or a >> valid node id? > > NUMA_NO_NODE := -1, which is not a valid node number. It is also, like I > said, not a valid device location on a NUMA system. > > Just because ACPI/BIOS is shit, doesn't mean the device doesn't have a > node association. It just means we don't know and might have to guess. How do we guess the device's location when ACPI/BIOS does not set it? It seems dev_to_node() does not do anything about that and leave the job to the caller or whatever function that get called with its return value, such as cpumask_of_node(). > >>> 2) is already true today, cpumask_of_node() requires a valid node_id. >> >> Ok, most of the user does check node_id before calling >> cpumask_of_node(), but does a little different type of checking: >> >> 1) some does " < 0" check; >> 2) some does "== NUMA_NO_NODE" check; >> 3) some does ">= MAX_NUMNODES" check; >> 4) some does "< 0 || >= MAX_NUMNODES || !node_online(node)" check. > > The one true way is: > > '(unsigned)node_id >= nr_node_ids' I missed the magic of the "unsigned" in your previous reply. > >>> 3) is just wrong and increases overhead for everyone. >> >> Ok, cpumask_of_node() is also used in some critical path such >> as scheduling, which may not need those checking, the overhead >> is unnecessary. >> >> But for non-critical path such as setup or configuration path, >> it better to have consistent checking, and also simplify the >> user code that calls cpumask_of_node(). >> >> Do you think it is worth the trouble to add a new function >> such as cpumask_of_node_check(maybe some other name) to do >> consistent checking? >> >> Or caller just simply check if dev_to_node()'s return value is >> NUMA_NO_NODE before calling cpumask_of_node()? > > It is not a matter of convenience. The function is called > cpumask_of_node(), when node < 0 || node >= nr_node_ids, it is not a > valid node, therefore the function shouldn't return anything except an > error. what do you mean by error? What I can think is three type of errors: 1) return NULL, this way it seems cpumask_of_node() also leave the job to the function that calls it. 2) cpu_none_mask, I am not sure what this means, maybe it means there is no cpu on the same node with the device? 3) give a warning, stack dump, or even a BUG_ON? I would prefer the second one, and implement the third one when the CONFIG_DEBUG_PER_CPU_MAPS is selected. Any suggestion? > > Also note that the CONFIG_DEBUG_PER_CPU_MAPS version of > cpumask_of_node() already does this (although it wants the below fix). Thanks for the note and example.
