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

[Jonathan Creekmore <jonathan.creekmore@gmail.com>]

Jan Beulich writes:

>>>> On 08.01.16 at 22:22, <jonathan.creekmore@gmail.com> 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.