Re: [PATCH V13 4/4] powerpc/vphn: Fix numa update end-loop bug

[Michael Bringmann <mwb@linux.vnet.ibm.com>]

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 <mwb@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
mwb@linux.vnet.ibm.com