Re: [Xen-devel] [PATCH v3 29/47] xen/sched: introduce unit_runnable_state()

[Jürgen Groß <jgross@xxxxxxxx>]

On 23.09.19 17:24, Jan Beulich wrote:
> On 14.09.2019 10:52, Juergen Gross wrote:
> > Today the vcpu runstate of a new scheduled vcpu is always set to
> > "running" even if at that time vcpu_runnable() is already returning
> > false due to a race (e.g. with pausing the vcpu).
> I can see this part, ...
>
> > With core scheduling this can no longer work as not all vcpus of a
> > schedule unit have to be "running" when being scheduled. So the vcpu's
> > new runstate has to be selected at the same time as the runnability of
> > the related schedule unit is probed.
> ... but I continue having trouble here. If it has been okay to set
> a vCPU no longer runnable to "running" nevertheless, why would the
> same not be true for schedule units? Part of the problem may be
> that ...
The difference is the need to drop the scheduling lock for doing the
rendezvous. vcpu_sleep() or vcpu_wake() could now interfere with
scheduling in a way which was not possible before.

> > --- a/xen/include/xen/sched-if.h
> > +++ b/xen/include/xen/sched-if.h
> > @@ -76,6 +76,29 @@ static inline bool unit_runnable(const struct sched_unit 
> > *unit)
> >       return vcpu_runnable(unit->vcpu_list);
> ... this clearly still isn't doing the (I suppose) intended loop,
> and hence ...
>
> >   }
> >   
> > +static inline bool unit_runnable_state(const struct sched_unit *unit)
> > +{
> > +    struct vcpu *v;
> > +    bool runnable, ret = false;
> > +
> > +    if ( is_idle_unit(unit) )
> > +        return true;
> > +
> > +    for_each_sched_unit_vcpu ( unit, v )
> > +    {
> > +        runnable = vcpu_runnable(v);
> > +
> > +        v->new_state = runnable ? RUNSTATE_running
> > +                                : (v->pause_flags & VPF_blocked)
> > +                                  ? RUNSTATE_blocked : RUNSTATE_offline;
> > +
> > +        if ( runnable )
> > +            ret = true;
> > +    }
> > +
> > +    return ret;
> > +}
> ... it's not obvious what the eventual difference between the two is
> going to be.
>
> Furthermore I think a function of the given name, returning bool, and
> taking a pointer to const deserves a comment as to the (possibly
> slightly unexpected) state change it does. This comment might then be
> worthwhile to extend to also outline the usage difference between it
> and its sibling above.
I'll add that.

> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -174,6 +174,7 @@ struct vcpu
> >           XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t) compat;
> >       } runstate_guest; /* guest address */
> >   #endif
> > +    unsigned int     new_state;
> Similarly I think it would be nice for this field to gain a brief
> comment as to its purpose compared to runstate.state.
Okay.


Juergen


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