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

From: "Jan Beulich" <JBeulich@suse.com>
To: Jonathan Creekmore <jonathan.creekmore@gmail.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>,
	xen-devel@lists.xenproject.org,
	Dario Faggioli <dario.faggioli@citrix.com>
Subject: Re: [PATCH v4 2/5] build: Hook the schedulers into Kconfig
Date: Mon, 11 Jan 2016 08:43:19 -0700	[thread overview]
Message-ID: <5693DBA702000078000C5851@prv-mh.provo.novell.com> (raw)
In-Reply-To: <m237u440eu.fsf@Nebula.lan>

>>> On 11.01.16 at 16:10, <jonathan.creekmore@gmail.com> wrote:
> Jan Beulich writes:
>>>>> On 08.01.16 at 22:22, <jonathan.creekmore@gmail.com> wrote:
>>> +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.

I think we need to separate what we want as "upstream", and what
your internal intentions are. Any gap between that would need to be
taken care of by private patches you'd have to carry.

As "upstream", I think not allowing the default scheduler to be turned
off is quite reasonable an approach.

>>> --- 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.

Yes and no - it would still change / obtain the value for those
parts of the system (CPU pools) where that scheduler is being
used. That set may be empty, but the scheduler is still present,
and hence the operations succeeding is a valid thing as long as
that scheduler exists. This changes, though, if the scheduler
doesn't get built anymore. (And not how the problem would go
away altogether if credit wasn't configurable.)

Jan