Re: [PATCH V9 2/2] powerpc/nodes: Ensure enough nodes avail for operations

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

On 08/21/2017 04:44 PM, Michael Bringmann wrote:
> To: linuxppc-dev@lists.ozlabs.org
> 
> From: Michael Bringmann <mwb@linux.vnet.ibm.com>
> 
> To: linux-kernel@vger.kernel.org
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Michael Bringmann <mwb@linux.vnet.ibm.com>
> Cc: John Allen <jallen@linux.vnet.ibm.com>
> Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> Subject: [PATCH V9 2/2] powerpc/nodes: Ensure enough nodes avail for operations
> 
> powerpc/nodes: On systems like PowerPC which allow 'hot-add' of CPU
> or memory resources, it may occur that the new resources are to be
> inserted into nodes that were not used for these resources at bootup.
> In the kernel, any node that is used must be defined and initialized
> at boot.
> 
> This patch extracts the value of the lowest domain level (number of
> allocable resources) from the "rtas" device tree property
> "ibm,max-associativity-domains" to use as the maximum number of nodes
> to setup as possibly available in the system.  This new setting will
> override the instruction,
> 
>     nodes_and(node_possible_map, node_possible_map, node_online_map);
> 
> presently seen in the function arch/powerpc/mm/numa.c:initmem_init().
> 
> If the property is not present at boot, no operation will be performed
> to define or enable additional nodes.
> 
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/numa.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 3fd4536..3ae6510 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -893,6 +893,48 @@ static void __init setup_node_data(int nid, u64 start_pfn, u64 end_pfn)
>  	NODE_DATA(nid)->node_spanned_pages = spanned_pages;
>  }
> 
> +static void __init node_associativity_setup(void)
> +{
> +	struct device_node *rtas;
> +	printk(KERN_INFO "%s:%d\n", __FUNCTION__, __LINE__);

Is there a reson we need to have all these KERN_INFO printk's?

This looks like debug statements that accidentally were left in.

> +
> +	rtas = of_find_node_by_path("/rtas");
> +	if (rtas) {
> +		const __be32 *prop;
> +		u32 len, entries, levelval, i;
> +	printk(KERN_INFO "%s:%d\n", __FUNCTION__, __LINE__);
> +
> +		prop = of_get_property(rtas, "ibm,max-associativity-domains", &len);

You could put the of_node_put() call here after getting the property and get
rid of all the goto's.

> +		if (!prop || len < sizeof(unsigned int)) {
> +	printk(KERN_INFO "%s:%d\n", __FUNCTION__, __LINE__);
> +			goto endit;
> +		}
> +
> +		entries = of_read_number(prop++, 1);
> +
> +		if (len < (entries * sizeof(unsigned int))) {
> +	printk(KERN_INFO "%s:%d\n", __FUNCTION__, __LINE__);
> +			goto endit;
> +		}
> +
> +		for (i = 0; i < entries; i++)
> +			levelval = of_read_number(prop++, 1);

Couldn't you just read the last enbtry instead of doing a loop reading each
entry until you get to the last one?

-Nathan

> +
> +		printk(KERN_INFO "Numa nodes avail: %d (%d) \n", (int) levelval, (int) entries);
> +
> +		for (i = 0; i < levelval; i++) {
> +			if (!node_possible(i)) {
> +				setup_node_data(i, 0, 0);
> +				node_set(i, node_possible_map);
> +			}
> +		}
> +	}
> +
> +endit:
> +	if (rtas)
> +		of_node_put(rtas)> +}
> +
>  void __init initmem_init(void)
>  {
>  	int nid, cpu;
> @@ -912,6 +954,8 @@ void __init initmem_init(void)
>  	 */
>  	nodes_and(node_possible_map, node_possible_map, node_online_map);
> 
> +	node_associativity_setup();
> +
>  	for_each_online_node(nid) {
>  		unsigned long start_pfn, end_pfn;
>