Re: [Xen-devel] [RTDS Patch for Xen4.8] xen: sched_rt.c Check not_tickled Mask

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Wed, 2017-02-22 at 17:59 -0500, Meng Xu wrote:
> Hi Haoran,
> 
> Thank you for sending this patch out quickly! :-)
> 
> The title can be
> [PATCH] xen: rtds: only tickle the same cpu once
> 
Or:
xen: rtds: only tickle non-already tickled CPUs

(Nitpicking, I know, but I don't like how "the same" sounds in there.)

> On Wed, Feb 22, 2017 at 5:16 PM, Haoran Li <naroahlee@xxxxxxxxx>
> wrote:
> > 
> > Bug Analysis:
> > We need to exclude tickled VCPUs when trying to evaluate
> > runq_tickle() case 1
> 
> Change the description to the following:
> 
> 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.
> 
Agreed.

> To fix this issue, we should always tickle the non-tickled PCPU in
> the
> runq_tickle().
> 
I'd change this sentence in something like:

"Therefore, always make sure that we only tickle PCPUs that have not
been tickled already."

> > --- a/xen/common/sched_rt.c
> > +++ b/xen/common/sched_rt.c
> > @@ -1175,7 +1175,8 @@ runq_tickle(const struct scheduler *ops,
> > struct rt_vcpu *new)
> >      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)) )
> > +    if ( is_idle_vcpu(curr_on_cpu(new->vcpu->processor)) &&
> > +         cpumask_test_cpu(new->vcpu->processor, &not_tickled))
> 
> You should have a space before the last ).
> 
Indeed.

But it looks to me that we can take the chance to tweak the code a
little bit, get rid of the special casing, and by that making it more
compact (and hence easier to read), and maybe a tiny bit more efficient
too.

I'm thinking about getting rid entirely of the 'if' above, and then
transforming the loop into something like this:

    cpu = cpumask_test_or_cycle(new->vcpu->processor,
                                &not_tickled);
    while ( cpu != nr_cpu_ids )
    {
        iter_vc = curr_on_cpu(cpu);
        /* ... existing loop body ... */
        cpumask_clear_cpu(cpu, &not_tickle);
        cpu = cpumask_cycle(cpu, &not_tickled);
    }

(I do thinks this is correct, but please, do double check.)

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