On Tue, 4 Aug 2020 at 11:59, Michael Ellerman <m...@ellerman.id.au> wrote: > > Joel Stanley <j...@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: sta...@vger.kernel.org # v3.14 > > Signed-off-by: Joel Stanley <j...@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