Simplest change IMO: for_each_cpu(sibling, cpu_sibling_mask(cpu)) { ud = &updates[i++]; + ud->next = &updates[i]; ud->cpu = sibling; ud->new_nid = new_nid; ud->old_nid = numa_cpu_lookup_table[sibling]; cpumask_set_cpu(sibling, &updated_cpus); - if (i < weight) - ud->next = &updates[i]; } cpu = cpu_last_thread_sibling(cpu);
} if (i) updates[i-1].next = NULL; Link all of the updates together, and NULL the link pointer in the last entry to be filled in. No worries about invalid comparisons. Reduced code. Michael On 09/07/2017 08:35 AM, Nathan Fontenot wrote: > On 09/06/2017 05:03 PM, Michael Bringmann wrote: >> >> >> On 09/06/2017 09:45 AM, Nathan Fontenot wrote: >>> On 09/01/2017 10:48 AM, Michael Bringmann wrote: >>>> powerpc/vphn: On Power systems with shared configurations of CPUs >>>> and memory, there are some issues with the association of additional >>>> CPUs and memory to nodes when hot-adding resources. This patch >>>> fixes an end-of-updates processing problem observed occasionally >>>> in numa_update_cpu_topology(). >>>> >>>> Signed-off-by: Michael Bringmann <m...@linux.vnet.ibm.com> >>>> --- >>>> arch/powerpc/mm/numa.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c >>>> index 3a5b334..fccf23f 100644 >>>> --- a/arch/powerpc/mm/numa.c >>>> +++ b/arch/powerpc/mm/numa.c >>>> @@ -1410,6 +1410,13 @@ int numa_update_cpu_topology(bool cpus_locked) >>>> cpu = cpu_last_thread_sibling(cpu); >>>> } >>>> >>>> + /* >>>> + * Prevent processing of 'updates' from overflowing array >>>> + * in cases where last entry filled in a 'next' pointer. >>>> + */ >>>> + if (i) >>>> + updates[i-1].next = NULL; >>>> + >>> >>> This really looks like the bug is in the code above this where we >>> fill in the updates array for each of the sibling cpus. The code >>> there assumes that if the current update entry is not the end that >>> there will be more updates and blindly sets the next pointer. >>> >>> Perhaps correcting the logic in that code to next pointers. Set the >>> ud pointer to NULL before the outer for_each_cpu() loop. Then in the >>> inner for_each_cpu(sibling,...) loop update the ud-> next pointer as >>> the first operation. >>> >>> for_each_cpu(sibling, cpu_sibling_mask(cpu)) { >>> if (ud) >>> ud->next = &updates[i]; >>> ... >>> } >>> >>> Obviously untested, but I think this would prevent setting the next >>> pointer in the last update entry that is filled out erroneously. >> >> The above fragment looks to skip initialization of the 'next' pointer >> in the first element of the the 'updates'. That would abort subsequent >> evaluation of the array too soon, I believe. I would like to take another >> look >> to see whether the current check 'if (i < weight) ud->next = &updates[i];' >> is having problems due to i being 0-relative and weight being 1-relative. > > Another thing to keep in mind is that cpus can be skipped by checks earlier > in the loop. There is not guarantee that we will add 'weight' elements to > the ud list. > > -Nathan > >> >>> >>> -Nathan >> >> Michael >> >>> >>>> pr_debug("Topology update for the following CPUs:\n"); >>>> if (cpumask_weight(&updated_cpus)) { >>>> for (ud = &updates[0]; ud; ud = ud->next) { >>>> >>> >> > > -- Michael W. Bringmann Linux Technology Center IBM Corporation Tie-Line 363-5196 External: (512) 286-5196 Cell: (512) 466-0650 m...@linux.vnet.ibm.com