On Tue, 4 Aug 2020 at 11:59, Michael Ellerman [off-list ref] wrote:

Joel Stanley [off-list ref] writes:

quoted

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.

...

quoted

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

quoted

      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