linux-kernel - Re: [PATCH V13 1/4] powerpc/vphn: Update CPU topology when VPHN enabled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <ed69e975-2282-0a50-484c-84db5156583e@linux.vnet.ibm.com>
Date:   Wed, 6 Sep 2017 09:20:23 -0500
From:   Nathan Fontenot <nfont@...ux.vnet.ibm.com>
To:     Michael Bringmann <mwb@...ux.vnet.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        John Allen <jallen@...ux.vnet.ibm.com>
Subject: Re: [PATCH V13 1/4] powerpc/vphn: Update CPU topology when VPHN
 enabled

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
> corrects the currently broken capability to set the topology for
> shared CPUs in LPARs.  At boot time for shared CPU lpars, the
> topology for each CPU was being set to node zero.  Now when
> numa_update_cpu_topology() is called appropriately, the Virtual
> Processor Home Node (VPHN) capabilities information provided by the
> pHyp allows the appropriate node in the shared configuration to be
> selected for the CPU.
> 
> Signed-off-by: Michael Bringmann <mwb@...ux.vnet.ibm.com>
> ---
> Changes in V13:
>   -- Split patch for improved review
> ---
>  arch/powerpc/mm/numa.c |   31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index b95c584..312f6ee 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1153,6 +1153,8 @@ struct topology_update_data {
>  static int vphn_enabled;
>  static int prrn_enabled;
>  static void reset_topology_timer(void);
> +static int topology_inited;
> +static int topology_update_needed;
> 
>  /*
>   * Store the current values of the associativity change counters in the
> @@ -1246,6 +1248,11 @@ static long vphn_get_associativity(unsigned long cpu,
>  			"hcall_vphn() experienced a hardware fault "
>  			"preventing VPHN. Disabling polling...\n");
>  		stop_topology_update();
> +		break;
> +	case H_SUCCESS:
> +		dbg("VPHN hcall succeeded. Reset polling...\n");
> +		timed_topology_update(0);
> +		break;
>  	}
> 
>  	return rc;
> @@ -1323,8 +1330,11 @@ int numa_update_cpu_topology(bool cpus_locked)
>  	struct device *dev;
>  	int weight, new_nid, i = 0;
> 
> -	if (!prrn_enabled && !vphn_enabled)
> +	if (!prrn_enabled && !vphn_enabled) {
> +		if (!topology_inited)
> +			topology_update_needed = 1;
>  		return 0;
> +	}
> 
>  	weight = cpumask_weight(&cpu_associativity_changes_mask);
>  	if (!weight)
> @@ -1363,6 +1373,8 @@ int numa_update_cpu_topology(bool cpus_locked)
>  			cpumask_andnot(&cpu_associativity_changes_mask,
>  					&cpu_associativity_changes_mask,
>  					cpu_sibling_mask(cpu));
> +			pr_info("Assoc chg gives same node %d for cpu%d\n",
> +					new_nid, cpu);

As mentioned previously, this should either be removed or changed to a
debug statement.

>  			cpu = cpu_last_thread_sibling(cpu);
>  			continue;
>  		}
> @@ -1433,6 +1445,7 @@ int numa_update_cpu_topology(bool cpus_locked)
> 
>  out:
>  	kfree(updates);
> +	topology_update_needed = 0;
>  	return changed;
>  }
> 
> @@ -1453,6 +1466,13 @@ static void topology_schedule_update(void)
>  	schedule_work(&topology_work);
>  }
> 
> +void shared_topology_update(void)
> +{
> +	if (firmware_has_feature(FW_FEATURE_VPHN) &&
> +		   lppaca_shared_proc(get_lppaca()))

Could we just check vphn_enabled here? The init routine will set this to true
only if the feature is supported and we are using shared processors.

Also, this routine seems a bit odd. The only place it is called from is an
init routine, topology_update_init. Is there a reason this check couldn't just
be in that routine, or it seems like you could just call topology_schedule_update
directly from start_topology_update when vphn is initialized.

-Nathan

> +		topology_schedule_update();
> +}
> +
>  static void topology_timer_fn(unsigned long ignored)
>  {
>  	if (prrn_enabled && cpumask_weight(&cpu_associativity_changes_mask))
> @@ -1519,7 +1539,6 @@ int start_topology_update(void)
>  	if (firmware_has_feature(FW_FEATURE_PRRN)) {
>  		if (!prrn_enabled) {
>  			prrn_enabled = 1;
> -			vphn_enabled = 0;
>  #ifdef CONFIG_SMP
>  			rc = of_reconfig_notifier_register(&dt_update_nb);
>  #endif
> @@ -1527,7 +1546,6 @@ int start_topology_update(void)
>  	} else if (firmware_has_feature(FW_FEATURE_VPHN) &&
>  		   lppaca_shared_proc(get_lppaca())) {
>  		if (!vphn_enabled) {
> -			prrn_enabled = 0;
>  			vphn_enabled = 1;
>  			setup_cpu_associativity_change_counters();
>  			init_timer_deferrable(&topology_timer);
> @@ -1613,9 +1631,16 @@ static int topology_update_init(void)
>  	if (topology_updates_enabled)
>  		start_topology_update();
> 
> +	shared_topology_update();
> +
>  	if (!proc_create("powerpc/topology_updates", 0644, NULL, &topology_ops))
>  		return -ENOMEM;
> 
> +	topology_inited = 1;
> +	if (topology_update_needed)
> +		bitmap_fill(cpumask_bits(&cpu_associativity_changes_mask),
> +					nr_cpumask_bits);
> +
>  	return 0;
>  }
>  device_initcall(topology_update_init);
>