Re: [PATCH v12 4/5] arm64, numa: Add NUMA support for arm64 platforms.

[David Daney <ddaney@caviumnetworks.com>]

On 02/26/2016 10:53 AM, Will Deacon wrote:
[...]
>> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
>> new file mode 100644
>> index 0000000..604e886
>> --- /dev/null
>> +++ b/arch/arm64/mm/numa.c
>> @@ -0,0 +1,403 @@
[...]
>> +
>> +static int numa_off;
>> +static int numa_distance_cnt;
>> +static u8 *numa_distance;
>> +
>> +static __init int numa_parse_early_param(char *opt)
>> +{
>> +	if (!opt)
>> +		return -EINVAL;
>> +	if (!strncmp(opt, "off", 3)) {
>> +		pr_info("%s\n", "NUMA turned off");
>> +		numa_off = 1;
>> +	}
>> +	return 0;
>> +}
>> +early_param("numa", numa_parse_early_param);
>
> Curious, but when is this option actually useful?
>

Good point.  I will remove that bit, it was used as an aid in debugging 
while bringing up the patch set.


>> +
>> +cpumask_var_t node_to_cpumask_map[MAX_NUMNODES];
>> +EXPORT_SYMBOL(node_to_cpumask_map);
>> +
>> +#ifdef CONFIG_DEBUG_PER_CPU_MAPS
>> +
>> +/*
>> + * Returns a pointer to the bitmask of CPUs on Node 'node'.
>> + */
>> +const struct cpumask *cpumask_of_node(int node)
>> +{
>> +	if (WARN_ON(node >= nr_node_ids))
>> +		return cpu_none_mask;
>> +
>> +	if (WARN_ON(node_to_cpumask_map[node] == NULL))
>> +		return cpu_online_mask;
>> +
>> +	return node_to_cpumask_map[node];
>> +}
>> +EXPORT_SYMBOL(cpumask_of_node);
>> +
>> +#endif
>> +
>> +static void map_cpu_to_node(unsigned int cpu, int nid)
>> +{
>> +	set_cpu_numa_node(cpu, nid);
>> +	if (nid >= 0)
>> +		cpumask_set_cpu(cpu, node_to_cpumask_map[nid]);
>> +}
>> +
>> +static void unmap_cpu_to_node(unsigned int cpu)
>> +{
>> +	int nid = cpu_to_node(cpu);
>> +
>> +	if (nid >= 0)
>> +		cpumask_clear_cpu(cpu, node_to_cpumask_map[nid]);
>> +	set_cpu_numa_node(cpu, NUMA_NO_NODE);
>> +}
>
> How do you end up with negative nids this late in the game?
>

It might be possible with some of the hot plugging code.  It is a little 
paranoia programming.

If you really don't like it, we can remove it.

>> +
>> +void numa_clear_node(unsigned int cpu)
>> +{
>> +	unmap_cpu_to_node(cpu);
>
> Why don't you just inline this function?

Good point, I will do that.

[...]
>> +int __init numa_add_memblk(int nid, u64 start, u64 size)
>> +{
>> +	int ret;
>> +
>> +	ret = memblock_set_node(start, size, &memblock.memory, nid);
>> +	if (ret < 0) {
>> +		pr_err("NUMA: memblock [0x%llx - 0x%llx] failed to add on node %d\n",
>> +			start, (start + size - 1), nid);
>> +		return ret;
>> +	}
>> +
>> +	node_set(nid, numa_nodes_parsed);
>> +	pr_info("NUMA: Adding memblock [0x%llx - 0x%llx] on node %d\n",
>> +			start, (start + size - 1), nid);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(numa_add_memblk);
>
> But this is marked __init... (and you've done this elsewhere in the patch
> too).

I will fix these.

>
> Will
>