Re: [Xen-devel] [PATCH v4 2/5] build: Hook the schedulers into Kconfig

[Jonathan Creekmore <jonathan.creekmore@xxxxxxxxx>]

Jan Beulich writes:

>>>> On 08.01.16 at 22:22, <jonathan.creekmore@xxxxxxxxx> wrote:
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -51,4 +51,71 @@ config KEXEC
>>
>>        If unsure, say Y.
>>
>> +# Enable schedulers
>> +menu "Schedulers"
>> +    visible if EXPERT = "y"
>
> Does "visible if EXPERT" not suffice here?

No, because EXPERT is a string and not a boolean. It has to be a string
since it is pulling in a string from the environment to set its value.

>
>> +config SCHED_CREDIT
>> +    bool "Credit scheduler support"
>> +    default y
>
> I continue to think that not making the primary scheduler configurable
> would be the better solution to the problems resulting from possibly
> all of them getting turned off.

Except that is completely contrary to my goal with this patchset (being
able to compile in just the scheduler that I want to use). Yes, at the
moment, credit is the only non-experimental scheduler and will likely be
the one we choose. However, in the future, when credit2 and possibly
others are non-experimental, we may choose one of the other schedulers
and do not want to carry along credit in our build just because it is
the primary scheduler.

>
>> +config SCHED_CREDIT2
>> +    bool "Credit2 scheduler support (EXPERIMENTAL)"
>> +    default y
>> +    ---help---
>> +      The credit2 scheduler is a general purpose scheduler that is
>> +      optimized for lower latency and higher VM density.
>> +
>> +      If unsure, say Y.
>> +
>> +config SCHED_RTDS
>> +    bool "RTDS scheduler support (EXPERIMENTAL)"
>> +    default y
>> +    ---help---
>> +      The RTDS scheduler is a soft and firm real-time scheduler for
>> +      multicore, targeted for embedded, automotive, graphics and gaming
>> +      in the cloud, and general low-latency workloads.
>> +
>> +      If unsure, say N.
>> +
>> +config SCHED_ARINC653
>> +    bool "ARINC653 scheduler support (EXPERIMENTAL)"
>> +    default y
>> +    ---help---
>> +      The ARINC653 scheduler is a hard real-time scheduler for single
>> +      cores, targeted for avionics, drones, and medical devices.
>> +
>> +      If unsure, say N.
>> +
>> +choice
>> +    prompt "Default Scheduler?"
>> +    default SCHED_CREDIT_DEFAULT if SCHED_CREDIT
>> +    default SCHED_CREDIT2_DEFAULT if SCHED_CREDIT2
>> +    default SCHED_RTDS_DEFAULT if SCHED_RTDS
>> +    default SCHED_ARINC653_DEFAULT if SCHED_ARINC653
>> +
>> +    config SCHED_CREDIT_DEFAULT
>> +            bool "Credit Scheduler" if SCHED_CREDIT
>> +    config SCHED_CREDIT2_DEFAULT
>> +            bool "Credit2 Scheduler" if SCHED_CREDIT2
>> +    config SCHED_RTDS_DEFAULT
>> +            bool "RT Scheduler" if SCHED_RTDS
>> +    config SCHED_ARINC653_DEFAULT
>> +            bool "ARINC653 Scheduler" if SCHED_ARINC653
>> +endchoice
>> +
>> +config SCHED_DEFAULT
>> +    string
>> +    default "credit" if SCHED_CREDIT_DEFAULT
>> +    default "credit2" if SCHED_CREDIT2_DEFAULT
>> +    default "rtds" if SCHED_RTDS_DEFAULT
>> +    default "arinc653" if SCHED_ARINC653_DEFAULT
>> +    default "credit"
>
> What use is this last line?
>

When the scheduler menu is not visible, the choice selection does not
cause a scheduler to be chosen. The last line forces credit to be the
default if none of the _DEFAULT values are selected. It is purely an
artifact of introducing the visibility option on the menu. It could be
moved into the defaults section for the choice like this:

+       default SCHED_CREDIT_DEFAULT if SCHED_CREDIT
+       default SCHED_CREDIT2_DEFAULT if SCHED_CREDIT2
+       default SCHED_RTDS_DEFAULT if SCHED_RTDS
+       default SCHED_ARINC653_DEFAULT if SCHED_ARINC653
+       default SCHED_CREDIT_DEFAULT


>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -850,8 +850,13 @@ static inline bool_t is_vcpu_online(const struct vcpu 
>> *v)
>>      return !test_bit(_VPF_down, &v->pause_flags);
>>  }
>>
>> +#ifdef CONFIG_SCHED_CREDIT
>>  void set_vcpu_migration_delay(unsigned int delay);
>>  unsigned int get_vcpu_migration_delay(void);
>> +#else
>> +static inline void set_vcpu_migration_delay(unsigned int delay) { }
>> +static inline unsigned int get_vcpu_migration_delay(void) { return 0; }
>> +#endif
>
> I don't think these are appropriate: The respective sysctl sub-ops
> would probably better indicate failure to the caller.

I can make that change if you want me to. As it stands now, the existing
sysctl sub-ops are probably not doing the right thing since they are
setting and getting this migration delay in the credit scheduler
regardless of which scheduler is actually in use.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel