Xen devel - Mar 2013 - [PATCH] xen: Always ask the scheduler to re-place the vcpu when the affinity changes

It''s probably a good idea to re-evaluate placement whenever the
affinity changes.

This additionally has the benefit of removing scheduler-specific
exceptions introduced in git c/s e6a6fd63.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/common/schedule.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index de11110..dbef5af 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -613,9 +613,10 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t
*affinity)
     vcpu_schedule_lock_irq(v);
 
     cpumask_copy(v->cpu_affinity, affinity);
-    if ( VCPU2OP(v)->sched_id == XEN_SCHEDULER_SEDF ||
-         !cpumask_test_cpu(v->processor, v->cpu_affinity) )
-        set_bit(_VPF_migrating, &v->pause_flags);
+
+    /* Always ask the scheduler to re-evaluate placement
+     * when changing the affinity */
+    set_bit(_VPF_migrating, &v->pause_flags);
 
     vcpu_schedule_unlock_irq(v);
 
-- 
1.7.9.5

>>> On 04.03.13 at 13:19, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> It''s probably a good idea to re-evaluate placement whenever the
> affinity changes.
> 
> This additionally has the benefit of removing scheduler-specific
> exceptions introduced in git c/s e6a6fd63.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
>  xen/common/schedule.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index de11110..dbef5af 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -613,9 +613,10 @@ int vcpu_set_affinity(struct vcpu *v, const cpumask_t 
> *affinity)
>      vcpu_schedule_lock_irq(v);
>  
>      cpumask_copy(v->cpu_affinity, affinity);
> -    if ( VCPU2OP(v)->sched_id == XEN_SCHEDULER_SEDF ||
> -         !cpumask_test_cpu(v->processor, v->cpu_affinity) )
> -        set_bit(_VPF_migrating, &v->pause_flags);
> +
> +    /* Always ask the scheduler to re-evaluate placement
> +     * when changing the affinity */
> +    set_bit(_VPF_migrating, &v->pause_flags);
>  
>      vcpu_schedule_unlock_irq(v);
>  
The code right below the context visible here is

    if ( test_bit(_VPF_migrating, &v->pause_flags) )
    {
        vcpu_sleep_nosync(v);
        vcpu_migrate(v);
    }

and I think the conditional could (and should) now be removed.

Jan

On 04/03/13 12:35, Jan Beulich wrote:
>>>> On 04.03.13 at 13:19, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
>> It''s probably a good idea to re-evaluate placement whenever
the
>> affinity changes.
>>
>> This additionally has the benefit of removing scheduler-specific
>> exceptions introduced in git c/s e6a6fd63.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> ---
>>   xen/common/schedule.c |    7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>> index de11110..dbef5af 100644
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -613,9 +613,10 @@ int vcpu_set_affinity(struct vcpu *v, const
cpumask_t
>> *affinity)
>>       vcpu_schedule_lock_irq(v);
>>   
>>       cpumask_copy(v->cpu_affinity, affinity);
>> -    if ( VCPU2OP(v)->sched_id == XEN_SCHEDULER_SEDF ||
>> -         !cpumask_test_cpu(v->processor, v->cpu_affinity) )
>> -        set_bit(_VPF_migrating, &v->pause_flags);
>> +
>> +    /* Always ask the scheduler to re-evaluate placement
>> +     * when changing the affinity */
>> +    set_bit(_VPF_migrating, &v->pause_flags);
>>   
>>       vcpu_schedule_unlock_irq(v);
>>   
> The code right below the context visible here is
>
>      if ( test_bit(_VPF_migrating, &v->pause_flags) )
>      {
>          vcpu_sleep_nosync(v);
>          vcpu_migrate(v);
>      }
>
> and I think the conditional could (and should) now be removed.
Yeah, I wasn''t sure what to make of that one -- it looked as though it 
was coded such that someone else might be able to clear _VPF_migrating 
between releasing the lock and this test.  But if that can happen, it''s
racy anyway.  vcpu_force_reschedule() has this "set, unlock, re-check"
pattern.

It looks like there actually is a way that it could conceivably be 
cleared: if the vcpu is running on another pcpu, if after we release the 
lock the vcpu is de-scheduled and context_saved() is called, it will 
check for _VPF_migrating and call vcpu_migrate(), which can clear the flag.

