Re: [PATCH] powerpc: Warn about use of smt_snooze_delay
Joel Stanley 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.
>
> Signed-off-by: Joel Stanley
> ---
> 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", );
> - 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");
We shouldn't have user-triggerable WARNs.
I think this should just be a pr_warn_ratelimited(), maybe including
current->comm & pid.
cheers
Re: [PATCH] powerpc: Warn about use of smt_snooze_delay
On Mon, 29 Jun 2020 at 10:42, Gautham R Shenoy wrote:
>
> 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
>
> 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
> > ---
> > 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", );
> > - 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
> >
Re: [PATCH] powerpc: Warn about use of 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
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
> ---
> 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", );
> - 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
>
Re: [PATCH] powerpc: Warn about use of smt_snooze_delay
On 6/25/20 3:29 AM, Christophe Leroy wrote:
>
>
> 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 ?
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
>
>>
>> Signed-off-by: Joel Stanley
>> ---
>> 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", );
>> - 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);
>>
Re: [PATCH] powerpc: Warn about use of smt_snooze_delay
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
Signed-off-by: Joel Stanley
---
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", );
- 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);
[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
---
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", );
- 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
