Re: [PATCH v2] sched: credit2: respect per-vcpu hard affinity

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

On 03/09/2015 07:11 AM, Justin Weaver wrote:
>>>>> +static int get_safe_pcpu(struct csched2_vcpu *svc)
>>>>> +{
>>>>>
>>>> I also don't like the name... __choose_cpu() maybe ?
>>>
>>> I'm not 100% happy with the name, but I think "get_safe_pcpu" is more
>>> descriptive than just "__choose_cpu".
>>>
>> I don't like the "_safe_" part, but that is not a big deal, I certainly
>> can live with it.
> 
> I changed it to _choose_valid_pcpu ... discuss! (Also, I can send out
> a pre-patch to change the double underscores in the whole file)

What about "get_fallback_cpu()"?  That's basically what we want when
this function is called -- the runqueue we wanted wasn't available, so
we just want somewhere else reasonable to put it.

>>>> Oh, and, with either yours or my variant... can csched2_cpumask end up
>>>> empty after the two &&-s? That's important or, below, cpumask_any will
>>>> return garbage! :-/
>>>>
>>>> From a quick inspection, it does not seem it can, in which case, I'd put
>>>> down an ASSERT() about this somewhere. OTOH, if it can, I'd look for
>>>> ways of preventing that to happen before getting here... It seems easier
>>>> and more correct to do that, rather than trying to figure out what to do
>>>> here if the mask is empty. :-O
>>>
>>> Yes, we need to go through the code and make sure that we understand
>>> what our assumptions are, so that people can't crash Xen by doing
>>> irrational things like making the hard affinity not intersect with the
>>> cpupool cpus.
>>>
>> True.
>>
>> Something like that can be figured out and either forbidden or, in
>> general, addressed in other places, rather than requiring us to care
>> here. In fact, this seems to me to be what's happening already (see
>> below).
> 
> I don't think there's any way the mask can be empty after the
> cpumask_and with the cpu_hard_affinity and the VCPU2ONLINE. In
> schedule.c:vcpu_set_hard_affinity, if the intersection of the new hard
> mask and VCPU2ONLINE is empty, the function returns -EINVAL. I took
> into account all the discussion here and modified the function for v3.

What about this:

* Create a cpu pool with cpus 0 and 1.  online_cpus is now [0,1].
* Set a hard affinity of [1].  This succeeds.
* Move cpu 1 to a different cpupool.

After a quick look I don't see anything that updates the hard affinity
when cpus are removed from pools.

And, even if it does *now*, it's possible that something might be
changed in the future that would forget it; this ASSERT() isn't exactly
next to that code.

It seems like handling the case where hard_affinity and cpus_online
don't overlap would be fairly simple; and if there was ever a bug such
that this was possible, handling the case would change that bug from
"hit an ASSERT" (or "have undefined behavior") to "have well-defined
behavior".  So it seems to me like handling that case makes the software
more robust for little cost.

 -George