Re: [Xen-devel] [PATCH v4] xen: rtds: only tickle non-already tickled CPUs

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Tue, 2017-08-01 at 15:24 -0400, Meng Xu wrote:
> The initial discussion of this patch can be found at
> https://lists.xenproject.org/archives/html/xen-devel/2017-02/msg02857
> .html
> 
> Changes in v4:
> 1) Take Dario's suggestions:
>    Search the new->cpu first for the cpu to tickle.
>    This get rid of the if statement in previous versions.
> 2) Reword the comments and commit messages.
> 3) Rebased on staging branch.
> 
> Issues in v2 and v3:
> Did not rebase on the latest staging branch.
> Did not solve the comments/issues in v1.
> Please ignore the v2 and v3.
>
Ok, thanks for taking care of this.

I've only have two more minor comments.

> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 39f6bee..5fec95f 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -1147,9 +1147,9 @@ 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.
>
Either:

"if there is an ile CPU, kick it."

Or

"if there are any idle CPUs, kick one."

Feel both more accurate (it's a CPU that is idle, not a vCPU, although,
yes, vcpus of the idle domain do exist, I know), and better in English.

This applies to both here and below, where this line is repeated.

> +      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.
>   *
> @@ -1177,17 +1177,13 @@ 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 */
> -    /* The same loop also find the one with lowest priority */
> -    for_each_cpu(cpu, &not_tickled)
> +    /*
> +     * 1) If there are any idle vcpu, kick it.
> +     *    For cache benefit,we first search new->cpu.
>
"we check new->cpu for first" (or "as first", I think they both can be
used but I'm no native speaker).

With these two adjustments, you can have my:

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

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