[Xen-devel] [RTDS Patch v3 for Xen4.8]

[Haoran Li <naroahlee@xxxxxxxxx>]

From: naroahlee <naroahlee@xxxxxxxxx>

 When more than one idle VCPUs that have
 the same PCPU as their previous running core invoke runq_tickle(), they will
 tickle the same PCPU. The tickled PCPU will only pick at most one VCPU, i.e.,
 the highest-priority one, to execute. The other VCPUs will not be scheduled
 for a period, even when there is an idle core, making these VCPUs
 unnecessarily starve for one period. Therefore, always make sure that we only
 tickle PCPUs that have not been tickled already.

Signed-off-by: Haoran Li <naroahlee@xxxxxxxxx>
Reviewed-by:   Meng Xu   <mengxu@xxxxxxxxxxxxx>

---
 xen/common/sched_rt.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 1b30014..b3d55d8 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1144,12 +1144,11 @@ rt_vcpu_sleep(const struct scheduler *ops, struct vcpu 
*vc)
  * Called by wake() and context_saved()
  * We have a running candidate here, the kick logic is:
  * Among all the cpus that are within the cpu affinity
- * 1) if the new->cpu is idle, kick it. This could benefit cache hit
- * 2) if there are any idle vcpu, kick it.
- * 3) now all pcpus are busy;
+ * 1) if there are any idle vcpu, kick it.
+ *    For cache benefit, we first search new->cpu.
+ * 2) now all pcpus are busy;
  *    among all the running vcpus, pick lowest priority one
  *    if snext has higher priority, kick it.
- *
  * TODO:
  * 1) what if these two vcpus belongs to the same domain?
  *    replace a vcpu belonging to the same domain introduces more overhead
@@ -1174,17 +1173,11 @@ runq_tickle(const struct scheduler *ops, struct rt_vcpu 
*new)
     cpumask_and(&not_tickled, online, new->vcpu->cpu_hard_affinity);
     cpumask_andnot(&not_tickled, &not_tickled, &prv->tickled);
 
-    /* 1) if new's previous cpu is idle, kick it for cache benefit */
-    if ( is_idle_vcpu(curr_on_cpu(new->vcpu->processor)) )
-    {
-        SCHED_STAT_CRANK(tickled_idle_cpu);
-        cpu_to_tickle = new->vcpu->processor;
-        goto out;
-    }
-
-    /* 2) if there are any idle pcpu, kick it */
+    /* 1) if there are any idle pcpu, kick it */
     /* The same loop also find the one with lowest priority */
-    for_each_cpu(cpu, &not_tickled)
+       /* For cache benefit, we search new->cpu first */
+    cpu = cpumask_test_or_cycle(new->vcpu->processor, &not_tickled);
+    while ( cpu != nr_cpu_ids )
     {
         iter_vc = curr_on_cpu(cpu);
         if ( is_idle_vcpu(iter_vc) )
@@ -1197,9 +1190,12 @@ runq_tickle(const struct scheduler *ops, struct rt_vcpu 
*new)
         if ( latest_deadline_vcpu == NULL ||
              iter_svc->cur_deadline > latest_deadline_vcpu->cur_deadline )
             latest_deadline_vcpu = iter_svc;
+
+        cpumask_clear_cpu(cpu, &not_tickled);
+        cpu = cpumask_cycle(cpu, &not_tickled);
     }
 
-    /* 3) candicate has higher priority, kick out lowest priority vcpu */
+    /* 2) candicate has higher priority, kick out lowest priority vcpu */
     if ( latest_deadline_vcpu != NULL &&
          new->cur_deadline < latest_deadline_vcpu->cur_deadline )
     {
@@ -1207,7 +1203,6 @@ runq_tickle(const struct scheduler *ops, struct rt_vcpu 
*new)
         cpu_to_tickle = latest_deadline_vcpu->vcpu->processor;
         goto out;
     }
-
     /* didn't tickle any cpu */
     SCHED_STAT_CRANK(tickled_no_cpu);
     return;
-- 
1.9.1


On Fri, 2017-02-24 at 15:54 -0600, Haoran Li wrote:
> From: naroahlee <naroahlee@xxxxxxxxx>
>
> Bug Analysis:
>
Just kill this line above.

> When more than one idle VCPUs that have the same PCPU as their
> previous running core invoke runq_tickle(), they will tickle the same
> PCPU. The tickled PCPU will only pick at most one VCPU, i.e., the
> highest-priority one, to execute. The other VCPUs will not be
> scheduled for a period, even when there is an idle core, making these
> VCPUs unnecessarily starve for one period.
> Therefore, always make sure that we only tickle PCPUs that have not
> been tickled already.
>
And I'd say to wrap around the lines at a shorter threshold. `git log',
for instance, indents the changelogs, and the idea would be for them to
look good on 80 characters terminal.

> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1144,9 +1144,10 @@ rt_vcpu_sleep(const struct scheduler *ops,
> struct vcpu *vc)
>   * Called by wake() and context_saved()
>   * We have a running candidate here, the kick logic is:
>   * Among all the cpus that are within the cpu affinity
> - * 1) if the new->cpu is idle, kick it. This could benefit cache hit
> - * 2) if there are any idle vcpu, kick it.
> - * 3) now all pcpus are busy;
> + * 1) if there are any idle vcpu, kick it.
> + *    For cache benefit, we first search new->cpu.
> + *     
> + * 2) now all pcpus are busy;
>
As Meng said, no blank line here.

>   *    among all the running vcpus, pick lowest priority one
>   *    if snext has higher priority, kick it.
>   *
> @@ -1174,17 +1175,11 @@ runq_tickle(const struct scheduler *ops,
> struct rt_vcpu *new)
>      cpumask_and(&not_tickled, online, new->vcpu->cpu_hard_affinity);
>      cpumask_andnot(&not_tickled, &not_tickled, &prv->tickled);
>  
> -    /* 1) if new's previous cpu is idle, kick it for cache benefit
> */
> -    if ( is_idle_vcpu(curr_on_cpu(new->vcpu->processor)) )
> -    {
> -        SCHED_STAT_CRANK(tickled_idle_cpu);
> -        cpu_to_tickle = new->vcpu->processor;
> -        goto out;
> -    }
> -
> -    /* 2) if there are any idle pcpu, kick it */
> +    /* 1) if there are any idle pcpu, kick it */
>
While there, do you mind adding a full stop at the end of the sentence?

>      /* The same loop also find the one with lowest priority */
> -    for_each_cpu(cpu, &not_tickled)
> +     /* For cache benefit, we search new->cpu first */
>
And this looks to me to be misindented.

If you fix these things and resend, you can add (together to Meng's
one):

Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

And I'm Cc-ing George, so he can also adivse if he wants, as hee is
also a scheduler maintainer... not to mention that he will most likely
be the one that will commit the change, so please do Cc him yourself as
well when you resend the patch (I should have asked to do that before,
but did not notice he was not there).

Thanks and Regards,
Dario


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel