Re: [Xen-devel] [PATCH v2 04/10] xen: credit2: only reset credit on reset condition

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-09-30 at 13:25 +0100, anshul makkar wrote:
> On 30/09/16 03:53, Dario Faggioli wrote:
> > @@ -2336,6 +2345,7 @@ csched2_schedule(
> >      struct csched2_runqueue_data *rqd;
> >      struct csched2_vcpu * const scurr = CSCHED2_VCPU(current);
> >      struct csched2_vcpu *snext = NULL;
> > +    unsigned int skipped_vcpus = 0;
> >      struct task_slice ret;
> > 
> >      SCHED_STAT_CRANK(schedule);
> > @@ -2385,7 +2395,7 @@ csched2_schedule(
> >          snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> >      }
> >      else
> > -        snext = runq_candidate(rqd, scurr, cpu, now);
> > +        snext = runq_candidate(rqd, scurr, cpu, now,
> > &skipped_vcpus);
> > 
> >      /* If switching from a non-idle runnable vcpu, put it
> >       * back on the runqueue. */
> > @@ -2409,8 +2419,21 @@ csched2_schedule(
> >              __set_bit(__CSFLAG_scheduled, &snext->flags);
> >          }
> > 
> > -        /* Check for the reset condition */
> > -        if ( snext->credit <= CSCHED2_CREDIT_RESET )
> > +        /*
> > +         * The reset condition is "has a scheduler epoch come to
> > an end?".
> > +         * The way this is enforced is checking whether the vcpu
> > at the top
> > +         * of the runqueue has negative credits. This means the
> > epochs have
> > +         * variable lenght, as in one epoch expores when:
> > +         *  1) the vcpu at the top of the runqueue has executed
> > for
> > +         *     around 10 ms (with default parameters);
> > +         *  2) no other vcpu with higher credits wants to run.
> > +         *
> > +         * Here, where we want to check for reset, we need to make
> > sure the
> > +         * proper vcpu is being used. In fact,
> > runqueue_candidate() may have
> > +         * not returned the first vcpu in the runqueue, for
> > various reasons
> > +         * (e.g., affinity). Only trigger a reset when it does.
> > +         */
> > +        if ( skipped_vcpus == 0 && snext->credit <=
> > CSCHED2_CREDIT_RESET )
> >          {
> Is there no other way of checking that the returned vcpu from 
> runqueue_candidate is the first one apart from using "skipped_vcpu" 
> extra variable. Looks a bit ugly.
>
Well, I guess we can try to use list_first_entry() on the runqueue, and
compare it with snext.

However, that "leaks" the bit of information that the runqueue is
implemented as a list, and would break if at some point we decide to
use something different. TBF, this is not a big problem, as we're
inside the scheduler implementation anyway (so not really too big of a
leak), and we'd most likely notice the breakage at build time... but
still.

More important, I don't think that "just" doing that would work. In
fact, if scurr is running, and there is no-one in the runqueue, and
scurr's credits are negative, right now, we reset. So, to catch that
case the condition would need to become something like this:

 if ( (list_empty(&RQD(ops, cpu)->runq) ||
       list_first_entry(&RQD(ops, cpu)->runq) == snext) &&
      snext->credit <= CSCHED2_CREDIT_RESET )

I agree that counting is a bit ugly, but this one here above does not
look much prettier to me.

Also, the number of skipped vcpus, if reported via tracing, does convey
some information on the status of the runqueue. Nothing crucial, but
something that could be useful (at least, it was to me, during
developing and debugging).

So, for these reasons, I'd be inclined to leave this as it is.

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