Subject: Re: [PATCH V13 4/4] powerpc/vphn: Fix numa update end-loop bug
To: Michael Bringmann <mwb@linux.vnet.ibm.com>,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>,
        John Allen <jallen@linux.vnet.ibm.com>
References: <b967cb9b-efc4-3ff3-27c8-876a81250133@linux.vnet.ibm.com>
From: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Date: Wed, 6 Sep 2017 09:45:52 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <b967cb9b-efc4-3ff3-27c8-876a81250133@linux.vnet.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <0babc60c-8e66-e2f2-c316-c569a2ee1dfb@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1852
Lines: 53

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.
  
-Nathan

>  	pr_debug("Topology update for the following CPUs:\n");
>  	if (cpumask_weight(&updated_cpus)) {
>  		for (ud = &updates[0]; ud; ud = ud->next) {
>