| Message ID | 20200630015935.2675676-1-joel@jms.id.au (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] powerpc: Warn about use of smt_snooze_delay | expand |
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 >
Joel Stanley <joel@jms.id.au> writes: > 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> > -- > 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. Hmm, that is pretty spammy. I know I suggested the ratelimit, but I was thinking it would print once for each ppc64_cpu invocation, not ~30 times. How about pr_warn_once(), that should still be sufficient for people to notice if they're looking for it. ... > 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); Can we make this: "%s (%d) stored to unsupported smt_snooze_delay, which has no effect.\n", > 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); It has as much effect as it ever did :) So maybe: "%s (%d) read from unsupported smt_snooze_delay.\n", I can do those changes when applying if you like rather than making you do a v3. cheers
On Tue, 4 Aug 2020 at 11:59, Michael Ellerman <mpe@ellerman.id.au> wrote: > > Joel Stanley <joel@jms.id.au> writes: > > 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> > > -- > > 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. > > Hmm, that is pretty spammy. > > I know I suggested the ratelimit, but I was thinking it would print once > for each ppc64_cpu invocation, not ~30 times. > > How about pr_warn_once(), that should still be sufficient for people to > notice if they're looking for it. I think that's a reasonable suggestion. > > ... > > 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); > > Can we make this: > > "%s (%d) stored to unsupported smt_snooze_delay, which has no effect.\n", ack > > > > 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); > > It has as much effect as it ever did :) > > So maybe: > > "%s (%d) read from unsupported smt_snooze_delay.\n", > > > I can do those changes when applying if you like rather than making you > do a v3. Yes please! Your suggested changes lgtm. Cheers, Joel
On Tue, 30 Jun 2020 11:29:35 +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> > -- > 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 > > [...] Applied to powerpc/next. [1/1] powerpc: Warn about use of smt_snooze_delay https://git.kernel.org/powerpc/c/a02f6d42357acf6e5de6ffc728e6e77faf3ad217 cheers
Michael Ellerman <patch-notifications@ellerman.id.au> writes: > On Tue, 30 Jun 2020 11:29:35 +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> >> -- >> 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 >> >> [...] > > Applied to powerpc/next. > > [1/1] powerpc: Warn about use of smt_snooze_delay > https://git.kernel.org/powerpc/c/a02f6d42357acf6e5de6ffc728e6e77faf3ad217 I applied v3 actually. cheers
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);
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> -- 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(-)