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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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
-- 
<<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: signature.asc
Description: This is a digitally signed message part

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