Re: [Xen-devel] [PATCH v2 27/48] xen/sched: Change vcpu_migrate_*() to operate on schedule unit

[Juergen Gross <jgross@suse.com>]

On 10.09.19 17:11, Jan Beulich wrote:
> On 09.08.2019 16:58, Juergen Gross wrote:
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -733,35 +733,40 @@ void vcpu_unblock(struct vcpu *v)
>>   }
>>   
>>   /*
>> - * Do the actual movement of a vcpu from old to new CPU. Locks for *both*
>> + * Do the actual movement of an unit from old to new CPU. Locks for *both*
>>    * CPUs needs to have been taken already when calling this!
>>    */
>> -static void vcpu_move_locked(struct vcpu *v, unsigned int new_cpu)
>> +static void sched_unit_move_locked(struct sched_unit *unit,
>> +                                   unsigned int new_cpu)
>>   {
>> -    unsigned int old_cpu = v->processor;
>> +    unsigned int old_cpu = unit->res->processor;
>> +    struct vcpu *v;
>>   
>>       /*
>>        * Transfer urgency status to new CPU before switching CPUs, as
>>        * once the switch occurs, v->is_urgent is no longer protected by
>>        * the per-CPU scheduler lock we are holding.
>>        */
>> -    if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) )
>> +    for_each_sched_unit_vcpu ( unit, v )
>>       {
>> -        atomic_inc(&get_sched_res(new_cpu)->urgent_count);
>> -        atomic_dec(&get_sched_res(old_cpu)->urgent_count);
>> +        if ( unlikely(v->is_urgent) && (old_cpu != new_cpu) )
>> +        {
>> +            atomic_inc(&get_sched_res(new_cpu)->urgent_count);
>> +            atomic_dec(&get_sched_res(old_cpu)->urgent_count);
>> +        }
>>       }
> 
> Shouldn't is_urgent become an attribute of unit rather than a vCPU,
> too, eliminating the need for a loop here? I can't see a reason
> why not, seeing this collapsing into a single urgent_count.

With moving urgent_count to a percpu variable this no longer applies.

> 
> Then again the question remains whether the non-deep sleeping as
> a result of a non-zero urgent_count should indeed be distributed
> to all constituents of a unit. I can see arguments both in favor
> and against.

Against has won. :-)

> 
>> -static void vcpu_migrate_finish(struct vcpu *v)
>> +static void sched_unit_migrate_finish(struct sched_unit *unit)
>>   {
>>       unsigned long flags;
>>       unsigned int old_cpu, new_cpu;
>>       spinlock_t *old_lock, *new_lock;
>>       bool_t pick_called = 0;
>> +    struct vcpu *v;
>>   
>>       /*
>> -     * If the vcpu is currently running, this will be handled by
>> +     * If the unit is currently running, this will be handled by
>>        * context_saved(); and in any case, if the bit is cleared, then
>>        * someone else has already done the work so we don't need to.
>>        */
>> -    if ( v->sched_unit->is_running ||
>> -         !test_bit(_VPF_migrating, &v->pause_flags) )
>> -        return;
>> +    for_each_sched_unit_vcpu ( unit, v )
>> +    {
>> +        if ( unit->is_running || !test_bit(_VPF_migrating, &v->pause_flags) )
>> +            return;
>> +    }
> 
> Do you really need the loop invariant unit->is_running to be evaluated
> once per loop iteration? (Same again further down at least once.)

No, I should test that before entering the loop.

> 
> Furthermore I wonder if VPF_migrating shouldn't become a per-unit
> attribute.

This would make vcpu_runnable() much more complicated. I don't think
that is worth it.

> 
>> @@ -858,22 +871,30 @@ static void vcpu_migrate_finish(struct vcpu *v)
>>        * because they both happen in (different) spinlock regions, and those
>>        * regions are strictly serialised.
>>        */
>> -    if ( v->sched_unit->is_running ||
>> -         !test_and_clear_bit(_VPF_migrating, &v->pause_flags) )
>> +    for_each_sched_unit_vcpu ( unit, v )
>>       {
>> -        sched_spin_unlock_double(old_lock, new_lock, flags);
>> -        return;
>> +        if ( unit->is_running ||
>> +             !test_and_clear_bit(_VPF_migrating, &v->pause_flags) )
>> +        {
>> +            sched_spin_unlock_double(old_lock, new_lock, flags);
>> +            return;
>> +        }
>>       }
>>   
>> -    vcpu_move_locked(v, new_cpu);
>> +    sched_unit_move_locked(unit, new_cpu);
>>   
>>       sched_spin_unlock_double(old_lock, new_lock, flags);
>>   
>>       if ( old_cpu != new_cpu )
>> -        sched_move_irqs(v->sched_unit);
>> +    {
>> +        for_each_sched_unit_vcpu ( unit, v )
>> +            sync_vcpu_execstate(v);
> 
> This is new without being explained anywhere. Or wait, it is mentioned
> (with the wrong function name, which is why initially - by searching -
> I didn't spot it), but only with a justification of "needed anyway".

I'll correct it and make it more verbose.

> 
>> @@ -1794,7 +1814,7 @@ void context_saved(struct vcpu *prev)
>>   
>>       sched_context_saved(vcpu_scheduler(prev), prev->sched_unit);
>>   
>> -    vcpu_migrate_finish(prev);
>> +    sched_unit_migrate_finish(prev->sched_unit);
>>   }
> 
> By the end of the series context_saved() still acts on vCPU-s, not
> units. What is the meaning/effect of multiple sched_unit_migrate_*()?

That's corrected in V3 by having split context_saved() into a vcpu- and
a unit-part.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel