Re: [PATCH V13 3/4] powerpc/hotplug: Improve responsiveness of hotplug change

[Nathan Fontenot]

On 09/01/2017 10:48 AM, Michael Bringmann wrote:
> powerpc/hotplug: 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.  During hotplug
> CPU operations, this patch resets the timer on topology update work
> function to a small value to better ensure that the CPU topology is
> detected and configured sooner.

Looking through the changes you've made here I don't see where the
topology timeout ever gets set to the default timeout. When calculating
the next timeout you use topology_timer_secs which is initialized to 1, so
the timer pops every second after initialization. Then after a dlpar cpu
operation the timer is set to pop every second. There is no place that I
see where the timeout is set to the default 60 seconds.

> 
> Signed-off-by: Michael Bringmann <mwb@xxxxxxxxxxxxxxxxxx>
> ---
>  arch/powerpc/include/asm/topology.h          |    8 ++++++++
>  arch/powerpc/mm/numa.c                       |   21 ++++++++++++++++++++-
>  arch/powerpc/platforms/pseries/hotplug-cpu.c |    2 ++
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index dc4e159..beb9bca 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -98,6 +98,14 @@ static inline int prrn_is_enabled(void)
>  }
>  #endif /* CONFIG_NUMA && CONFIG_PPC_SPLPAR */
> 
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_NEED_MULTIPLE_NODES)
> +#if defined(CONFIG_PPC_SPLPAR)
> +extern int timed_topology_update(int nsecs);
> +#else
> +#define	timed_topology_update(nsecs)
> +#endif /* CONFIG_PPC_SPLPAR */
> +#endif /* CONFIG_HOTPLUG_CPU || CONFIG_NEED_MULTIPLE_NODES */
> +
>  #include <asm-generic/topology.h>
> 
>  #ifdef CONFIG_SMP
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index c08d736..3a5b334 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -1148,15 +1148,34 @@ struct topology_update_data {
>  	int new_nid;
>  };
> 
> +#define TOPOLOGY_DEF_TIMER_SECS	60
> +
>  static u8 vphn_cpu_change_counts[NR_CPUS][MAX_DISTANCE_REF_POINTS];
>  static cpumask_t cpu_associativity_changes_mask;
>  static int vphn_enabled;
>  static int prrn_enabled;
>  static void reset_topology_timer(void);
> +static int topology_timer_secs = 1;
>  static int topology_inited;
>  static int topology_update_needed;
> 
>  /*
> + * Change polling interval for associativity changes.
> + */
> +int timed_topology_update(int nsecs)
> +{
> +	if (nsecs > 0)
> +		topology_timer_secs = nsecs;
> +	else
> +		topology_timer_secs = TOPOLOGY_DEF_TIMER_SECS;
> +
> +	if (vphn_enabled)
> +		reset_topology_timer();

Should this whole thing be wrapped by if (vphn_enabled) ?

-Nathan

> +
> +	return 0;
> +}
> +
> +/*
>   * Store the current values of the associativity change counters in the
>   * hypervisor.
>   */
> @@ -1489,7 +1508,7 @@ static void topology_timer_fn(unsigned long ignored)
>  static void reset_topology_timer(void)
>  {
>  	topology_timer.data = 0;
> -	topology_timer.expires = jiffies + 60 * HZ;
> +	topology_timer.expires = jiffies + topology_timer_secs * HZ;
>  	mod_timer(&topology_timer, topology_timer.expires);
>  }
> 
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 6afd1ef..5a7fb1e 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -356,6 +356,7 @@ static int dlpar_online_cpu(struct device_node *dn)
>  			BUG_ON(get_cpu_current_state(cpu)
>  					!= CPU_STATE_OFFLINE);
>  			cpu_maps_update_done();
> +			timed_topology_update(1);
>  			rc = device_online(get_cpu_device(cpu));
>  			if (rc)
>  				goto out;
> @@ -522,6 +523,7 @@ static int dlpar_offline_cpu(struct device_node *dn)
>  				set_preferred_offline_state(cpu,
>  							    CPU_STATE_OFFLINE);
>  				cpu_maps_update_done();
> +				timed_topology_update(1);
>  				rc = device_offline(get_cpu_device(cpu));
>  				if (rc)
>  					goto out;
>