[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.