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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

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

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

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

> @@ -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_*()?

Jan

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