[PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

As pointed out in the discussion of the patch at
http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
generalizing the conditions here means code elsewhere doesn't need to
take into consideration internals of how load balancing in the credit
scheduler works.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
    instead of cpu_online_map (suggested by Dario).

--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
 static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
                                            const cpumask_t *mask)
 {
-    if ( cpumask_full(vc->cpu_soft_affinity)
-         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
-        return 0;
-
-    return 1;
+    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
+                           vc->cpu_soft_affinity) &&
+           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
+           cpumask_intersects(vc->cpu_soft_affinity, mask);
 }
 
 /*




[-- Attachment #2: credit-has-soft-affinity.patch --]
[-- Type: text/plain, Size: 1211 bytes --]

credit: generalize __vcpu_has_soft_affinity()

As pointed out in the discussion of the patch at
http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html
generalizing the conditions here means code elsewhere doesn't need to
take into consideration internals of how load balancing in the credit
scheduler works.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
    instead of cpu_online_map (suggested by Dario).

--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
 static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
                                            const cpumask_t *mask)
 {
-    if ( cpumask_full(vc->cpu_soft_affinity)
-         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
-        return 0;
-
-    return 1;
+    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
+                           vc->cpu_soft_affinity) &&
+           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
+           cpumask_intersects(vc->cpu_soft_affinity, mask);
 }
 
 /*

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 616 bytes --]

On Fri, 2015-03-06 at 07:36 +0000, Jan Beulich wrote:
> As pointed out in the discussion of the patch at
> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
> generalizing the conditions here means code elsewhere doesn't need to
> take into consideration internals of how load balancing in the credit
> scheduler works.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>     instead of cpu_online_map (suggested by Dario).
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[George Dunlap]

On 03/06/2015 07:36 AM, Jan Beulich wrote:
> As pointed out in the discussion of the patch at
> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
> generalizing the conditions here means code elsewhere doesn't need to
> take into consideration internals of how load balancing in the credit
> scheduler works.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>     instead of cpu_online_map (suggested by Dario).
> 
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>                                             const cpumask_t *mask)
>  {
> -    if ( cpumask_full(vc->cpu_soft_affinity)
> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
> -        return 0;
> -
> -    return 1;
> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
> +                           vc->cpu_soft_affinity) &&
> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
> +           cpumask_intersects(vc->cpu_soft_affinity, mask);

It looks like the comment above this line could use changing too; perhaps:

---
Hard affinity balancing is always necessary and must never be skipped.
But soft affinity need only be considered when it has a functionally
different effect than other constraints (such as hard affinity, cpus
online, or cpupools).

Soft affinity only needs to be considered if:
* The cpus in the cpupool are not a subset of soft affinity
* The hard affinity is not a subset of soft affinity
* There is an overlap between the soft affinity and the mask which is
currently being considered.
---

With the comment updated:

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Jan Beulich]

>>> On 06.03.15 at 10:53, <george.dunlap@eu.citrix.com> wrote:
> On 03/06/2015 07:36 AM, Jan Beulich wrote:
>> As pointed out in the discussion of the patch at
>> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
>> generalizing the conditions here means code elsewhere doesn't need to
>> take into consideration internals of how load balancing in the credit
>> scheduler works.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>>     instead of cpu_online_map (suggested by Dario).
>> 
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>>                                             const cpumask_t *mask)
>>  {
>> -    if ( cpumask_full(vc->cpu_soft_affinity)
>> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
>> -        return 0;
>> -
>> -    return 1;
>> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>> +                           vc->cpu_soft_affinity) &&
>> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>> +           cpumask_intersects(vc->cpu_soft_affinity, mask);
> 
> It looks like the comment above this line could use changing too; perhaps:
> 
> ---
> Hard affinity balancing is always necessary and must never be skipped.
> But soft affinity need only be considered when it has a functionally
> different effect than other constraints (such as hard affinity, cpus
> online, or cpupools).
> 
> Soft affinity only needs to be considered if:
> * The cpus in the cpupool are not a subset of soft affinity
> * The hard affinity is not a subset of soft affinity

"hard" and "soft" appear to be swapped here. I corrected this,
please let me know if you disagree (in which case the patch would
need changing too).

> * There is an overlap between the soft affinity and the mask which is
> currently being considered.
> ---
> 
> With the comment updated:
> 
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Thanks, Jan

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1779 bytes --]

On Fri, 2015-03-06 at 09:53 +0000, George Dunlap wrote:
> On 03/06/2015 07:36 AM, Jan Beulich wrote:

> It looks like the comment above this line could use changing too;
> 
It probably does.

> ---
> Hard affinity balancing is always necessary and must never be skipped.
> But soft affinity need only be considered when it has a functionally
> different effect than other constraints (such as hard affinity, cpus
> online, or cpupools).
> 
> Soft affinity only needs to be considered if:
> * The cpus in the cpupool are not a subset of soft affinity
> * The hard affinity is not a subset of soft affinity
> * There is an overlap between the soft affinity and the mask which is
> currently being considered.
> ---
The original comment was more aimed at trying to make as evident as
possible to the reader why the occurrence of a particular combination of
hard and soft affinity makes using the latter pointless (perhaps it was
not that effective at that, but that's another story! :-D).

So, if changing it, I'd go for something more 'high level'; something
that helps the reader grasp more quickly what is the actual meaning of
those _subsect() and _intersect() operations. Perhaps:

---
Hard affinity balancing is always necessary and must never be skipped.
Soft affinity balancing is only useful if it, potentially, makes a
difference.

In more details, there is no point checking soft affinity if:
* there is no online pCPU in the domain's cpupool that is not in the
  soft affinity mask
* there is no pCPU in the hard affinity mask that is not in the soft
  affinity mask
* there is no overlapping between the soft affinity mask and the mask
  being considered (which, in most cases, is the hard affinity mask)
---

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[George Dunlap]

On 03/06/2015 10:16 AM, Jan Beulich wrote:
>>>> On 06.03.15 at 10:53, <george.dunlap@eu.citrix.com> wrote:
>> On 03/06/2015 07:36 AM, Jan Beulich wrote:
>>> As pointed out in the discussion of the patch at
>>> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
>>> generalizing the conditions here means code elsewhere doesn't need to
>>> take into consideration internals of how load balancing in the credit
>>> scheduler works.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>>>     instead of cpu_online_map (suggested by Dario).
>>>
>>> --- a/xen/common/sched_credit.c
>>> +++ b/xen/common/sched_credit.c
>>> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>>>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>>>                                             const cpumask_t *mask)
>>>  {
>>> -    if ( cpumask_full(vc->cpu_soft_affinity)
>>> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
>>> -        return 0;
>>> -
>>> -    return 1;
>>> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>>> +                           vc->cpu_soft_affinity) &&
>>> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>>> +           cpumask_intersects(vc->cpu_soft_affinity, mask);
>>
>> It looks like the comment above this line could use changing too; perhaps:
>>
>> ---
>> Hard affinity balancing is always necessary and must never be skipped.
>> But soft affinity need only be considered when it has a functionally
>> different effect than other constraints (such as hard affinity, cpus
>> online, or cpupools).
>>
>> Soft affinity only needs to be considered if:
>> * The cpus in the cpupool are not a subset of soft affinity
>> * The hard affinity is not a subset of soft affinity
> 
> "hard" and "soft" appear to be swapped here. I corrected this,
> please let me know if you disagree (in which case the patch would
> need changing too).

Uum -- I think my comment is right.  If the soft affinity is a subset of
hard affinity, then there are some cpus in the hard affinity which are
"preferred" (soft affine) and some that are "not preferred"
(non-soft-affine).  Whereas, if hard affinity is a subset of soft
affinity, then all cpus in the hard affinity are "preffered" (soft
affine), and so there's no sense in doing the soft affinity step.

In which case, yes, I think the patch needs to be adjusted.

Dario, am I crazy?

 -George

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[George Dunlap]

On 03/06/2015 11:12 AM, Dario Faggioli wrote:
> On Fri, 2015-03-06 at 09:53 +0000, George Dunlap wrote:
>> On 03/06/2015 07:36 AM, Jan Beulich wrote:
> 
>> It looks like the comment above this line could use changing too;
>>
> It probably does.
> 
>> ---
>> Hard affinity balancing is always necessary and must never be skipped.
>> But soft affinity need only be considered when it has a functionally
>> different effect than other constraints (such as hard affinity, cpus
>> online, or cpupools).
>>
>> Soft affinity only needs to be considered if:
>> * The cpus in the cpupool are not a subset of soft affinity
>> * The hard affinity is not a subset of soft affinity
>> * There is an overlap between the soft affinity and the mask which is
>> currently being considered.
>> ---
> The original comment was more aimed at trying to make as evident as
> possible to the reader why the occurrence of a particular combination of
> hard and soft affinity makes using the latter pointless (perhaps it was
> not that effective at that, but that's another story! :-D).
> 
> So, if changing it, I'd go for something more 'high level'; something
> that helps the reader grasp more quickly what is the actual meaning of
> those _subsect() and _intersect() operations. Perhaps:
> 
> ---
> Hard affinity balancing is always necessary and must never be skipped.
> Soft affinity balancing is only useful if it, potentially, makes a
> difference.
> 
> In more details, there is no point checking soft affinity if:
> * there is no online pCPU in the domain's cpupool that is not in the
>   soft affinity mask
> * there is no pCPU in the hard affinity mask that is not in the soft
>   affinity mask
> * there is no overlapping between the soft affinity mask and the mask
>   being considered (which, in most cases, is the hard affinity mask)
> ---

I think what you've said here is almost exactly the same thing as what I
said, except using different words. (i.e., "X not a subset of Y" means
"there are X which are not in Y")

I don't mind necessarily, but I'm not sure you've actually accomplished
your purpose. :-)

 -George

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Jan Beulich]

>>> On 06.03.15 at 12:32, <george.dunlap@eu.citrix.com> wrote:
> On 03/06/2015 10:16 AM, Jan Beulich wrote:
>>>>> On 06.03.15 at 10:53, <george.dunlap@eu.citrix.com> wrote:
>>> On 03/06/2015 07:36 AM, Jan Beulich wrote:
>>>> As pointed out in the discussion of the patch at
>>>> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
>>>> generalizing the conditions here means code elsewhere doesn't need to
>>>> take into consideration internals of how load balancing in the credit
>>>> scheduler works.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>>>>     instead of cpu_online_map (suggested by Dario).
>>>>
>>>> --- a/xen/common/sched_credit.c
>>>> +++ b/xen/common/sched_credit.c
>>>> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>>>>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>>>>                                             const cpumask_t *mask)
>>>>  {
>>>> -    if ( cpumask_full(vc->cpu_soft_affinity)
>>>> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
>>>> -        return 0;
>>>> -
>>>> -    return 1;
>>>> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>>>> +                           vc->cpu_soft_affinity) &&
>>>> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>>>> +           cpumask_intersects(vc->cpu_soft_affinity, mask);
>>>
>>> It looks like the comment above this line could use changing too; perhaps:
>>>
>>> ---
>>> Hard affinity balancing is always necessary and must never be skipped.
>>> But soft affinity need only be considered when it has a functionally
>>> different effect than other constraints (such as hard affinity, cpus
>>> online, or cpupools).
>>>
>>> Soft affinity only needs to be considered if:
>>> * The cpus in the cpupool are not a subset of soft affinity
>>> * The hard affinity is not a subset of soft affinity
>> 
>> "hard" and "soft" appear to be swapped here. I corrected this,
>> please let me know if you disagree (in which case the patch would
>> need changing too).
> 
> Uum -- I think my comment is right.  If the soft affinity is a subset of
> hard affinity, then there are some cpus in the hard affinity which are
> "preferred" (soft affine) and some that are "not preferred"
> (non-soft-affine).  Whereas, if hard affinity is a subset of soft
> affinity, then all cpus in the hard affinity are "preffered" (soft
> affine), and so there's no sense in doing the soft affinity step.

I.e. confusion about which affinity means what is ongoing:
Earlier on we had Dario answering a question of mine

>> Hmm, not sure. And I keep being confused whether soft means
>> "allow" and hard means "prefer" or the other way around. 
>>
>"hard" means allow (or not allow)
>"soft" means prefer

while I read your reply to mean the opposite.

Jan

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[George Dunlap]

On 03/06/2015 11:44 AM, Jan Beulich wrote:
>>>> On 06.03.15 at 12:32, <george.dunlap@eu.citrix.com> wrote:
>> On 03/06/2015 10:16 AM, Jan Beulich wrote:
>>>>>> On 06.03.15 at 10:53, <george.dunlap@eu.citrix.com> wrote:
>>>> On 03/06/2015 07:36 AM, Jan Beulich wrote:
>>>>> As pointed out in the discussion of the patch at
>>>>> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
>>>>> generalizing the conditions here means code elsewhere doesn't need to
>>>>> take into consideration internals of how load balancing in the credit
>>>>> scheduler works.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>>>>>     instead of cpu_online_map (suggested by Dario).
>>>>>
>>>>> --- a/xen/common/sched_credit.c
>>>>> +++ b/xen/common/sched_credit.c
>>>>> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>>>>>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>>>>>                                             const cpumask_t *mask)
>>>>>  {
>>>>> -    if ( cpumask_full(vc->cpu_soft_affinity)
>>>>> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
>>>>> -        return 0;
>>>>> -
>>>>> -    return 1;
>>>>> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>>>>> +                           vc->cpu_soft_affinity) &&
>>>>> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>>>>> +           cpumask_intersects(vc->cpu_soft_affinity, mask);
>>>>
>>>> It looks like the comment above this line could use changing too; perhaps:
>>>>
>>>> ---
>>>> Hard affinity balancing is always necessary and must never be skipped.
>>>> But soft affinity need only be considered when it has a functionally
>>>> different effect than other constraints (such as hard affinity, cpus
>>>> online, or cpupools).
>>>>
>>>> Soft affinity only needs to be considered if:
>>>> * The cpus in the cpupool are not a subset of soft affinity
>>>> * The hard affinity is not a subset of soft affinity
>>>
>>> "hard" and "soft" appear to be swapped here. I corrected this,
>>> please let me know if you disagree (in which case the patch would
>>> need changing too).
>>
>> Uum -- I think my comment is right.  If the soft affinity is a subset of
>> hard affinity, then there are some cpus in the hard affinity which are
>> "preferred" (soft affine) and some that are "not preferred"
>> (non-soft-affine).  Whereas, if hard affinity is a subset of soft
>> affinity, then all cpus in the hard affinity are "preffered" (soft
>> affine), and so there's no sense in doing the soft affinity step.
> 
> I.e. confusion about which affinity means what is ongoing:
> Earlier on we had Dario answering a question of mine
> 
>>> Hmm, not sure. And I keep being confused whether soft means
>>> "allow" and hard means "prefer" or the other way around. 
>>>
>> "hard" means allow (or not allow)
>> "soft" means prefer
> 
> while I read your reply to mean the opposite.

No, Dario and I mean the same thing: "Hard affinity" means "May only run
on these cpus and no others".  "Soft affinity" means "I prefer you to
run here if you can, but if it's too busy, go ahead and run it somewhere
else".

Hard affinity replaces what used to be called "vcpu_affinity"; and soft
affinity is typically used for vNUMA implementation.

Consider the following example

Soft affinity: 00001100
Hard affinity: 00111100

In this case, the scheduler should *prefer* to run it on cpus 4 or 5
(which is in both the soft and hard affinities), but *may* run it on
cpus 2-3 if it thinks it's necessary; so there is an "effective soft
affinity" -- even though soft affinity is a subset of hard affinity.

Now consider the reverse:

Soft affinity: 00111100
Hard affinity: 00001100

In this case, the scheduler *must* run it on either cpus 4 or 5; but
there is no preference between the two, since both are in the soft
affinity set.  So this is effectively the same as not having any soft
affinity.  And hard affinity is a subset of soft affinity.

 -George

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Jan Beulich]

>>> On 06.03.15 at 13:00, <george.dunlap@eu.citrix.com> wrote:
> On 03/06/2015 11:44 AM, Jan Beulich wrote:
>>>>> On 06.03.15 at 12:32, <george.dunlap@eu.citrix.com> wrote:
>>> On 03/06/2015 10:16 AM, Jan Beulich wrote:
>>>>>>> On 06.03.15 at 10:53, <george.dunlap@eu.citrix.com> wrote:
>>>>> On 03/06/2015 07:36 AM, Jan Beulich wrote:
>>>>>> As pointed out in the discussion of the patch at
>>>>>> http://lists.xenproject.org/archives/html/xen-devel/2015-02/msg03256.html 
>>>>>> generalizing the conditions here means code elsewhere doesn't need to
>>>>>> take into consideration internals of how load balancing in the credit
>>>>>> scheduler works.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> ---
>>>>>> v2: Use VCPU2ONLINE(vc) (or really an open coded variant thereof)
>>>>>>     instead of cpu_online_map (suggested by Dario).
>>>>>>
>>>>>> --- a/xen/common/sched_credit.c
>>>>>> +++ b/xen/common/sched_credit.c
>>>>>> @@ -292,11 +292,10 @@ __runq_remove(struct csched_vcpu *svc)
>>>>>>  static inline int __vcpu_has_soft_affinity(const struct vcpu *vc,
>>>>>>                                             const cpumask_t *mask)
>>>>>>  {
>>>>>> -    if ( cpumask_full(vc->cpu_soft_affinity)
>>>>>> -         || !cpumask_intersects(vc->cpu_soft_affinity, mask) )
>>>>>> -        return 0;
>>>>>> -
>>>>>> -    return 1;
>>>>>> +    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>>>>>> +                           vc->cpu_soft_affinity) &&
>>>>>> +           !cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>>>>>> +           cpumask_intersects(vc->cpu_soft_affinity, mask);
>>>>>
>>>>> It looks like the comment above this line could use changing too; perhaps:
>>>>>
>>>>> ---
>>>>> Hard affinity balancing is always necessary and must never be skipped.
>>>>> But soft affinity need only be considered when it has a functionally
>>>>> different effect than other constraints (such as hard affinity, cpus
>>>>> online, or cpupools).
>>>>>
>>>>> Soft affinity only needs to be considered if:
>>>>> * The cpus in the cpupool are not a subset of soft affinity
>>>>> * The hard affinity is not a subset of soft affinity
>>>>
>>>> "hard" and "soft" appear to be swapped here. I corrected this,
>>>> please let me know if you disagree (in which case the patch would
>>>> need changing too).
>>>
>>> Uum -- I think my comment is right.  If the soft affinity is a subset of
>>> hard affinity, then there are some cpus in the hard affinity which are
>>> "preferred" (soft affine) and some that are "not preferred"
>>> (non-soft-affine).  Whereas, if hard affinity is a subset of soft
>>> affinity, then all cpus in the hard affinity are "preffered" (soft
>>> affine), and so there's no sense in doing the soft affinity step.
>> 
>> I.e. confusion about which affinity means what is ongoing:
>> Earlier on we had Dario answering a question of mine
>> 
>>>> Hmm, not sure. And I keep being confused whether soft means
>>>> "allow" and hard means "prefer" or the other way around. 
>>>>
>>> "hard" means allow (or not allow)
>>> "soft" means prefer
>> 
>> while I read your reply to mean the opposite.
> 
> No, Dario and I mean the same thing: "Hard affinity" means "May only run
> on these cpus and no others".  "Soft affinity" means "I prefer you to
> run here if you can, but if it's too busy, go ahead and run it somewhere
> else".
> 
> Hard affinity replaces what used to be called "vcpu_affinity"; and soft
> affinity is typically used for vNUMA implementation.
> 
> Consider the following example
> 
> Soft affinity: 00001100
> Hard affinity: 00111100
> 
> In this case, the scheduler should *prefer* to run it on cpus 4 or 5
> (which is in both the soft and hard affinities), but *may* run it on
> cpus 2-3 if it thinks it's necessary; so there is an "effective soft
> affinity" -- even though soft affinity is a subset of hard affinity.
> 
> Now consider the reverse:
> 
> Soft affinity: 00111100
> Hard affinity: 00001100
> 
> In this case, the scheduler *must* run it on either cpus 4 or 5; but
> there is no preference between the two, since both are in the soft
> affinity set.  So this is effectively the same as not having any soft
> affinity.  And hard affinity is a subset of soft affinity.

Okay, good - less confusion. I.e. soft affinity is always a subset of
hard affinity (anything in excess is simply ignored). And then I agree
that the current check in the patch makes no sense, i.e. needs its
operands swapped. v3 in the works.

Jan

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2741 bytes --]

On Fri, 2015-03-06 at 12:00 +0000, George Dunlap wrote:
> >> On 03/06/2015 10:16 AM, Jan Beulich wrote:

> >>> Hmm, not sure. And I keep being confused whether soft means
> >>> "allow" and hard means "prefer" or the other way around. 
> >>>
> >> "hard" means allow (or not allow)
> >> "soft" means prefer
> > 
> > while I read your reply to mean the opposite.
> 
> No, Dario and I mean the same thing: "Hard affinity" means "May only run
> on these cpus and no others".  "Soft affinity" means "I prefer you to
> run here if you can, but if it's too busy, go ahead and run it somewhere
> else".
> 
Yep.

> Consider the following example
> 
> Soft affinity: 00001100
> Hard affinity: 00111100
> 
> In this case, the scheduler should *prefer* to run it on cpus 4 or 5
> (which is in both the soft and hard affinities), but *may* run it on
> cpus 2-3 if it thinks it's necessary; so there is an "effective soft
> affinity" -- even though soft affinity is a subset of hard affinity.
> 
Exactly.

So, in this case:

 cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) = 1

And, as far as this check only is concerned, __vcpu_has_soft_affinity()
should return 'true'.

> Now consider the reverse:
> 
> Soft affinity: 00111100
> Hard affinity: 00001100
> 
> In this case, the scheduler *must* run it on either cpus 4 or 5; but
> there is no preference between the two, since both are in the soft
> affinity set.  So this is effectively the same as not having any soft
> affinity.  And hard affinity is a subset of soft affinity.
> 
Correct again. So, here:

 cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) = 0

And __vcpu_has_soft_affinity() should (modulo the other parts) return
'false'.

Which means that, yes, this part of the condition must loose the '!', or
have the arguments of the call to cpumask_subset() switched. I
personally prefer the former:

    return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
                           vc->cpu_soft_affinity) &&
           cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
           cpumask_intersects(vc->cpu_soft_affinity, mask);

At which point it may make sense to turn everything into 'affermative'
logic. It at least look easier to read here:

    return cpumask_subset(vc->cpu_soft_affinity,
                          cpupool_online_cpumask(vc->domain->cpupool) &&
           cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
           cpumask_intersects(vc->cpu_soft_affinity, mask);


IAC, sorry for overlooking this, I was focusing more on the very first
item of the conditional expression, and managed to miss this. :-(

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1597 bytes --]

On Fri, 2015-03-06 at 11:38 +0000, George Dunlap wrote:
> On 03/06/2015 11:12 AM, Dario Faggioli wrote:

> > ---
> > Hard affinity balancing is always necessary and must never be skipped.
> > Soft affinity balancing is only useful if it, potentially, makes a
> > difference.
> > 
> > In more details, there is no point checking soft affinity if:
> > * there is no online pCPU in the domain's cpupool that is not in the
> >   soft affinity mask
> > * there is no pCPU in the hard affinity mask that is not in the soft
> >   affinity mask
> > * there is no overlapping between the soft affinity mask and the mask
> >   being considered (which, in most cases, is the hard affinity mask)
> > ---
> 
> I think what you've said here is almost exactly the same thing as what I
> said, except using different words. (i.e., "X not a subset of Y" means
> "there are X which are not in Y")
> 
Well, of course it is! I mean, it wouldn't have, if you comment were
wrong, which it wasn't. :-)

It's also exactly my point. In fact, in this case, adding something like
what you're suggesting to the comment seems to me to be aimed at making
the reader able to understand the code faster, isn't it? If yes, then
the words "subset" is already used in the code, and suggesting to use
different words is the entire point I'm trying to make! :-)

> I don't mind necessarily, but I'm not sure you've actually accomplished
> your purpose. :-)
> 
That's more than possible possible... It's why comments are so important
and, sometimes, so hard to write, I think! :-D

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Jan Beulich]

>>> On 06.03.15 at 14:23, <dario.faggioli@citrix.com> wrote:
> Which means that, yes, this part of the condition must loose the '!', or
> have the arguments of the call to cpumask_subset() switched. I
> personally prefer the former:
> 
>     return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
>                            vc->cpu_soft_affinity) &&
>            cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
>            cpumask_intersects(vc->cpu_soft_affinity, mask);

The form without ! and with operands swapped isn't correct afaict,
since subset(x,y) is not the same as !subset(y,x). In particular
when the two are identical, they are also subsets of one another.
Which is precisely a case you don't want the "soft" scheduling
cycle to be gone through.

Jan

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1000 bytes --]

On Fri, 2015-03-06 at 13:32 +0000, Jan Beulich wrote:
> >>> On 06.03.15 at 14:23, <dario.faggioli@citrix.com> wrote:
> > Which means that, yes, this part of the condition must loose the '!', or
> > have the arguments of the call to cpumask_subset() switched. I
> > personally prefer the former:
> > 
> >     return !cpumask_subset(cpupool_online_cpumask(vc->domain->cpupool),
> >                            vc->cpu_soft_affinity) &&
> >            cpumask_subset(vc->cpu_soft_affinity, vc->cpu_hard_affinity) &&
> >            cpumask_intersects(vc->cpu_soft_affinity, mask);
> 
> The form without ! and with operands swapped isn't correct afaict,
> since subset(x,y) is not the same as !subset(y,x).
>
I was thinking to loose the '!' and don't swap them, but...

> In particular
> when the two are identical, they are also subsets of one another.
>
... that is true, and is not the only issue the variant I suggested
above would introduce. So forget it, '!' it is. :-)

Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[George Dunlap]

On 03/06/2015 01:31 PM, Dario Faggioli wrote:
> On Fri, 2015-03-06 at 11:38 +0000, George Dunlap wrote:
>> On 03/06/2015 11:12 AM, Dario Faggioli wrote:
> 
>>> ---
>>> Hard affinity balancing is always necessary and must never be skipped.
>>> Soft affinity balancing is only useful if it, potentially, makes a
>>> difference.
>>>
>>> In more details, there is no point checking soft affinity if:
>>> * there is no online pCPU in the domain's cpupool that is not in the
>>>   soft affinity mask
>>> * there is no pCPU in the hard affinity mask that is not in the soft
>>>   affinity mask
>>> * there is no overlapping between the soft affinity mask and the mask
>>>   being considered (which, in most cases, is the hard affinity mask)
>>> ---
>>
>> I think what you've said here is almost exactly the same thing as what I
>> said, except using different words. (i.e., "X not a subset of Y" means
>> "there are X which are not in Y")
>>
> Well, of course it is! I mean, it wouldn't have, if you comment were
> wrong, which it wasn't. :-)
> 
> It's also exactly my point. In fact, in this case, adding something like
> what you're suggesting to the comment seems to me to be aimed at making
> the reader able to understand the code faster, isn't it? If yes, then
> the words "subset" is already used in the code, and suggesting to use
> different words is the entire point I'm trying to make! :-)

Well when you said "high level", I thought you meant explaining it in a
different way or from a different perspective, as opposed to using
non-Latin-based words. :-)

 -George

Re: [PATCH v2] credit: generalize __vcpu_has_soft_affinity()

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1024 bytes --]

On Fri, 2015-03-06 at 14:57 +0000, George Dunlap wrote:
> On 03/06/2015 01:31 PM, Dario Faggioli wrote:

> > It's also exactly my point. In fact, in this case, adding something like
> > what you're suggesting to the comment seems to me to be aimed at making
> > the reader able to understand the code faster, isn't it? If yes, then
> > the words "subset" is already used in the code, and suggesting to use
> > different words is the entire point I'm trying to make! :-)
> 
> Well when you said "high level", I thought you meant explaining it in a
> different way or from a different perspective, as opposed to using
> non-Latin-based words. :-)
> 
Mmm... I still think my variant was somewhat an higher level
description... but that it is something subjective, I guess, at least up
to a certain extent... or, now that you make me think about it, it might
be the Latin, as I always hated studying it in high school! :-P

Anyway, v3 is out with your wording, and, FWIW, I'm fine with it.

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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