From: "Michal Suchánek" <msuchanek@suse.de>
To: Laurent Dufour <ldufour@linux.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org, linux-arch@vger.kernel.org,
kernel test robot <lkp@intel.com>,
dave.hansen@linux.intel.com, linux-kernel@vger.kernel.org,
mingo@redhat.com, bp@alien8.de, npiggin@gmail.com,
tglx@linutronix.de, rui.zhang@intel.com
Subject: Re: [PATCH v4 07/10] cpu/SMT: Allow enabling partial SMT states via sysfs
Date: Mon, 8 Apr 2024 16:13:41 +0200 [thread overview]
Message-ID: <20240408141341.GN20665@kitsune.suse.cz> (raw)
In-Reply-To: <20230705145143.40545-8-ldufour@linux.ibm.com>
Hello,
On Wed, Jul 05, 2023 at 04:51:40PM +0200, Laurent Dufour wrote:
> From: Michael Ellerman <mpe@ellerman.id.au>
>
> Add support to the /sys/devices/system/cpu/smt/control interface for
> enabling a specified number of SMT threads per core, including partial
> SMT states where not all threads are brought online.
>
> The current interface accepts "on" and "off", to enable either 1 or all
> SMT threads per core.
>
> This commit allows writing an integer, between 1 and the number of SMT
> threads supported by the machine. Writing 1 is a synonym for "off", 2 or
> more enables SMT with the specified number of threads.
>
> When reading the file, if all threads are online "on" is returned, to
> avoid changing behaviour for existing users. If some other number of
> threads is online then the integer value is returned.
>
> Architectures like x86 only supporting 1 thread or all threads, should not
> define CONFIG_SMT_NUM_THREADS_DYNAMIC. Architecture supporting partial SMT
> states, like PowerPC, should define it.
This causes a regression:
https://groups.google.com/g/powerpc-utils-devel/c/wrwVzAAnRlI/m/5KJSoqP4BAAJ
The userspace code only changes the SMT mode of online CPUs (at lest one
thread is online) and does not touch offline CPUs.
Using this new interface offlined CPUs are onlined when SMT value is
set.
Would you look into special-casing the offline CPUs?
Thanks
Michal
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> [ldufour: slightly reword the commit's description]
> [ldufour: remove switch() in __store_smt_control()]
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202306282340.Ihqm0fLA-lkp@intel.com/
> [ldufour: fix build issue in control_show()]
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> .../ABI/testing/sysfs-devices-system-cpu | 1 +
> kernel/cpu.c | 60 ++++++++++++++-----
> 2 files changed, 45 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index ecd585ca2d50..6dba65fb1956 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -555,6 +555,7 @@ Description: Control Symmetric Multi Threading (SMT)
> ================ =========================================
> "on" SMT is enabled
> "off" SMT is disabled
> + "<N>" SMT is enabled with N threads per core.
> "forceoff" SMT is force disabled. Cannot be changed.
> "notsupported" SMT is not supported by the CPU
> "notimplemented" SMT runtime toggling is not
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 9a8d0685e055..7e8f1b044772 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2876,11 +2876,19 @@ static const struct attribute_group cpuhp_cpu_root_attr_group = {
>
> #ifdef CONFIG_HOTPLUG_SMT
>
> +static bool cpu_smt_num_threads_valid(unsigned int threads)
> +{
> + if (IS_ENABLED(CONFIG_SMT_NUM_THREADS_DYNAMIC))
> + return threads >= 1 && threads <= cpu_smt_max_threads;
> + return threads == 1 || threads == cpu_smt_max_threads;
> +}
> +
> static ssize_t
> __store_smt_control(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int ctrlval, ret;
> + int ctrlval, ret, num_threads, orig_threads;
> + bool force_off;
>
> if (cpu_smt_control == CPU_SMT_FORCE_DISABLED)
> return -EPERM;
> @@ -2888,30 +2896,39 @@ __store_smt_control(struct device *dev, struct device_attribute *attr,
> if (cpu_smt_control == CPU_SMT_NOT_SUPPORTED)
> return -ENODEV;
>
> - if (sysfs_streq(buf, "on"))
> + if (sysfs_streq(buf, "on")) {
> ctrlval = CPU_SMT_ENABLED;
> - else if (sysfs_streq(buf, "off"))
> + num_threads = cpu_smt_max_threads;
> + } else if (sysfs_streq(buf, "off")) {
> ctrlval = CPU_SMT_DISABLED;
> - else if (sysfs_streq(buf, "forceoff"))
> + num_threads = 1;
> + } else if (sysfs_streq(buf, "forceoff")) {
> ctrlval = CPU_SMT_FORCE_DISABLED;
> - else
> + num_threads = 1;
> + } else if (kstrtoint(buf, 10, &num_threads) == 0) {
> + if (num_threads == 1)
> + ctrlval = CPU_SMT_DISABLED;
> + else if (cpu_smt_num_threads_valid(num_threads))
> + ctrlval = CPU_SMT_ENABLED;
> + else
> + return -EINVAL;
> + } else {
> return -EINVAL;
> + }
>
> ret = lock_device_hotplug_sysfs();
> if (ret)
> return ret;
>
> - if (ctrlval != cpu_smt_control) {
> - switch (ctrlval) {
> - case CPU_SMT_ENABLED:
> - ret = cpuhp_smt_enable();
> - break;
> - case CPU_SMT_DISABLED:
> - case CPU_SMT_FORCE_DISABLED:
> - ret = cpuhp_smt_disable(ctrlval);
> - break;
> - }
> - }
> + orig_threads = cpu_smt_num_threads;
> + cpu_smt_num_threads = num_threads;
> +
> + force_off = ctrlval != cpu_smt_control && ctrlval == CPU_SMT_FORCE_DISABLED;
> +
> + if (num_threads > orig_threads)
> + ret = cpuhp_smt_enable();
> + else if (num_threads < orig_threads || force_off)
> + ret = cpuhp_smt_disable(ctrlval);
>
> unlock_device_hotplug();
> return ret ? ret : count;
> @@ -2939,6 +2956,17 @@ static ssize_t control_show(struct device *dev,
> {
> const char *state = smt_states[cpu_smt_control];
>
> +#ifdef CONFIG_HOTPLUG_SMT
> + /*
> + * If SMT is enabled but not all threads are enabled then show the
> + * number of threads. If all threads are enabled show "on". Otherwise
> + * show the state name.
> + */
> + if (cpu_smt_control == CPU_SMT_ENABLED &&
> + cpu_smt_num_threads != cpu_smt_max_threads)
> + return sysfs_emit(buf, "%d\n", cpu_smt_num_threads);
> +#endif
> +
> return snprintf(buf, PAGE_SIZE - 2, "%s\n", state);
> }
>
> --
> 2.41.0
>
next prev parent reply other threads:[~2024-04-08 14:13 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-05 14:51 [PATCH v4 00/10] Introduce SMT level and add PowerPC support Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 01/10] cpu/hotplug: remove dependancy against cpu_primary_thread_mask Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 02/10] cpu/SMT: Move SMT prototypes into cpu_smt.h Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 03/10] cpu/SMT: Move smt/control simple exit cases earlier Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 04/10] cpu/SMT: Store the current/max number of threads Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 05/10] cpu/SMT: Remove topology_smt_supported() Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 06/10] cpu/SMT: Create topology_smt_thread_allowed() Laurent Dufour
2023-07-05 14:51 ` [PATCH v4 07/10] cpu/SMT: Allow enabling partial SMT states via sysfs Laurent Dufour
2024-04-08 14:13 ` Michal Suchánek [this message]
2023-07-05 14:51 ` [PATCH v4 08/10] powerpc/pseries: Initialise CPU hotplug callbacks earlier Laurent Dufour
2023-08-14 10:51 ` Srikar Dronamraju
2023-07-05 14:51 ` [PATCH v4 09/10] powerpc: Add HOTPLUG_SMT support Laurent Dufour
2023-08-14 10:52 ` Srikar Dronamraju
2023-07-05 14:51 ` [PATCH v4 10/10] powerpc/pseries: Honour current SMT state when DLPAR onlining CPUs Laurent Dufour
2023-08-14 10:53 ` Srikar Dronamraju
2023-07-09 15:25 ` [PATCH v4 00/10] Introduce SMT level and add PowerPC support Zhang, Rui
2023-07-10 9:08 ` Laurent Dufour
2023-07-28 7:40 ` Thomas Gleixner
2023-07-28 14:23 ` Zhang, Rui
2023-07-28 14:51 ` Thomas Gleixner
2023-07-28 7:58 ` Thomas Gleixner
2023-07-31 11:55 ` Laurent Dufour
2023-08-10 6:23 ` Michael Ellerman
2023-08-10 8:51 ` Laurent Dufour
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240408141341.GN20665@kitsune.suse.cz \
--to=msuchanek@suse.de \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=ldufour@linux.ibm.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lkp@intel.com \
--cc=mingo@redhat.com \
--cc=npiggin@gmail.com \
--cc=rui.zhang@intel.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).