But that''s probably a rare enough occurrence that it''s better
overall to
take the occasional double-migrate.

  -George

On 04/03/13 13:45, George Dunlap wrote:
> On 04/03/13 12:35, Jan Beulich wrote:
>>>>> On 04.03.13 at 13:19, George Dunlap
<george.dunlap@eu.citrix.com>
>>>>> wrote:
>>> It''s probably a good idea to re-evaluate placement
whenever the
>>> affinity changes.
>>>
>>> This additionally has the benefit of removing scheduler-specific
>>> exceptions introduced in git c/s e6a6fd63.
>>>
>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>> ---
>>>   xen/common/schedule.c |    7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
>>> index de11110..dbef5af 100644
>>> --- a/xen/common/schedule.c
>>> +++ b/xen/common/schedule.c
>>> @@ -613,9 +613,10 @@ int vcpu_set_affinity(struct vcpu *v, const 
>>> cpumask_t
>>> *affinity)
>>>       vcpu_schedule_lock_irq(v);
>>>         cpumask_copy(v->cpu_affinity, affinity);
>>> -    if ( VCPU2OP(v)->sched_id == XEN_SCHEDULER_SEDF ||
>>> -         !cpumask_test_cpu(v->processor, v->cpu_affinity) )
>>> -        set_bit(_VPF_migrating, &v->pause_flags);
>>> +
>>> +    /* Always ask the scheduler to re-evaluate placement
>>> +     * when changing the affinity */
>>> +    set_bit(_VPF_migrating, &v->pause_flags);
>>>         vcpu_schedule_unlock_irq(v);
>> The code right below the context visible here is
>>
>>      if ( test_bit(_VPF_migrating, &v->pause_flags) )
>>      {
>>          vcpu_sleep_nosync(v);
>>          vcpu_migrate(v);
>>      }
>>
>> and I think the conditional could (and should) now be removed.
>
> Yeah, I wasn''t sure what to make of that one -- it looked as
though it
> was coded such that someone else might be able to clear _VPF_migrating 
> between releasing the lock and this test.  But if that can happen, 
> it''s racy anyway.  vcpu_force_reschedule() has this "set,
unlock,
> re-check" pattern.
>
> It looks like there actually is a way that it could conceivably be 
> cleared: if the vcpu is running on another pcpu, if after we release 
> the lock the vcpu is de-scheduled and context_saved() is called, it 
> will check for _VPF_migrating and call vcpu_migrate(), which can clear 
> the flag.
>
> But that''s probably a rare enough occurrence that it''s
better overall
> to take the occasional double-migrate.
Hmm -- but thinking it further, it actually seems likely that a 
different double-migrate race will happen:

1. vcpu is running on pcpu A
2. pcpu B runs set_affinity, setting VPF_migrate
3. pcpu B calls vcpu_sleep_nosync
4. pcpu A wakes up and grabs the schedule lock
5. pcpu A notices that VPF_migrate is set, and calls vcpu_migrate()
6. pcpu B calls vcpu_migrate()

Either that, or 6 happens before 4, but 4 still happens before pcpu B 
clears VPF_migrate.

It seems like we should really only call if (!v->is_running || 
v->processor == this_cpu).

Dario, any thoughts?

  -George

>
>  -George

On 04/03/2013 13:45, "George Dunlap"
<George.Dunlap@eu.citrix.com> wrote:
>> The code right below the context visible here is
>> 
>>      if ( test_bit(_VPF_migrating, &v->pause_flags) )
>>      {
>>          vcpu_sleep_nosync(v);
>>          vcpu_migrate(v);
>>      }
>> 
>> and I think the conditional could (and should) now be removed.
> 
> Yeah, I wasn''t sure what to make of that one -- it looked as
though it
> was coded such that someone else might be able to clear _VPF_migrating
> between releasing the lock and this test.  But if that can happen,
it''s
> racy anyway.  vcpu_force_reschedule() has this "set, unlock,
re-check"
> pattern.
> 
> It looks like there actually is a way that it could conceivably be
> cleared: if the vcpu is running on another pcpu, if after we release the
> lock the vcpu is de-scheduled and context_saved() is called, it will
> check for _VPF_migrating and call vcpu_migrate(), which can clear the flag.
> 
> But that''s probably a rare enough occurrence that it''s
better overall to
> take the occasional double-migrate.
Yes, the check is safe to remove, or indeed safe to keep.

 -- Keir

