From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 11 Jun 2018 21:22:15 -0000 Received: from aserp2120.oracle.com ([141.146.126.78]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fSUGT-0003cF-QF for speck@linutronix.de; Mon, 11 Jun 2018 23:22:14 +0200 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w5BLKueK086365 for ; Mon, 11 Jun 2018 21:22:07 GMT Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by aserp2120.oracle.com with ESMTP id 2jgecxegm4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 11 Jun 2018 21:22:06 +0000 Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id w5BLM6oZ010562 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 11 Jun 2018 21:22:06 GMT Received: from abhmp0015.oracle.com (abhmp0015.oracle.com [141.146.116.21]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id w5BLM6Pd005492 for ; Mon, 11 Jun 2018 21:22:06 GMT Date: Mon, 11 Jun 2018 17:22:05 -0400 From: Konrad Rzeszutek Wilk Subject: [MODERATED] Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT Message-ID: <20180611212205.GE25607@char.us.oracle.com> References: <20180606192714.754943543@linutronix.de> <20180606192807.337089954@linutronix.de> MIME-Version: 1.0 In-Reply-To: <20180606192807.337089954@linutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Wed, Jun 06, 2018 at 09:27:19PM +0200, speck for Thomas Gleixner wrote: > Subject: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT > From: Thomas Gleixner > > Proide a command line and a sysfs knob to control SMT. s/Proide/Provide/ > > Switching SMT control off, offlines all online CPUs which are secondary > threads of a physical core and prevents onlining of secondary threads after > that point. > > Switching it back on lifts the restrictions again, but does not online the I am not sure what you mean by 'Switching it back on lifts the restrictions again' as you said in earlier 'prevents onlining of secondary'. So that sounds like you can't switch the CPU back on as the restriction is in place? Oh you mean offlining a _core_ CPU and then onlining it! Perhaps "When offlining and onlining a core CPU the restrictions are still in effect - that is you can't online secondary siblings' ? [But then I am looking at the patch and you do allow it?!] > offlined secondary siblings. > > It also can be set to force off, which is an irreversible operation and s/force off/force off (aka, never allow secondary siblings to be onlined)/? > later changes will add support for x86 to discard secondary threads in the is it worth to say 'later changes' ? Perhaps just enumerate the name of the patches? ..snip.. > --- a/include/linux/cpu.h > +++ b/include/linux/cpu.h > @@ -166,4 +166,16 @@ void cpuhp_report_idle_dead(void); > static inline void cpuhp_report_idle_dead(void) { } > #endif /* #ifdef CONFIG_HOTPLUG_CPU */ > > +enum { > + CPU_SMT_ENABLED, > + CPU_SMT_DISABLED, > + CPU_SMT_FORCE_DISABLED, > +}; > + > +#if defined(CONFIG_SMP) && defined(CONFIG_HOTPLUG_SMT) > +extern int cpu_smt_control; Could this be an named enum? We had this disaster with the ssb_something_cmd and ssb_something mismatch - and if we ever do something similar this can at least be nicely caught? > +#else > +# define cpu_smt_control (CPU_SMT_ENABLED) > +#endif > + > #endif /* _LINUX_CPU_H_ */ > --- a/kernel/cpu.c > +++ b/kernel/cpu.c > @@ -933,6 +933,29 @@ EXPORT_SYMBOL(cpu_down); > #define takedown_cpu NULL > #endif /*CONFIG_HOTPLUG_CPU*/ > > +#ifdef CONFIG_HOTPLUG_SMT > +int cpu_smt_control __read_mostly = CPU_SMT_ENABLED; > + > +static int __init smt_cmdline_disable(char *str) > +{ > + cpu_smt_control = CPU_SMT_DISABLED; > + if (str && !strcmp(str, "force")) { > + pr_info("SMT: Force disabled\n"); > + cpu_smt_control = CPU_SMT_FORCE_DISABLED; > + } > + return 0; > +} > +early_param("nosmt", smt_cmdline_disable); > + > +static inline bool cpu_smt_blocked(unsigned int cpu) May I suggest: cpu_smt_prohibited ? cpu_smt_allowed ? blocked sounds like an poll or something that returns -EBUSY whiile this will most certainly never do that. > +{ > + return cpu_smt_control != CPU_SMT_ENABLED && > + !topology_is_primary_thread(cpu); Heck, this could even be' cpu_smt_check and return -EPERM? > +} > +#else > +static inline bool cpu_smt_blocked(unsigned int cpu) { return false; } > +#endif > + > /** > * notify_cpu_starting(cpu) - Invoke the callbacks on the starting CPU > * @cpu: cpu that just started > @@ -1056,6 +1079,10 @@ static int do_cpu_up(unsigned int cpu, e > err = -EBUSY; > goto out; > } > + if (cpu_smt_blocked(cpu)) { > + err = -EPERM; .. which allow us to remove those '{}' but maybe I am overthinking it. > + goto out; > + } > > err = _cpu_up(cpu, 0, target); > out: > @@ -1904,10 +1931,115 @@ static const struct attribute_group cpuh > NULL > }; > > +#ifdef CONFIG_HOTPLUG_SMT > + > +static const char *smt_states[] = { > + "on", > + "off", > + "forceoff", > +}; May I suggest we follow the same pattern as in bugs.c? That is static const char *smt_stats[] = { [SMT_ENABLED] = "on", ... and so on? > + ..snip.. > +static ssize_t > +store_smt_control(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int val, ret; > + > + if (sysfs_streq(buf, "on")) > + val = CPU_SMT_ENABLED; > + else if (sysfs_streq(buf, "off")) > + val = CPU_SMT_DISABLED; > + else if (sysfs_streq(buf, "forceoff")) > + val = CPU_SMT_FORCE_DISABLED; > + else > + return -EINVAL; > + > + if (cpu_smt_control == CPU_SMT_FORCE_DISABLED) > + return -EPERM; > + > + ret = lock_device_hotplug_sysfs(); > + if (ret) > + return ret; > + > + if (val != cpu_smt_control) > + ret = val ? cpuhp_smt_disable(val) : cpuhp_smt_enable(); Just in case somebody messed up the enums would it make sense to add BUILD_BUG_ON(CPU_SMT_ENABLED != 0); so that this logic can still work properly? Or alterntively switch to an 'switch' statement?