Re: [PATCH 1/4] build: Hook the schedulers into Kconfig

[George Dunlap <george.dunlap@citrix.com>]

On 18/12/15 01:35, Dario Faggioli wrote:
> On Thu, 2015-12-17 at 14:59 -0600, Jonathan Creekmore wrote:
>> Allow the schedulers to be independently enabled or disabled at
>> compile-time instead of just allowing the scheduler to be selected on
>> the command line. 
>>
> Reading this quickly, that "instead" gave me a bit of an hard time. I'm
> not a native English speaker, and I'm sure it's me that am wrong, but
> for some reason the sentence made me think that the patch would somehow
> disallow specifying a scheduler during boot, in Xen's command line.

I think the "just" in the sentence is an attempt to disambiguate (i.e.,
instead of *only* allowing the scheduler to be chosen on the
command-line, *also* allow it to be chosen at compile-time).

But I agree that the whole clause isn't really necessary and it would be
clearer without it.

 -George

> 
> Fact is, I don't think the phrase "instead of just allowing the
> scheduler to be selected on the command line." adds much information,
> and I'd just remove it.
> 
>> To match existing behavior, all four schedulers are
>> compiled in by default, although the RTDS and ARINC653 are marked
>> EXPERIMENTAL to match their not currently supported status.
>>
> This may change shortly, but as of now, Credit2 is still experimental
> too. I think I'd still recommend enabling it in the help file (i.e.,
> I'm ok with "If unsure, say Y"), but we certainly should mark it with
> "(EPERIMENTAL)".
> 
> I don't know much on how kconfig works, so all the changes to the
> makefile, etc, I'm not able to review them properly.
> 
> On the other hand, the code here below...
> 
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -38,8 +38,8 @@
>>  #include <public/sched.h>
>>  #include <xsm/xsm.h>
>>  
>> -/* opt_sched: scheduler - default to credit */
>> -static char __initdata opt_sched[10] = "credit";
>> +/* opt_sched: scheduler - default to configured value */
>> +static char __initdata opt_sched[10] = CONFIG_SCHED_DEFAULT;
>>  string_param("sched", opt_sched);
>>  
>>  /* if sched_smt_power_savings is set,
>> @@ -65,10 +65,18 @@ DEFINE_PER_CPU(struct schedule_data,
>> schedule_data);
>>  DEFINE_PER_CPU(struct scheduler *, scheduler);
>>  
>>  static const struct scheduler *schedulers[] = {
>> +#ifdef CONFIG_SCHED_CREDIT
>>      &sched_credit_def,
>> +#endif
>> +#ifdef CONFIG_SCHED_CREDIT2
>>      &sched_credit2_def,
>> +#endif
>> +#ifdef CONFIG_SCHED_ARINC653
>>      &sched_arinc653_def,
>> +#endif
>> +#ifdef CONFIG_SCHED_RTDS
>>      &sched_rtds_def,
>> +#endif
>>  };
>>  
> ... can have, with the changelog changed as shown, my:
> 
>  Acked-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> Regards,
> Daio
>