| Message ID | 1487973277-20854-1-git-send-email-naroahlee@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(¬_tickled, online, new->vcpu->cpu_hard_affinity); > cpumask_andnot(¬_tickled, ¬_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, ¬_tickled) > + /* For cache benefit, we search new->cpu first */ > + cpu = cpumask_test_or_cycle(new->vcpu->processor, ¬_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, ¬_tickled); > + cpu = cpumask_cycle(cpu, ¬_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/
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(¬_tickled, online, new->vcpu->cpu_hard_affinity); > cpumask_andnot(¬_tickled, ¬_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, ¬_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
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(¬_tickled, online, new->vcpu->cpu_hard_affinity); cpumask_andnot(¬_tickled, ¬_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, ¬_tickled) + /* For cache benefit, we search new->cpu first */ + cpu = cpumask_test_or_cycle(new->vcpu->processor, ¬_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, ¬_tickled); + cpu = cpumask_cycle(cpu, ¬_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 ) {
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(-)