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

[Juergen Gross <jgross@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel