Re: [PATCH v2] powerpc: Warn about use of smt_snooze_delay

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

Hi Joel,

On Tue, Jun 30, 2020 at 11:29:35AM +0930, Joel Stanley wrote:
> It's not done anything for a long time. Save the percpu variable, and
> emit a warning to remind users to not expect it to do anything.
> 
> Fixes: 3fa8cad82b94 ("powerpc/pseries/cpuidle: smt-snooze-delay cleanup.")
> Cc: stable@vger.kernel.org # v3.14
> Signed-off-by: Joel Stanley <joel@jms.id.au>


Sorry I missed this v2.

The patch looks good to me.

Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> --
> v2:
>  Use pr_warn instead of WARN
>  Reword and print proccess name with pid in message
>  Leave CPU_FTR_SMT test in
>  Add Fixes line
> 
> mpe, if you don't agree then feel free to drop the cc stable.
> 
> Testing 'ppc64_cpu --smt=off' on a 24 core / 4 SMT system it's quite noisy
> as the online/offline loop that ppc64_cpu runs is slow.
> 
> This could be fixed by open coding pr_warn_ratelimit with the ratelimit
> parameters tweaked if someone was concerned. I'll leave that to someone
> else as a future enhancement.
> 
> [  237.642088][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642175][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642261][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642345][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642430][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642516][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642625][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642709][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642793][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  237.642878][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264030][ T1197] store_smt_snooze_delay: 14 callbacks suppressed
> [  254.264033][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264048][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264062][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264075][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264089][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264103][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264116][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264130][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264143][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> [  254.264157][ T1197] ppc64_cpu (1197) used unsupported smt_snooze_delay, this has no effect
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  arch/powerpc/kernel/sysfs.c | 41 +++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 571b3259697e..ba6d4cee19ef 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -32,29 +32,26 @@
> 
>  static DEFINE_PER_CPU(struct cpu, cpu_devices);
> 
> -/*
> - * SMT snooze delay stuff, 64-bit only for now
> - */
> -
>  #ifdef CONFIG_PPC64
> 
> -/* Time in microseconds we delay before sleeping in the idle loop */
> -static DEFINE_PER_CPU(long, smt_snooze_delay) = { 100 };
> +/*
> + * Snooze delay has not been hooked up since 3fa8cad82b94 ("powerpc/pseries/cpuidle:
> + * smt-snooze-delay cleanup.") and has been broken even longer. As was foretold in
> + * 2014:
> + *
> + *  "ppc64_util currently utilises it. Once we fix ppc64_util, propose to clean
> + *  up the kernel code."
> + *
> + * At some point in the future this code should be removed.
> + */
> 
>  static ssize_t store_smt_snooze_delay(struct device *dev,
>  				      struct device_attribute *attr,
>  				      const char *buf,
>  				      size_t count)
>  {
> -	struct cpu *cpu = container_of(dev, struct cpu, dev);
> -	ssize_t ret;
> -	long snooze;
> -
> -	ret = sscanf(buf, "%ld", &snooze);
> -	if (ret != 1)
> -		return -EINVAL;
> -
> -	per_cpu(smt_snooze_delay, cpu->dev.id) = snooze;
> +	pr_warn_ratelimited("%s (%d) used unsupported smt_snooze_delay, this has no effect\n",
> +			    current->comm, current->pid);
>  	return count;
>  }
> 
> @@ -62,9 +59,9 @@ static ssize_t show_smt_snooze_delay(struct device *dev,
>  				     struct device_attribute *attr,
>  				     char *buf)
>  {
> -	struct cpu *cpu = container_of(dev, struct cpu, dev);
> -
> -	return sprintf(buf, "%ld\n", per_cpu(smt_snooze_delay, cpu->dev.id));
> +	pr_warn_ratelimited("%s (%d) used unsupported smt_snooze_delay, this has no effect\n",
> +			    current->comm, current->pid);
> +	return sprintf(buf, "100\n");
>  }
> 
>  static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,
> @@ -72,16 +69,10 @@ static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,
> 
>  static int __init setup_smt_snooze_delay(char *str)
>  {
> -	unsigned int cpu;
> -	long snooze;
> -
>  	if (!cpu_has_feature(CPU_FTR_SMT))
>  		return 1;
> 
> -	snooze = simple_strtol(str, NULL, 10);
> -	for_each_possible_cpu(cpu)
> -		per_cpu(smt_snooze_delay, cpu) = snooze;
> -
> +	pr_warn("smt-snooze-delay command line option has no effect\n");
>  	return 1;
>  }
>  __setup("smt-snooze-delay=", setup_smt_snooze_delay);
> -- 
> 2.27.0
>