[RTDS Patch v2 for Xen4.8] xen: rtds: only tickle non-already tickled CPUs

[RTDS Patch v2 for Xen4.8] xen: rtds: only tickle non-already tickled CPUs

[Haoran Li]

From: naroahlee <naroahlee@gmail.com>

Bug Analysis:
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.
---
 xen/common/sched_rt.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 1b30014..012975c 100644
--- 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;
  *    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 */
     /* 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 +1192,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 )
     {
-- 
1.9.1

---
CC: <mengxu@cis.upenn.edu>
CC: <dario.faggioli@citrix.com>

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

Re: [RTDS Patch v2 for Xen4.8] xen: rtds: only tickle non-already tickled CPUs

[Meng Xu]

On Fri, Feb 24, 2017 at 4:54 PM, Haoran Li <naroahlee@gmail.com> wrote:
> From: naroahlee <naroahlee@gmail.com>
>
> Bug Analysis:
> 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.
> ---
>  xen/common/sched_rt.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 1b30014..012975c 100644
> --- 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.
> + *

Please ditch this empty line.

> + * 2) now all pcpus are busy;
>   *    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 */
>      /* 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 +1192,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 )
>      {
> --
> 1.9.1
>
> ---
> CC: <mengxu@cis.upenn.edu>
> CC: <dario.faggioli@citrix.com>

The code looks good to me.

The empty line in the comment may not be a bit deal.

Reviewed-by: Meng Xu <mengxu@cis.upenn.edu>

Thanks,

Meng
-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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

Re: [RTDS Patch v2 for Xen4.8] xen: rtds: only tickle non-already tickled CPUs

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3356 bytes --]

On Fri, 2017-02-24 at 15:54 -0600, Haoran Li wrote:
> From: naroahlee <naroahlee@gmail.com>
> 
> 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@citrix.com>

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
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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