Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6417d5ef-1fb6-7d6b-689d-39699b50af4e@linux.ibm.com>
Date:   Mon, 3 Apr 2023 10:20:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.1
Subject: Re: [PATCH 1/2] pseries/smp: export the smt level in the SYS FS.
To:     =?UTF-8?Q?Michal_Such=c3=a1nek?= <msuchanek@suse.de>,
        mpe@ellerman.id.au
Cc:     npiggin@gmail.com, christophe.leroy@csgroup.eu,
        nathanl@linux.ibm.com, linux-kernel@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org,
        Srikar Dronamraju <srikar@linux.vnet.ibm.com>
References: <20230331153905.31698-1-ldufour@linux.ibm.com>
 <20230331153905.31698-2-ldufour@linux.ibm.com>
 <20230331160527.GA3132@kitsune.suse.cz>
Content-Language: en-US
From:   Laurent Dufour <ldufour@linux.ibm.com>
In-Reply-To: <20230331160527.GA3132@kitsune.suse.cz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 31/03/2023 18:05:27, Michal Suchánek wrote:
> 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@linux.vnet.ibm.com>
>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>> ---
>>  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.
> 

Hi Michal,

I do agree, the pseries code is implying SMP.

When writing that code, I found that SMP conditional block and just add
this define to be sure the code will compile in the case SMP is not
defined, but that's probably useless.

Instead of resending a new series, Michael, could you please remove that
line when applying the patch to your tree?

Thanks,
Laurent.

> 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
>>