Message ID | 20230331153905.31698-2-ldufour@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp663626vqo; Fri, 31 Mar 2023 08:56:13 -0700 (PDT) X-Google-Smtp-Source: AKy350ZlMwJfOKduESsJCTmISuIOGTM2RE149MCBYwzYVlp56HZHNeBRkGzVP/Rfk5DMn23EP5kk X-Received: by 2002:a17:907:c01a:b0:932:3d1b:b69d with SMTP id ss26-20020a170907c01a00b009323d1bb69dmr29598212ejc.47.1680278172891; Fri, 31 Mar 2023 08:56:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1680278172; cv=none; d=google.com; s=arc-20160816; b=KofYT9neEg1DFfRWp+EB1AgBeyc37CTcrq60zVNEWY8Ge/WY99CljdZnPUU45LCnGZ sYw64sLEbDm3t0sRDcpjbCdqVnq9PfT9gKpZL+DbpwCuctbdisQEoAR8Yuga0Pnlmd9K MxhYycfrAZ9Ixy8dTq3NWW/lnPAFx76fPWeSUa3xSaV3WmktxPYAIA7pzEExDMIhQ5de i9a/CDbViAIMKZUJYF14OkiYsH7Hw0d0IMdpNeRs7e11FhFkRNYXToZiBi9jrl6TrUDN BzTLkn9rhKCyOFw9ee/EPoilG5Ymnl22oD47OBGP32BVMJsOeuFiapvk8z2bBQ21zDj2 8+qg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=QYQpr9VmLdqpeVZbTAb42z+J0T6n0Z4Ey3Cl/9VQzic=; b=PH4x3zF/qDkZM3GRjZ67xAD+ozDzziiVPmb6kAn6z54vxBcGlanw79zMKSAYZmHNA9 nACaabEIXb9cItsngJcO8PwCfPSNiSZCdRNv2uVp/FG2wrNSl4GwxcxV7GnoZPfeJYyA ksOcTsgq6B+0mwGjhbZ7M9J85Swo2vlGzbhf9MTN0/nIuok7ZSpT6xCoqpAyUWx16yNH dSrsiWtsYGnNUqYcfWSp88RBK9x/3TwFDEqWTaWyO6BScvs7GmYiYVWgVTdiR4J5OIY0 JpRNZCzCkmxXI0LCHnf0SXQunhf7ItddPgyOTaMBrcbiEHcK8kaWBC58S7uxz7l/nwTm 0x7A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ibm.com header.s=pp1 header.b=h88zBcpY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=NONE dis=NONE) header.from=ibm.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id a23-20020a1709063e9700b008d78cb027b7si1775894ejj.667.2023.03.31.08.55.48; Fri, 31 Mar 2023 08:56:12 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@ibm.com header.s=pp1 header.b=h88zBcpY; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=NONE dis=NONE) header.from=ibm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232721AbjCaPjj (ORCPT <rfc822;dexuan.linux@gmail.com> + 99 others); Fri, 31 Mar 2023 11:39:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59332 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232735AbjCaPjh (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 31 Mar 2023 11:39:37 -0400 Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0D4391EFD1 for <linux-kernel@vger.kernel.org>; Fri, 31 Mar 2023 08:39:35 -0700 (PDT) Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 32VF9aWQ022595; Fri, 31 Mar 2023 15:39:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-transfer-encoding; s=pp1; bh=QYQpr9VmLdqpeVZbTAb42z+J0T6n0Z4Ey3Cl/9VQzic=; b=h88zBcpYxQZTuhlgnAdjVBHTOucunkwWXTKwCg8Fgh8RIEB2r9/cCyto6tLXMkrTfOvh JiIgXH6C9XktuQaQPhNJvEeSM2BwfjSI+XPJaIdbXwjS+wpVetOrS4Qks+XjW47yBSfY JSRqzbQJ6UPY1Z9YNCYTywkwY6YX9r9lUHjVs0Hz5U2sxF+sxOi5+I/qoj9XvfSl3L3m m0h9omnXutm344pLqa9Gx4BmLVDv1HMJYvI/4r0jtzwHnXT0aioPincFRgAyYClqlWQr WVuy6DD71H0/1dhJ3PYEmrT/Ba8ZPSd7d7x37tcoeYTpnUUuIz7KennAwfhBGdAA0qE/ Eg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3pp16d20rr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 31 Mar 2023 15:39:23 +0000 Received: from m0098421.ppops.net (m0098421.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 32VF9db8023005; Fri, 31 Mar 2023 15:39:23 GMT Received: from ppma06ams.nl.ibm.com (66.31.33a9.ip4.static.sl-reverse.com [169.51.49.102]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3pp16d20qy-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 31 Mar 2023 15:39:22 +0000 Received: from pps.filterd (ppma06ams.nl.ibm.com [127.0.0.1]) by ppma06ams.nl.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 32V1I3gi013053; Fri, 31 Mar 2023 15:39:21 GMT Received: from smtprelay03.fra02v.mail.ibm.com ([9.218.2.224]) by ppma06ams.nl.ibm.com (PPS) with ESMTPS id 3phr7fpvyg-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 31 Mar 2023 15:39:21 +0000 Received: from smtpav01.fra02v.mail.ibm.com (smtpav01.fra02v.mail.ibm.com [10.20.54.100]) by smtprelay03.fra02v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 32VFdH8H64291314 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 31 Mar 2023 15:39:17 GMT Received: from smtpav01.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id AA3AE20043; Fri, 31 Mar 2023 15:39:17 +0000 (GMT) Received: from smtpav01.fra02v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E846120040; Fri, 31 Mar 2023 15:39:16 +0000 (GMT) Received: from localhost.localdomain (unknown [9.179.0.144]) by smtpav01.fra02v.mail.ibm.com (Postfix) with ESMTP; Fri, 31 Mar 2023 15:39:16 +0000 (GMT) From: Laurent Dufour <ldufour@linux.ibm.com> To: mpe@ellerman.id.au, npiggin@gmail.com, christophe.leroy@csgroup.eu Cc: msuchanek@suse.de, nathanl@linux.ibm.com, linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Srikar Dronamraju <srikar@linux.vnet.ibm.com> Subject: [PATCH 1/2] pseries/smp: export the smt level in the SYS FS. Date: Fri, 31 Mar 2023 17:39:04 +0200 Message-Id: <20230331153905.31698-2-ldufour@linux.ibm.com> X-Mailer: git-send-email 2.40.0 In-Reply-To: <20230331153905.31698-1-ldufour@linux.ibm.com> References: <20230331153905.31698-1-ldufour@linux.ibm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: PGc9QIzO_ZHUuoWKf1gThmRJW9B1TMUD X-Proofpoint-GUID: 3nWUCXfU9EBO2gnYWIsNv4wvaUa9ZIOj X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-03-31_07,2023-03-31_01,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1011 mlxlogscore=999 malwarescore=0 lowpriorityscore=0 suspectscore=0 spamscore=0 impostorscore=0 phishscore=0 priorityscore=1501 mlxscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000 definitions=main-2303310123 X-Spam-Status: No, score=-0.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_EF,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761899365577655326?= X-GMAIL-MSGID: =?utf-8?q?1761899365577655326?= |
Series |
Online new threads according to the current SMT level
|
|
Commit Message
Laurent Dufour
March 31, 2023, 3:39 p.m. UTC
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(+)
Comments
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. 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 >
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 >>
Hi Laurent, Laurent Dufour <ldufour@linux.ibm.com> writes: > 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(+) There is a generic sysfs interface for smt in /sys/devices/system/cpu/smt I think we should be enabling that on powerpc and then adapting it to our needs, rather than adding a pseries specific file. Currently the generic code is only aware of SMT on/off, so it would need to be taught about SMT4 and 8 at least. There are already hooks in the generic code to check the SMT level when bringing CPUs up, see cpu_smt_allowed(), they may work for the pseries hotplug case too, though maybe we need some additional logic. Wiring up the basic support is pretty straight forward, something like the diff below. cheers diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 0f123f1f62a1..a48576f1c579 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -260,6 +260,7 @@ config PPC select HAVE_SYSCALL_TRACEPOINTS select HAVE_VIRT_CPU_ACCOUNTING select HAVE_VIRT_CPU_ACCOUNTING_GEN + select HOTPLUG_SMT if HOTPLUG_CPU select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE select IOMMU_HELPER if PPC64 select IRQ_DOMAIN diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h index 8a4d4f4d9749..bd23ba716d23 100644 --- a/arch/powerpc/include/asm/topology.h +++ b/arch/powerpc/include/asm/topology.h @@ -143,5 +143,8 @@ static inline int cpu_to_coregroup_id(int cpu) #endif #endif +bool topology_is_primary_thread(unsigned int cpu); +bool topology_smt_supported(void); + #endif /* __KERNEL__ */ #endif /* _ASM_POWERPC_TOPOLOGY_H */ diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c index 265801a3e94c..8619609809d5 100644 --- a/arch/powerpc/kernel/smp.c +++ b/arch/powerpc/kernel/smp.c @@ -1769,4 +1769,20 @@ void __noreturn arch_cpu_idle_dead(void) start_secondary_resume(); } +/** + * topology_is_primary_thread - Check whether CPU is the primary SMT thread + * @cpu: CPU to check + */ +bool topology_is_primary_thread(unsigned int cpu) +{ + return cpu == cpu_first_thread_sibling(cpu); +} + +/** + * topology_smt_supported - Check whether SMT is supported by the CPUs + */ +bool topology_smt_supported(void) +{ + return threads_per_core > 1; +} #endif
On 13/04/2023 15:37:59, Michael Ellerman wrote: > Hi Laurent, > > Laurent Dufour <ldufour@linux.ibm.com> writes: >> 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(+) > > There is a generic sysfs interface for smt in /sys/devices/system/cpu/smt > > I think we should be enabling that on powerpc and then adapting it to > our needs, rather than adding a pseries specific file. Thanks Michael, I was not aware of this sysfs interface. > Currently the generic code is only aware of SMT on/off, so it would need > to be taught about SMT4 and 8 at least. Do you think we should limit our support to SMT4 and SMT8 only? > There are already hooks in the generic code to check the SMT level when > bringing CPUs up, see cpu_smt_allowed(), they may work for the pseries > hotplug case too, though maybe we need some additional logic. > > Wiring up the basic support is pretty straight forward, something like > the diff below. I'll look into how to wire this up. Thanks a lot! > cheers > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 0f123f1f62a1..a48576f1c579 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -260,6 +260,7 @@ config PPC > select HAVE_SYSCALL_TRACEPOINTS > select HAVE_VIRT_CPU_ACCOUNTING > select HAVE_VIRT_CPU_ACCOUNTING_GEN > + select HOTPLUG_SMT if HOTPLUG_CPU > select HUGETLB_PAGE_SIZE_VARIABLE if PPC_BOOK3S_64 && HUGETLB_PAGE > select IOMMU_HELPER if PPC64 > select IRQ_DOMAIN > diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h > index 8a4d4f4d9749..bd23ba716d23 100644 > --- a/arch/powerpc/include/asm/topology.h > +++ b/arch/powerpc/include/asm/topology.h > @@ -143,5 +143,8 @@ static inline int cpu_to_coregroup_id(int cpu) > #endif > #endif > > +bool topology_is_primary_thread(unsigned int cpu); > +bool topology_smt_supported(void); > + > #endif /* __KERNEL__ */ > #endif /* _ASM_POWERPC_TOPOLOGY_H */ > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c > index 265801a3e94c..8619609809d5 100644 > --- a/arch/powerpc/kernel/smp.c > +++ b/arch/powerpc/kernel/smp.c > @@ -1769,4 +1769,20 @@ void __noreturn arch_cpu_idle_dead(void) > start_secondary_resume(); > } > > +/** > + * topology_is_primary_thread - Check whether CPU is the primary SMT thread > + * @cpu: CPU to check > + */ > +bool topology_is_primary_thread(unsigned int cpu) > +{ > + return cpu == cpu_first_thread_sibling(cpu); > +} > + > +/** > + * topology_smt_supported - Check whether SMT is supported by the CPUs > + */ > +bool topology_smt_supported(void) > +{ > + return threads_per_core > 1; > +} > #endif
Laurent Dufour <ldufour@linux.ibm.com> writes: > On 13/04/2023 15:37:59, Michael Ellerman wrote: >> Laurent Dufour <ldufour@linux.ibm.com> writes: >>> 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(+) >> >> There is a generic sysfs interface for smt in /sys/devices/system/cpu/smt >> >> I think we should be enabling that on powerpc and then adapting it to >> our needs, rather than adding a pseries specific file. > > Thanks Michael, I was not aware of this sysfs interface. > >> Currently the generic code is only aware of SMT on/off, so it would need >> to be taught about SMT4 and 8 at least. > > Do you think we should limit our support to SMT4 and SMT8 only? Possibly? Currently the SMT state is represented by an enum: enum cpuhp_smt_control { CPU_SMT_ENABLED, CPU_SMT_DISABLED, CPU_SMT_FORCE_DISABLED, CPU_SMT_NOT_SUPPORTED, CPU_SMT_NOT_IMPLEMENTED, }; Adding two states for SMT4 and SMT8 seeems like it might be acceptable. On the other hand if we want to support artbitrary SMT values from 3 to 8 then it might be better to store that value separately from the state enum. TBH I'm not sure whether we want to support values other than 1/2/4/8 via this interface. A user who wants some odd numbered SMT value can always configure that manually using the existing tools. But maybe it's less confusing if this interface supports all values? Even if they're unlikely to get much usage. >> There are already hooks in the generic code to check the SMT level when >> bringing CPUs up, see cpu_smt_allowed(), they may work for the pseries >> hotplug case too, though maybe we need some additional logic. >> >> Wiring up the basic support is pretty straight forward, something like >> the diff below. > > I'll look into how to wire this up. > Thanks a lot! Thanks. cheers
Hello, On Fri, Apr 14, 2023 at 10:11:24PM +1000, Michael Ellerman wrote: > Laurent Dufour <ldufour@linux.ibm.com> writes: > > On 13/04/2023 15:37:59, Michael Ellerman wrote: > >> Laurent Dufour <ldufour@linux.ibm.com> writes: > >>> 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(+) > >> > >> There is a generic sysfs interface for smt in /sys/devices/system/cpu/smt > >> > >> I think we should be enabling that on powerpc and then adapting it to > >> our needs, rather than adding a pseries specific file. > > > > Thanks Michael, I was not aware of this sysfs interface. > > > >> Currently the generic code is only aware of SMT on/off, so it would need > >> to be taught about SMT4 and 8 at least. > > > > Do you think we should limit our support to SMT4 and SMT8 only? > > Possibly? Currently the SMT state is represented by an enum: > > enum cpuhp_smt_control { > CPU_SMT_ENABLED, > CPU_SMT_DISABLED, > CPU_SMT_FORCE_DISABLED, > CPU_SMT_NOT_SUPPORTED, > CPU_SMT_NOT_IMPLEMENTED, > }; > > Adding two states for SMT4 and SMT8 seeems like it might be acceptable. > > On the other hand if we want to support artbitrary SMT values from 3 to > 8 then it might be better to store that value separately from the state > enum. > > TBH I'm not sure whether we want to support values other than 1/2/4/8 > via this interface. > > A user who wants some odd numbered SMT value can always configure that > manually using the existing tools. > > But maybe it's less confusing if this interface supports all values? > Even if they're unlikely to get much usage. It looks like ppc64_cpu simply enables first n threads of the CPU where n is the smt value without any interleaving hoping that the architecture does the right thing. Under this implementation smt=3 is well-defined. For the dual cluster P9 CPUs that have two clusters of four this might work out well for some workloads, and others might want that interleaving. With that the odd smt values are not well-definedd anymore. Nonetheless, if the kernel does support some smt=n parameter whatever the semantic this should be also supported by the runtime knob. If it's too difficult to get right there is always that option to not enable any thread by default, and let the userspace to implement arbitrarily complex schemes :) Thanks Michal
* Laurent Dufour <ldufour@linux.ibm.com> [2023-04-13 17:38:51]: > On 13/04/2023 15:37:59, Michael Ellerman wrote: > > Hi Laurent, > > > > Laurent Dufour <ldufour@linux.ibm.com> writes: > >> 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(+) > > > > There is a generic sysfs interface for smt in /sys/devices/system/cpu/smt > > > > I think we should be enabling that on powerpc and then adapting it to > > our needs, rather than adding a pseries specific file. > > Thanks Michael, I was not aware of this sysfs interface. > > > Currently the generic code is only aware of SMT on/off, so it would need > > to be taught about SMT4 and 8 at least. > > Do you think we should limit our support to SMT4 and SMT8 only? smt2 is also a valid already supported configuration and we are evaluating smt6 mode based on some inputs from ISV teams. So I believe having a value for all modes would be good. > > > There are already hooks in the generic code to check the SMT level when > > bringing CPUs up, see cpu_smt_allowed(), they may work for the pseries > > hotplug case too, though maybe we need some additional logic. > > > > Wiring up the basic support is pretty straight forward, something like > > the diff below. >
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 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);