[49/60] xen/sched: reject switching smt on/off with core scheduling active
diff mbox series

Message ID 20190528103313.1343-50-jgross@suse.com
State New, archived
Headers show
Series
  • xen: add core scheduling support
Related show

Commit Message

Jürgen Groß May 28, 2019, 10:33 a.m. UTC
When core or socket scheduling are active enabling or disabling smt is
not possible as that would require a major host reconfiguration.

Add a bool sched_disable_smt_switching which will be set for core or
socket scheduling.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1: new patch
---
 xen/arch/x86/sysctl.c   | 3 ++-
 xen/common/schedule.c   | 1 +
 xen/include/xen/sched.h | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

Comments

Jan Beulich May 28, 2019, 11:44 a.m. UTC | #1
>>> On 28.05.19 at 12:33, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -200,7 +200,8 @@ long arch_do_sysctl(
>  
>          case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
>          case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
> -            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings < 2 )
> +            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings < 2 ||
> +                 sched_disable_smt_switching )
>              {
>                  ret = -EOPNOTSUPP;
>                  break;

I'm not convinced -EOPNOTSUPP is an appropriate error code for
this new case. -EPERM, -EACCES, or -EIO would all seem more
appropriate to me (and perhaps there are further ones).

> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -57,6 +57,7 @@ integer_param("sched_ratelimit_us", sched_ratelimit_us);
>  
>  /* Number of vcpus per struct sched_unit. */
>  static unsigned int sched_granularity = 1;
> +bool sched_disable_smt_switching;

__read_mostly (perhaps also the pre-existing variable in context)?

Jan
Jürgen Groß May 28, 2019, 11:52 a.m. UTC | #2
On 28/05/2019 13:44, Jan Beulich wrote:
>>>> On 28.05.19 at 12:33, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -200,7 +200,8 @@ long arch_do_sysctl(
>>  
>>          case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
>>          case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
>> -            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings < 2 )
>> +            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings < 2 ||
>> +                 sched_disable_smt_switching )
>>              {
>>                  ret = -EOPNOTSUPP;
>>                  break;
> 
> I'm not convinced -EOPNOTSUPP is an appropriate error code for
> this new case. -EPERM, -EACCES, or -EIO would all seem more
> appropriate to me (and perhaps there are further ones).

I think -EIO or -EBUSY would be the best fit.

> 
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -57,6 +57,7 @@ integer_param("sched_ratelimit_us", sched_ratelimit_us);
>>  
>>  /* Number of vcpus per struct sched_unit. */
>>  static unsigned int sched_granularity = 1;
>> +bool sched_disable_smt_switching;
> 
> __read_mostly (perhaps also the pre-existing variable in context)?

Yes, can do.


Juergen
Dario Faggioli June 12, 2019, 9:36 a.m. UTC | #3
On Tue, 2019-05-28 at 13:52 +0200, Juergen Gross wrote:
> On 28/05/2019 13:44, Jan Beulich wrote:
> > > > > On 28.05.19 at 12:33, <jgross@suse.com> wrote:
> > > --- a/xen/arch/x86/sysctl.c
> > > +++ b/xen/arch/x86/sysctl.c
> > > @@ -200,7 +200,8 @@ long arch_do_sysctl(
> > >  
> > >          case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
> > >          case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
> > > -            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings
> > > < 2 )
> > > +            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings
> > > < 2 ||
> > > +                 sched_disable_smt_switching )
> > >              {
> > >                  ret = -EOPNOTSUPP;
> > >                  break;
> > 
> > I'm not convinced -EOPNOTSUPP is an appropriate error code for
> > this new case. -EPERM, -EACCES, or -EIO would all seem more
> > appropriate to me (and perhaps there are further ones).
> 
> I think -EIO or -EBUSY would be the best fit.
> 
I agree, with mild preference for EBUSY.

Regards

Patch
diff mbox series

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 3f06fecbd8..034d78fe67 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -200,7 +200,8 @@  long arch_do_sysctl(
 
         case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
         case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
-            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings < 2 )
+            if ( !cpu_has_htt || boot_cpu_data.x86_num_siblings < 2 ||
+                 sched_disable_smt_switching )
             {
                 ret = -EOPNOTSUPP;
                 break;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 3c85861b15..8607262a71 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -57,6 +57,7 @@  integer_param("sched_ratelimit_us", sched_ratelimit_us);
 
 /* Number of vcpus per struct sched_unit. */
 static unsigned int sched_granularity = 1;
+bool sched_disable_smt_switching;
 const cpumask_t *sched_res_mask = &cpumask_all;
 
 /* Various timer handlers. */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b6496f57f6..7dc63c449b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1020,6 +1020,7 @@  static inline bool is_vcpu_online(const struct vcpu *v)
 }
 
 extern bool sched_smt_power_savings;
+extern bool sched_disable_smt_switching;
 
 extern enum cpufreq_controller {
     FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen