Re: [PATCH 1/2] pseries/smp: export the smt level in the SYS FS.

From: Michal Suchánek
Date: Fri Mar 31 2023 - 12:05:37 EST


Hello,

On Fri, Mar 31, 2023 at 05:39:04PM +0200, Laurent Dufour wrote:
> There is no SMT level recorded in the kernel neither in user space.
> Indeed there is no real constraint about that and mixed SMT levels are
> allowed and system is working fine this way.
>
> However when new CPU are added, the kernel is onlining all the threads
> which is leading to mixed SMT levels and confuse end user a bit.
>
> To prevent this exports a SMT level from the kernel so user space
> application like the energy daemon, could read it to adjust their settings.
> There is no action unless recording the value when a SMT value is written
> into the new sysfs entry. User space applications like ppc64_cpu should
> update the sysfs when changing the SMT level to keep the system consistent.
>
> Suggested-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxx>
> ---
> arch/powerpc/platforms/pseries/pseries.h | 3 ++
> arch/powerpc/platforms/pseries/smp.c | 39 ++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index f8bce40ebd0c..af0a145af98f 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -23,7 +23,9 @@ extern int pSeries_machine_check_exception(struct pt_regs *regs);
> extern long pseries_machine_check_realmode(struct pt_regs *regs);
> void pSeries_machine_check_log_err(void);
>
> +
> #ifdef CONFIG_SMP
> +extern int pseries_smt;
> extern void smp_init_pseries(void);
>
> /* Get state of physical CPU from query_cpu_stopped */
> @@ -34,6 +36,7 @@ int smp_query_cpu_stopped(unsigned int pcpu);
> #define QCSS_HARDWARE_ERROR -1
> #define QCSS_HARDWARE_BUSY -2
> #else
> +#define pseries_smt 1

Is this really needed for anything?

The code using pseries_smt would not compile with a define, and would be
only compiled with SMP enabled anyway so we should not need this.

Thanks

Michal

> static inline void smp_init_pseries(void) { }
> #endif
>
> diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
> index c597711ef20a..6c382922f8f3 100644
> --- a/arch/powerpc/platforms/pseries/smp.c
> +++ b/arch/powerpc/platforms/pseries/smp.c
> @@ -21,6 +21,7 @@
> #include <linux/device.h>
> #include <linux/cpu.h>
> #include <linux/pgtable.h>
> +#include <linux/sysfs.h>
>
> #include <asm/ptrace.h>
> #include <linux/atomic.h>
> @@ -45,6 +46,8 @@
>
> #include "pseries.h"
>
> +int pseries_smt;
> +
> /*
> * The Primary thread of each non-boot processor was started from the OF client
> * interface by prom_hold_cpus and is spinning on secondary_hold_spinloop.
> @@ -280,3 +283,39 @@ void __init smp_init_pseries(void)
>
> pr_debug(" <- smp_init_pSeries()\n");
> }
> +
> +static ssize_t pseries_smt_store(struct class *class,
> + struct class_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int smt;
> +
> + if (kstrtou32(buf, 0, &smt) || !smt || smt > (u32) threads_per_core) {
> + pr_err("Invalid pseries_smt specified.\n");
> + return -EINVAL;
> + }
> +
> + pseries_smt = smt;
> +
> + return count;
> +}
> +
> +static ssize_t pseries_smt_show(struct class *class, struct class_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "%d\n", pseries_smt);
> +}
> +
> +static CLASS_ATTR_RW(pseries_smt);
> +
> +static int __init pseries_smt_init(void)
> +{
> + int rc;
> +
> + pseries_smt = smt_enabled_at_boot;
> + rc = sysfs_create_file(kernel_kobj, &class_attr_pseries_smt.attr);
> + if (rc)
> + pr_err("Can't create pseries_smt sysfs/kernel entry.\n");
> + return rc;
> +}
> +machine_device_initcall(pseries, pseries_smt_init);
> --
> 2.40.0
>