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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

Jan

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