Re: [PATCH v2 3/3] powerpc/numa: Support coregroup on PowerNV

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Srikar Dronamraju <srikar@linux.ibm.com> writes:

> Coregroup support on powerpc has so far been limited to PowerVM LPARs.
> However, PowerNV can also support coregroups when firmware exposes the
> required coregroup information through the associativity hierarchy.
>
> Detect coregroup support by checking whether primary_domain_index is the
> penultimate domain in the CPU node's ibm,associativity property. On
> PowerNV, a non-penultimate primary_domain_index indicates that firmware
> provides an additional level for coregroup information.
>
> This keeps the logic compatible with PowerVM systems, where
> primary_domain_index is likewise not the penultimate associativity
> domain.
>
> Signed-off-by: Srikar Dronamraju <srikar@linux.ibm.com>
> ---
> Changelog from v1: https://lkml.kernel.org/r/20260524010017.140408-1-srikar@linux.ibm.com
> - Handle comments from Christophe Leroy; make code more flat
>
>  arch/powerpc/mm/numa.c | 56 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 9aa71eb7e96b..e97b624203ea 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -889,12 +889,32 @@ static int __init numa_setup_drmem_lmb(struct drmem_lmb *lmb,
>  	return 0;
>  }
>  
> +/*
> + * If hierarchy extends beyond primary_domain_index + 1, then next
> + * level corresponds to coregroup.
> + */
> +static int detect_and_enable_coregroup(const __be32 *associativity, int index)

Do we care about it's return value? We are not reading that in the
patch.
this function is mainly only needed in __init, can we mark it so.

> +{
> +	if (!associativity || index == -1)
> +		goto out;
> +
> +	index = of_read_number(associativity, 1);
> +
> +	if (index > primary_domain_index + 1) {
> +		coregroup_enabled = 1;
> +		return index;
> +	}
> +out:
> +	coregroup_enabled = 0;
> +	return -1;
> +}

For PowerVM, we now have two places which will enable coregroup_enabled
during mem_topology_setup(). Is there some way we can unify that?

This also means we enable coregroup in case of PowerVM with SPLPAR when
per-cpu VPHN associativity index > primary_domain_index+1. But this
isn't reflected in your commit msg. The commit msg only says this
affects PowerNV.

setup_arch
  mem_topology_setup
    parse_numa_properties
      detect_and_enable_coregroup() {...
       // coregroup_enabled = 0/1
        	index = of_read_number(associativity, 1);

	        if (index > primary_domain_index + 1) {
            		coregroup_enabled = 1;
                    		return index;
            }

      }
    <...>
    find_possible_nodes() {...
        	prop_length /= sizeof(int);
	        if (prop_length > primary_domain_index + 2)
		       coregroup_enabled = 1;
    }

> +
>  static int __init parse_numa_properties(void)
>  {
>  	struct device_node *memory, *pci;
> -	int default_nid = 0;
> -	unsigned long i;
> +	int default_nid = 0, index = 0;
>  	const __be32 *associativity;
> +	unsigned long i;
>  
>  	if (numa_enabled == 0) {
>  		pr_warn("disabled by user\n");
> @@ -927,7 +947,6 @@ static int __init parse_numa_properties(void)
>  	 */
>  	for_each_present_cpu(i) {
>  		__be32 vphn_assoc[VPHN_ASSOC_BUFSIZE];
> -		struct device_node *cpu;
>  		int nid = NUMA_NO_NODE;
>  
>  		memset(vphn_assoc, 0, VPHN_ASSOC_BUFSIZE * sizeof(__be32));
> @@ -935,7 +954,9 @@ static int __init parse_numa_properties(void)
>  		if (__vphn_get_associativity(i, vphn_assoc) == 0) {
>  			nid = associativity_to_nid(vphn_assoc);
>  			initialize_form1_numa_distance(vphn_assoc);
> +			index = detect_and_enable_coregroup(vphn_assoc, index);
>  		} else {
> +			struct device_node *cpu;
>  
>  			/*
>  			 * Don't fall back to default_nid yet -- we will plug
> @@ -948,6 +969,7 @@ static int __init parse_numa_properties(void)
>  			associativity = of_get_associativity(cpu);
>  			if (associativity) {
>  				nid = associativity_to_nid(associativity);
> +				index = detect_and_enable_coregroup(associativity, index);
>  				initialize_form1_numa_distance(associativity);
>  			}
>  			of_node_put(cpu);
> @@ -1445,7 +1467,9 @@ static long vphn_get_associativity(unsigned long cpu,
>  
>  int cpu_to_coregroup_id(int cpu)
>  {
> -	__be32 associativity[VPHN_ASSOC_BUFSIZE] = {0};
> +	int coregroup_id = cpu_to_core_id(cpu);
> +	struct device_node *cpunode = NULL;
> +	const __be32 *associativity;
>  	int index;
>  
>  	if (cpu < 0 || cpu > nr_cpu_ids)
> @@ -1454,19 +1478,31 @@ int cpu_to_coregroup_id(int cpu)
>  	if (!coregroup_enabled)
>  		goto out;
>  
> -	if (!firmware_has_feature(FW_FEATURE_VPHN))
> -		goto out;
> +	if (firmware_has_feature(FW_FEATURE_VPHN)) {
> +		__be32 tmp[VPHN_ASSOC_BUFSIZE] = {0};
>  
> -	if (vphn_get_associativity(cpu, associativity))
> +		if (vphn_get_associativity(cpu, tmp))
> +			goto out;
> +
> +		associativity = tmp;
> +
> +	} else {
> +		cpunode = of_get_cpu_node(cpu, NULL);
> +		if (!cpunode)
> +			goto out;
> +
> +		associativity = of_get_associativity(cpunode);
> +	}
> +	if (!associativity)
>  		goto out;
>  
>  	index = of_read_number(associativity, 1);
>  	if (index > primary_domain_index + 1)
> -		return of_read_number(&associativity[index - 1], 1);
> +		coregroup_id = of_read_number(&associativity[index - 1], 1);
>  
>  out:
> -	return cpu_to_core_id(cpu);
> -}

Looks like leftover removed from previous patch. This change should be
fixed in patch-2 itself.

> +	if (cpunode)
> +		of_node_put(cpunode);
>  
>  	return coregroup_id;
>  }
> -- 
> 2.43.0

-ritesh