On 04/03/13 14:23, Keir Fraser wrote:
> On 04/03/2013 14:03, "George Dunlap"
<George.Dunlap@eu.citrix.com> wrote:
>
>>> But that''s probably a rare enough occurrence that
it''s better overall
>>> to take the occasional double-migrate.
>> Hmm -- but thinking it further, it actually seems likely that a
>> different double-migrate race will happen:
>>
>> 1. vcpu is running on pcpu A
>> 2. pcpu B runs set_affinity, setting VPF_migrate
>> 3. pcpu B calls vcpu_sleep_nosync
>> 4. pcpu A wakes up and grabs the schedule lock
>> 5. pcpu A notices that VPF_migrate is set, and calls vcpu_migrate()
>> 6. pcpu B calls vcpu_migrate()
>>
>> Either that, or 6 happens before 4, but 4 still happens before pcpu B
>> clears VPF_migrate.
>>
>> It seems like we should really only call if (!v->is_running ||
>> v->processor == this_cpu).
> It''s harmless for vcpu_migrate() to be called twice. It grabs
locks then
> checks it has work to do (and can do that work!).
If it''s calling it twice occasionally, that''s fine.  But if
it''s calling
it twice most of the time (and given the context I think that''s 
relatively likely), I think we should try to change that.

However, that may require some careful reworking; Jan, are you OK with 
checking the current patch in as-is, and having a separate patch to try 
to remove the if and avoid the double call?

  -George

On 04/03/2013 14:03, "George Dunlap"
<George.Dunlap@eu.citrix.com> wrote:
>> But that''s probably a rare enough occurrence that
it''s better overall
>> to take the occasional double-migrate.
> 
> Hmm -- but thinking it further, it actually seems likely that a
> different double-migrate race will happen:
> 
> 1. vcpu is running on pcpu A
> 2. pcpu B runs set_affinity, setting VPF_migrate
> 3. pcpu B calls vcpu_sleep_nosync
> 4. pcpu A wakes up and grabs the schedule lock
> 5. pcpu A notices that VPF_migrate is set, and calls vcpu_migrate()
> 6. pcpu B calls vcpu_migrate()
> 
> Either that, or 6 happens before 4, but 4 still happens before pcpu B
> clears VPF_migrate.
> 
> It seems like we should really only call if (!v->is_running ||
> v->processor == this_cpu).
It''s harmless for vcpu_migrate() to be called twice. It grabs locks
then
checks it has work to do (and can do that work!).

 -- Keir

>>> On 04.03.13 at 15:22, George Dunlap
<george.dunlap@eu.citrix.com> wrote:
> On 04/03/13 14:23, Keir Fraser wrote:
>> On 04/03/2013 14:03, "George Dunlap"
<George.Dunlap@eu.citrix.com> wrote:
>>
>>>> But that''s probably a rare enough occurrence that
it''s better overall
>>>> to take the occasional double-migrate.
>>> Hmm -- but thinking it further, it actually seems likely that a
>>> different double-migrate race will happen:
>>>
>>> 1. vcpu is running on pcpu A
>>> 2. pcpu B runs set_affinity, setting VPF_migrate
>>> 3. pcpu B calls vcpu_sleep_nosync
>>> 4. pcpu A wakes up and grabs the schedule lock
>>> 5. pcpu A notices that VPF_migrate is set, and calls vcpu_migrate()
>>> 6. pcpu B calls vcpu_migrate()
>>>
>>> Either that, or 6 happens before 4, but 4 still happens before pcpu
B
>>> clears VPF_migrate.
>>>
>>> It seems like we should really only call if (!v->is_running ||
>>> v->processor == this_cpu).
>> It''s harmless for vcpu_migrate() to be called twice. It grabs
locks then
>> checks it has work to do (and can do that work!).
> 
> If it''s calling it twice occasionally, that''s fine.  But
if it''s calling
> it twice most of the time (and given the context I think that''s 
> relatively likely), I think we should try to change that.
> 
> However, that may require some careful reworking; Jan, are you OK with 
> checking the current patch in as-is, and having a separate patch to try 
> to remove the if and avoid the double call?
Yes, with at least the description saying why the check is to remain
for the time being (to avoid unsuspecting readers like me asking why
it''s still there).

Jan