lkml.org 
[lkml]   [2020]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [RFC 1/2] x86/bugs: Disable coresched on hardware that does not need it
From
Date


On 13.11.20 16:55, Joel Fernandes wrote:
>
>
>>> +static enum coresched_cmds coresched_cmd __ro_after_init = CORE_SCHED_SECURE;
>>> +
>>> +static int __init coresched_parse_cmdline(char *arg)
>>> +{
>>> + if (!strcmp(arg, "off"))
>>> + coresched_cmd = CORE_SCHED_OFF;
>>> + else if (!strcmp(arg, "on"))
>>> + coresched_cmd = CORE_SCHED_ON;
>>> + else if (!strcmp(arg, "secure"))
>>> + coresched_cmd = CORE_SCHED_SECURE;
>>> + else
>>> + pr_crit("Unsupported coresched=%s, defaulting to secure.\n",
>>> + arg);
>>> +
>>> + return 0;
>>
>>
>> Instead of calling the matching function over and over again, can we just
>> configure a static branch (see below) based on the command line setting
>> here? Or do we not know about the bugs yet?
> [...]
>>> +static bool __coresched_supported(void)
>>> +{
>>> + /* coreched=off command line option. */
>>> + if (coresched_cmd_off())
>>> + return false;
>>> +
>>> + /*
>>> + * Some arch may not need coresched, example some x86 may not need
>>> + * coresched if coresched=secure option is passed (=secure is default).
>>> + */
>>> + return arch_allow_core_sched();
>>> +}
>>> +
>>> void sched_core_get(void)
>>> {
>>> + if (!__coresched_supported())
>>> + return;
>>
>> I would expect core scheduling to be basically an option that you set once
>> and never flip. This sounds like a prefect use case for a static branch to
>> me?
>
> Something ike so then? If Ok, let me know if I can add your Reviewed-by tag.
>
> thanks!
>
> ---8<-----------------------
>
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [PATCH] sched: Add a coresched command line option
>
> Some hardware such as certain AMD variants don't have cross-HT MDS/L1TF
> issues. Detect this and don't enable core scheduling as it can
> needlessly slow those device down.
>
> However, some users may want core scheduling even if the hardware is
> secure. To support them, add a coresched= option which defaults to
> 'secure' and can be overridden to 'on' if the user wants to enable
> coresched even if the HW is not vulnerable. 'off' would disable
> core scheduling in any case.
>
> Also add a sched_debug entry to indicate if core scheduling is turned on
> or not.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> arch/x86/kernel/cpu/bugs.c | 19 +++++++++++++++++++
> include/linux/cpu.h | 1 +
> include/linux/sched/smt.h | 4 ++++
> kernel/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++
> kernel/sched/core.c | 6 ++++++
> kernel/sched/debug.c | 4 ++++
> 6 files changed, 72 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index dece79e4d1e9..7607c9cd7f0f 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -43,6 +43,7 @@ static void __init mds_select_mitigation(void);
> static void __init mds_print_mitigation(void);
> static void __init taa_select_mitigation(void);
> static void __init srbds_select_mitigation(void);
> +static void __init coresched_select(void);
>
> /* The base value of the SPEC_CTRL MSR that always has to be preserved. */
> u64 x86_spec_ctrl_base;
> @@ -103,6 +104,9 @@ void __init check_bugs(void)
> if (boot_cpu_has(X86_FEATURE_STIBP))
> x86_spec_ctrl_mask |= SPEC_CTRL_STIBP;
>
> + /* Update whether core-scheduling is needed. */
> + coresched_select();
> +
> /* Select the proper CPU mitigations before patching alternatives: */
> spectre_v1_select_mitigation();
> spectre_v2_select_mitigation();
> @@ -1808,4 +1812,19 @@ ssize_t cpu_show_srbds(struct device *dev, struct device_attribute *attr, char *
> {
> return cpu_show_common(dev, attr, buf, X86_BUG_SRBDS);
> }
> +
> +/*
> + * When coresched=secure command line option is passed (default), disable core
> + * scheduling if CPU does not have MDS/L1TF vulnerability.
> + */
> +static void __init coresched_select(void)
> +{
> +#ifdef CONFIG_SCHED_CORE
> + if (!coresched_cmd_secure())

Make this a positive branch instead please :).

/*
* Disable core scheduling on non-MDS, non-L1TF systems
* when coresched=secure (default)
*/
if (coresched_cmd_secure() &&
!boot_cpu_has_bug(X86_BUG_MDS) &&
!boot_cpu_has_bug(X86_BUG_L1TF))
static_branch_disable(&sched_coresched_supported);

> + return;
> + if (!boot_cpu_has_bug(X86_BUG_MDS) && !boot_cpu_has_bug(X86_BUG_L1TF))
> + static_branch_disable(&sched_coresched_supported);
> +#endif
> +}
> +
> #endif
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index d6428aaf67e7..d1f1e64316d6 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -228,4 +228,5 @@ static inline int cpuhp_smt_disable(enum cpuhp_smt_control ctrlval) { return 0;
> extern bool cpu_mitigations_off(void);
> extern bool cpu_mitigations_auto_nosmt(void);
>
> +extern bool coresched_cmd_secure(void);
> #endif /* _LINUX_CPU_H_ */
> diff --git a/include/linux/sched/smt.h b/include/linux/sched/smt.h
> index 59d3736c454c..561064eb3268 100644
> --- a/include/linux/sched/smt.h
> +++ b/include/linux/sched/smt.h
> @@ -17,4 +17,8 @@ static inline bool sched_smt_active(void) { return false; }
>
> void arch_smt_update(void);
>
> +#ifdef CONFIG_SCHED_CORE
> +extern struct static_key_true sched_coresched_supported;
> +#endif
> +
> #endif
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6ff2578ecf17..b1cdfc7616b4 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2552,3 +2552,41 @@ bool cpu_mitigations_auto_nosmt(void)
> return cpu_mitigations == CPU_MITIGATIONS_AUTO_NOSMT;
> }
> EXPORT_SYMBOL_GPL(cpu_mitigations_auto_nosmt);
> +
> +/*
> + * These are used for a global "coresched=" cmdline option for controlling
> + * core scheduling. Note that core sched may be needed for usecases other
> + * than security as well.
> + */
> +enum coresched_cmds {
> + CORE_SCHED_OFF,
> + CORE_SCHED_SECURE,
> + CORE_SCHED_ON,
> +};
> +
> +static enum coresched_cmds coresched_cmd __ro_after_init = CORE_SCHED_SECURE;
> +
> +static int __init coresched_parse_cmdline(char *arg)
> +{
> + if (!strcmp(arg, "off"))
> + coresched_cmd = CORE_SCHED_OFF;
> + else if (!strcmp(arg, "on"))
> + coresched_cmd = CORE_SCHED_ON;
> + else if (!strcmp(arg, "secure"))
> + coresched_cmd = CORE_SCHED_SECURE;
> + else
> + pr_crit("Unsupported coresched=%s, defaulting to secure.\n",
> + arg);
> +
> + if (coresched_cmd == CORE_SCHED_OFF)
> + static_branch_disable(&sched_coresched_supported);
> + return 0;
> +}
> +early_param("coresched", coresched_parse_cmdline);
> +
> +/* coresched=secure */
> +bool coresched_cmd_secure(void)
> +{
> + return coresched_cmd == CORE_SCHED_SECURE;
> +}
> +EXPORT_SYMBOL_GPL(coresched_cmd_secure);
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 5ed26b469ed6..959fddf7d8de 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -333,8 +333,12 @@ static void __sched_core_disable(void)
> printk("core sched disabled\n");
> }
>
> +DEFINE_STATIC_KEY_TRUE(sched_coresched_supported);
> +
> void sched_core_get(void)
> {
> + if (!static_branch_likely(&sched_coresched_supported))
> + return;
> mutex_lock(&sched_core_mutex);
> if (!sched_core_count++)
> __sched_core_enable();
> @@ -343,6 +347,8 @@ void sched_core_get(void)
>
> void sched_core_put(void)
> {
> + if (!static_branch_likely(&sched_coresched_supported))
> + return;
> mutex_lock(&sched_core_mutex);
> if (!--sched_core_count)
> __sched_core_disable();
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index 88bf45267672..935b68be18cd 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -782,6 +782,10 @@ static void sched_debug_header(struct seq_file *m)
> "sysctl_sched_tunable_scaling",
> sysctl_sched_tunable_scaling,
> sched_tunable_scaling_names[sysctl_sched_tunable_scaling]);
> +#ifdef CONFIG_SCHED_CORE
> + SEQ_printf(m, " .%-40s: %d\n", "core_sched_enabled",
> + !!static_branch_likely(&__sched_core_enabled));
> +#endif
> SEQ_printf(m, "\n");
> }
>
>

[...]

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a36f08d74e09..8de377dc8592 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -698,6 +698,15 @@
> /proc/<pid>/coredump_filter.
> See also Documentation/filesystems/proc.rst.
>
> + coresched=

It would be nice to list the possible arguments here too.

> + [SCHED_CORE] Enable/disable the core scheduling feature.
> + A value of 'on' keeps coresched on always. A value of

This reads as if coresched=on means that all your tasks are core
scheduled. I'd prefer if you could clarify the option a bit to mean that
this *plus* tagging gets your core scheduling.


Alex

> + 'off' keeps coresched off always. A value of 'secure'
> + keeps it on only if the system has vulnerabilities. Defaults
> + to 'secure'. A user who has a non-security usecase that needs
> + core scheduling, such as improving performance of VMs by
> + tagging vCPU should pass 'on' to force it on.
> +
> coresight_cpu_debug.enable
> [ARM,ARM64]
> Format: <bool>




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



\
 
 \ /
  Last update: 2020-11-16 16:41    [W:0.719 / U:0.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site