| Message ID | 20190629083629.29037-2-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ["RFC,PATCH",1/2] powerpc/mm: Fix node look up with numa=off boot | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (c7d64b560ce80d8c44f082eee8352f0778a73195) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 39 lines checked |
On 6/29/19 2:06 PM, Aneesh Kumar K.V wrote: > Update min_common_depth = -1 if numa is disabled. This > help us to avoid checking for both in different code paths. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/mm/numa.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c > index f6d68baeaa96..c84062a390cc 100644 > --- a/arch/powerpc/mm/numa.c > +++ b/arch/powerpc/mm/numa.c > @@ -212,7 +212,7 @@ static int associativity_to_nid(const __be32 *associativity) > { > int nid = NUMA_NO_NODE; > > - if (min_common_depth == -1 || !numa_enabled) > + if (min_common_depth == -1) > goto out; > > if (of_read_number(associativity, 1) >= min_common_depth) > @@ -625,6 +625,7 @@ static int __init parse_numa_properties(void) > > if (numa_enabled == 0) { > printk(KERN_WARNING "NUMA disabled by user\n"); > + min_common_depth = -1; > return -1; > } > > @@ -747,7 +748,7 @@ void __init dump_numa_cpu_topology(void) > unsigned int node; > unsigned int cpu, count; > > - if (min_common_depth == -1 || !numa_enabled) > + if (min_common_depth == -1) > return; > > for_each_online_node(node) { > @@ -812,7 +813,7 @@ static void __init find_possible_nodes(void) > struct device_node *rtas; > u32 numnodes, i; > > - if (min_common_depth <= 0 || !numa_enabled) > + if (min_common_depth <= 0) > return; > > rtas = of_find_node_by_path("/rtas"); > @@ -1014,7 +1015,7 @@ int hot_add_scn_to_nid(unsigned long scn_addr) > struct device_node *memory = NULL; > int nid; > > - if (!numa_enabled || (min_common_depth < 0)) > + if (min_common_depth < 0) > return first_online_node; > > memory = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory"); > I was not sure whether a reverse switch if better so that we have if (!numa_enabled) check every where and we do the below @@ -625,14 +624,15 @@ static int __init parse_numa_properties(void) if (numa_enabled == 0) { printk(KERN_WARNING "NUMA disabled by user\n"); - min_common_depth = -1; return -1; } min_common_depth = find_min_common_depth(); - if (min_common_depth < 0) + if (min_common_depth < 0) { + numa_enabled = false; return min_common_depth; + } -aneesh
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > Update min_common_depth = -1 if numa is disabled. This > help us to avoid checking for both in different code paths. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/mm/numa.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c > index f6d68baeaa96..c84062a390cc 100644 > --- a/arch/powerpc/mm/numa.c > +++ b/arch/powerpc/mm/numa.c > @@ -212,7 +212,7 @@ static int associativity_to_nid(const __be32 *associativity) > { > int nid = NUMA_NO_NODE; > > - if (min_common_depth == -1 || !numa_enabled) > + if (min_common_depth == -1) > goto out; > > if (of_read_number(associativity, 1) >= min_common_depth) > @@ -625,6 +625,7 @@ static int __init parse_numa_properties(void) > > if (numa_enabled == 0) { > printk(KERN_WARNING "NUMA disabled by user\n"); > + min_common_depth = -1; > return -1; > } I would prefer updating the definition of variable 'min_common_depth' to static int min_common_depth = -1; This would handle the case where someone calls 'associativity_to_nid()' and other functions that read 'min_common_depth' and get an invalid result back. And also handle the case where kernel is booted with 'numa = off'. Also the init value 'min_common_depth == 0' indicates that the first word in "ibm,associativity" array represents the node-id which is wrong. Instead its the length of the "ibm,associativity" array. > > @@ -747,7 +748,7 @@ void __init dump_numa_cpu_topology(void) > unsigned int node; > unsigned int cpu, count; > > - if (min_common_depth == -1 || !numa_enabled) > + if (min_common_depth == -1) > return; > > for_each_online_node(node) { > @@ -812,7 +813,7 @@ static void __init find_possible_nodes(void) > struct device_node *rtas; > u32 numnodes, i; > > - if (min_common_depth <= 0 || !numa_enabled) > + if (min_common_depth <= 0) > return; > > rtas = of_find_node_by_path("/rtas"); > @@ -1014,7 +1015,7 @@ int hot_add_scn_to_nid(unsigned long scn_addr) > struct device_node *memory = NULL; > int nid; > > - if (!numa_enabled || (min_common_depth < 0)) > + if (min_common_depth < 0) > return first_online_node; > > memory = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory"); > -- > 2.21.0 >
On 6/29/19 9:09 PM, Vaibhav Jain wrote: > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > >> Update min_common_depth = -1 if numa is disabled. This >> help us to avoid checking for both in different code paths. >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/mm/numa.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c >> index f6d68baeaa96..c84062a390cc 100644 >> --- a/arch/powerpc/mm/numa.c >> +++ b/arch/powerpc/mm/numa.c >> @@ -212,7 +212,7 @@ static int associativity_to_nid(const __be32 *associativity) >> { >> int nid = NUMA_NO_NODE; >> >> - if (min_common_depth == -1 || !numa_enabled) >> + if (min_common_depth == -1) >> goto out; >> >> if (of_read_number(associativity, 1) >= min_common_depth) >> @@ -625,6 +625,7 @@ static int __init parse_numa_properties(void) >> >> if (numa_enabled == 0) { >> printk(KERN_WARNING "NUMA disabled by user\n"); >> + min_common_depth = -1; >> return -1; >> } > > I would prefer updating the definition of variable 'min_common_depth' to > > static int min_common_depth = -1; > > This would handle the case where someone calls 'associativity_to_nid()' and > other functions that read 'min_common_depth' and get an invalid result > back. And also handle the case where kernel is booted with 'numa = off'. > Sure. As mentioned in another email, I am wondering whether all that min_common_depth check should be if !numa_enabled. That makes it much easy to read. I will respin once i get more clarity on of_drconf_to_nid_single usage. > Also the init value 'min_common_depth == 0' indicates that the > first word in "ibm,associativity" array represents the node-id which is > wrong. Instead its the length of the "ibm,associativity" array. > -aneesh
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c index f6d68baeaa96..c84062a390cc 100644 --- a/arch/powerpc/mm/numa.c +++ b/arch/powerpc/mm/numa.c @@ -212,7 +212,7 @@ static int associativity_to_nid(const __be32 *associativity) { int nid = NUMA_NO_NODE; - if (min_common_depth == -1 || !numa_enabled) + if (min_common_depth == -1) goto out; if (of_read_number(associativity, 1) >= min_common_depth) @@ -625,6 +625,7 @@ static int __init parse_numa_properties(void) if (numa_enabled == 0) { printk(KERN_WARNING "NUMA disabled by user\n"); + min_common_depth = -1; return -1; } @@ -747,7 +748,7 @@ void __init dump_numa_cpu_topology(void) unsigned int node; unsigned int cpu, count; - if (min_common_depth == -1 || !numa_enabled) + if (min_common_depth == -1) return; for_each_online_node(node) { @@ -812,7 +813,7 @@ static void __init find_possible_nodes(void) struct device_node *rtas; u32 numnodes, i; - if (min_common_depth <= 0 || !numa_enabled) + if (min_common_depth <= 0) return; rtas = of_find_node_by_path("/rtas"); @@ -1014,7 +1015,7 @@ int hot_add_scn_to_nid(unsigned long scn_addr) struct device_node *memory = NULL; int nid; - if (!numa_enabled || (min_common_depth < 0)) + if (min_common_depth < 0) return first_online_node; memory = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
Update min_common_depth = -1 if numa is disabled. This help us to avoid checking for both in different code paths. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- arch/powerpc/mm/numa.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)