[PATCH] powerpc: Warn about use of 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.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/powerpc/kernel/sysfs.c | 41 +++++++++++++------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 571b3259697e..530ae92bc46d 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c

@@ -32,29 +32,25 @@
 
 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;
+	WARN_ON_ONCE("smt_snooze_delay sysfs file has no effect\n");
 	return count;
 }
 

@@ -62,9 +58,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);
+	WARN_ON_ONCE("smt_snooze_delay sysfs file has no effect\n");
 
-	return sprintf(buf, "%ld\n", per_cpu(smt_snooze_delay, cpu->dev.id));
+	return sprintf(buf, "100\n");
 }
 
 static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,

@@ -72,16 +68,7 @@ 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;
-
+	WARN_ON_ONCE("smt-snooze-delay command line option has no effect\n");
 	return 1;
 }
 __setup("smt-snooze-delay=", setup_smt_snooze_delay);

-- 
2.27.0

Le 25/06/2020 à 12:03, Joel Stanley a écrit :

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.

Why not just drop the file entirely  if it is useless ?

Christophe

On 6/25/20 3:29 AM, Christophe Leroy wrote:

Le 25/06/2020 à 12:03, Joel Stanley a écrit :

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.

Why not just drop the file entirely  if it is useless ?

Userspace tooling that hasn't learned its useless yet. Joel has also submitted a
pull request for the ppc64_util tool in question to drop using this interface.

-Tyrel

Christophe

quoted

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
  arch/powerpc/kernel/sysfs.c | 41 +++++++++++++------------------------
  1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 571b3259697e..530ae92bc46d 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c

@@ -32,29 +32,25 @@

    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;
+    WARN_ON_ONCE("smt_snooze_delay sysfs file has no effect\n");
      return count;
  }
  @@ -62,9 +58,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);
+    WARN_ON_ONCE("smt_snooze_delay sysfs file has no effect\n");
  -    return sprintf(buf, "%ld\n", per_cpu(smt_snooze_delay, cpu->dev.id));
+    return sprintf(buf, "100\n");
  }
    static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,

@@ -72,16 +68,7 @@ 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;
-
+    WARN_ON_ONCE("smt-snooze-delay command line option has no effect\n");
      return 1;
  }
  __setup("smt-snooze-delay=", setup_smt_snooze_delay);

On Thu, Jun 25, 2020 at 07:33:49PM +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.

Signed-off-by: Joel Stanley <joel@jms.id.au>

The only known user of "smt_snooze_delay" is the "ppc64_cpu" which
uses the presence of this file to assume that the system is SMT
capable.

Since we have "/sys/devices/system/cpu/smt/" these days, perhaps the
userspace utility can use that and we can get rid of the file
altogether ?

FWIW,
Acked-by: Gautham R. Shenoy <redacted>

On Mon, 29 Jun 2020 at 10:42, Gautham R Shenoy [off-list ref] wrote:

On Thu, Jun 25, 2020 at 07:33:49PM +0930, Joel Stanley wrote:

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.

Signed-off-by: Joel Stanley <joel@jms.id.au>

The only known user of "smt_snooze_delay" is the "ppc64_cpu" which
uses the presence of this file to assume that the system is SMT
capable.

Since we have "/sys/devices/system/cpu/smt/" these days, perhaps the
userspace utility can use that and we can get rid of the file
altogether ?

I've sent a change to the userspace tool to stop using the file. It
now uses the device tree parsing that was already present to determine
the smt state.

 https://github.com/ibm-power-utilities/powerpc-utils/pull/43

We will want to wait for the userspace tool to propagate through a
release and to distros before we remove the file all together. I agree
it should be removed in the future.

I've got of this patch v2 that changes the message to be:

         pr_warn_ratelimited("%s (%d) used unsupported
smt_snooze_delay, this has no effect\n",
                            current->comm, current->pid);

I'll send that out today.

Cheers,

Joel

FWIW,
Acked-by: Gautham R. Shenoy <redacted>

quoted

---
 arch/powerpc/kernel/sysfs.c | 41 +++++++++++++------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 571b3259697e..530ae92bc46d 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c

@@ -32,29 +32,25 @@

 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;
+     WARN_ON_ONCE("smt_snooze_delay sysfs file has no effect\n");
      return count;
 }

@@ -62,9 +58,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);
+     WARN_ON_ONCE("smt_snooze_delay sysfs file has no effect\n");

-     return sprintf(buf, "%ld\n", per_cpu(smt_snooze_delay, cpu->dev.id));
+     return sprintf(buf, "100\n");
 }

 static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,

@@ -72,16 +68,7 @@ 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;
-
+     WARN_ON_ONCE("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 [off-list ref] writes:

We shouldn't have user-triggerable WARNs.

I think this should just be a pr_warn_ratelimited(), maybe including
current->comm & pid.

cheers