Re: [RFC PATCH v3 4/4] arm64:numa: adding numa support for arm64 platforms.

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Wednesday 31 December 2014 13:03:28 Ganapatrao Kulkarni wrote:
> Adding numa support for arm64 based platforms.
> Adding dt node pasring for numa topology using property arm,associativity.
> 
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>

Maybe the parts that are common with powerpc can be moved to drivers/of/numa.c?
We can always look for both arm,associativity and ibm,associativity, I don't
think we should be worried about any conflicts that way.

> +#define MAX_DISTANCE_REF_POINTS 4

I think we should use 8 here like powerpc, four levels might get exceeded
on complex SoCs.

> +int dt_get_cpu_node_id(int cpu)
> +{
> +	struct device_node *dn = NULL;
> +
> +	while ((dn = of_find_node_by_type(dn, "cpu"))) {
> +		const u32 *cell;
> +		u64 hwid;
> +
> +		/*
> +		 * A cpu node with missing "reg" property is
> +		 * considered invalid to build a cpu_logical_map
> +		 * entry.
> +		 */
> +		cell = of_get_property(dn, "reg", NULL);
> +		if (!cell) {
> +			pr_err("%s: missing reg property\n", dn->full_name);
> +			return default_nid;
> +		}
> +		hwid = of_read_number(cell, of_n_addr_cells(dn));
> +
> +		if (cpu_logical_map(cpu) == hwid)
> +		return of_node_to_nid_single(dn);
> +	}
> +	return NUMA_NO_NODE;
> +}
> +EXPORT_SYMBOL(dt_get_cpu_node_id);

Maybe just expose a function to the device node for a CPU ID here, and
expect callers to use of_node_to_nid?

> +
> +/**
> + * early_init_dt_scan_numa_map - parse memory node and map nid to memory range.
> + */
> +int __init early_init_dt_scan_numa_map(unsigned long node, const char *uname,
> +				     int depth, void *data)
> +{
> +	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
> +
> +	/* We are scanning "numa-map" nodes only */

a stale comment?

> +/* DT node mapping is done already early_init_dt_scan_memory */
> +int __init arm64_dt_numa_init(void)
> +{
> +	int i;
> +	u32 nodea, nodeb, distance, node_count = 0;
> +
> +	of_scan_flat_dt(early_init_dt_scan_numa_map, NULL);
> +
> +	for_each_node_mask(i, numa_nodes_parsed)
> +		node_count = i;
> +	node_count++;
> +
> +	for (nodea =  0; nodea < node_count; nodea++) {
> +		for (nodeb = 0; nodeb < node_count; nodeb++) {
> +			distance = dt_get_node_distance(nodea, nodeb);
> +			numa_set_distance(nodea, nodeb, distance);
> +		}
> +	}
> +	return 0;
> +}
> +EXPORT_SYMBOL(arm64_dt_numa_init);

No need to export functions that are called only be architecture code.
Since this works on the flattened device tree format, you can never
have loadable modules calling it.

> @@ -461,7 +464,12 @@ static int c_show(struct seq_file *m, void *v)
>  		 * "processor".  Give glibc what it expects.
>  		 */
>  #ifdef CONFIG_SMP
> +	if (IS_ENABLED(CONFIG_NUMA)) {
> +		seq_printf(m, "processor\t: %d", i);
> +		seq_printf(m, " [nid: %d]\n", cpu_to_node(i));
> +	} else {
>  		seq_printf(m, "processor\t: %d\n", i);
> +	}
>  #endif
>  	}

Do we need to make this conditional? I think we can just always
print the node number, even if it's going to be zero for systems
without the associativity properties.

> +
> +int cpu_to_node_map[NR_CPUS];
> +EXPORT_SYMBOL(cpu_to_node_map);

This seems to be x86 specific, do we need it?

> +/*
> + *  Set the cpu to node and mem mapping
> + */
> +void numa_store_cpu_info(int cpu)
> +{
> +#ifdef CONFIG_ARM64_DT_NUMA
> +	node_cpu_hwid[cpu].node_id  =  dt_get_cpu_node_id(cpu);
> +#endif

I would try to avoid the #ifdef here, by providing a stub function of
dt_get_cpu_node_id or whichever function we end up calling here when
NUMA is disabled.

> +
> +/**
> + * arm64_numa_init - Initialize NUMA
> + *
> + * Try each configured NUMA initialization method until one succeeds.  The
> + * last fallback is dummy single node config encomapssing whole memory and
> + * never fails.
> + */
> +void __init arm64_numa_init(void)
> +{
> +	if (!numa_off) {
> +#ifdef CONFIG_ARM64_DT_NUMA
> +		if (!numa_init(arm64_dt_numa_init))
> +			return;
> +#endif
> +	}
> +
> +	numa_init(dummy_numa_init);
> +}

I don't think we need the CONFIG_ARM64_DT_NUMA=n case here, it should just
not be conditional, and the arm64_dt_numa_init should fall back to doing
something reasonable when numa is turned off or there are no associativity
properties.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html