[MODERATED] Re: [patch V2 05/12] cpu/hotplug: Provide knob to control SMT

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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 <tglx@linutronix.de>
> 
> 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 <shrugs> 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?