Re: [Xen-devel] [PATCH 05/24] xen: credit2: make tickling more deterministic

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Tue, 2016-09-13 at 12:13 +0100, George Dunlap wrote:
> On 05/09/16 14:47, Dario Faggioli wrote:
> > On Wed, 2016-08-31 at 18:10 +0100, anshul makkar wrote:
> > > > @@ -1273,6 +1280,7 @@ csched2_alloc_vdata(const struct
> > > > scheduler
> > > > *ops, struct vcpu *vc, void *dd)
> > > >       else
> > > >       {
> > > >           ASSERT(svc->sdom == NULL);
> > > > +        svc->tickled_cpu = svc->vcpu->vcpu_id;
> > > If I understood correctly, tickled_cpu refers to pcpu and not a
> > > vcpu. 
> > > Saving vcpu_id in tickled_cpu looks wrong.
> > > 
> > Yes, and in fact, as you can see in the previous hunk, for pretty
> > much
> > all vcpus, tickled_cpu is initialized to -1.
> > 
> > Here, we are dealing with the vcpus of the idle domain. And for
> > vcpus
> > of the idle domain, their vcpu id is the same as the id of the pcpu
> > they're associated to.
> 
> But what I haven't sussed out yet is why we need to initialize this
> for
> the idle domain at all.  What benefit does it give you, and what
> effect
> does it have?
> 
It makes things more consistent and uniform, one effect being that it
is easier to manage a performance counter, for recording the number of
time this 'direct tickling' mechanism has been overridden.

In fact, I've tried it and, AFAICR, doing this was looking worse, when
I was not initializing the field for idle vcpus.

static struct csched2_vcpu *
runq_candidate(struct csched2_runqueue_data *rqd,
               struct csched2_vcpu *scurr,
               int cpu, s_time_t now,
               unsigned int *pos)
{
    ...
    [1]
    if ( unlikely(snext->tickled_cpu != -1 && snext->tickled_cpu != cpu) )
        SCHED_STAT_CRANK(tickled_cpu_overridden);

    return snext;
}

In fact, it is possible to reach [1] with snext being the idle vcpu, in
which case, if tickled_cpu is 0 for all of them (which would be the
case, I think, if I don't init it) the counter will be incremented in a
not really predictable way.

That being said, initializing to -1 should work, and it's easier to
read and understand (as I won't be special casing idle vcpus). So I'd
go for it (and test it, of course).. what do you think?

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