Re: [PATCH v2 1/4] xen: credit2: implement utilization cap

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5183 bytes --]

On Thu, 2017-08-24 at 20:42 +0100, Anshul Makkar wrote:
> On 8/18/17 4:50 PM, Dario Faggioli wrote:
> >   
> > @@ -1515,7 +1633,16 @@ static void reset_credit(const struct
> > scheduler *ops, int cpu, s_time_t now,
> >            * that the credit it has spent so far get accounted.
> >            */
> >           if ( svc->vcpu == curr_on_cpu(svc_cpu) )
> > +        {
> >               burn_credits(rqd, svc, now);
> > +            /*
> > +             * And, similarly, in case it has run out of budget,
> > as a
> > +             * consequence of this round of accounting, we also
> > must inform
> > +             * its pCPU that it's time to park it, and pick up
> > someone else.
> > +             */
> > +            if ( unlikely(svc->budget <= 0) )
> > +                tickle_cpu(svc_cpu, rqd);
> 
> This is for accounting of credit. Why it willl impact the budget. Do
> you 
> intend to refer that
> budget of current vcpu expired while doing calculation for credit ??
>
burn_credits() burns does budget acounting too now. So, it's entirely
possible that the vCPU has actually run out of budget, and we figure it
out now (and we should take appropriate actions!).

> > @@ -1571,27 +1698,35 @@ void burn_credits(struct
> > csched2_runqueue_data *rqd,
> >   
> >       delta = now - svc->start_time;
> >   
> > -    if ( likely(delta > 0) )
> > -    {
> > -        SCHED_STAT_CRANK(burn_credits_t2c);
> > -        t2c_update(rqd, delta, svc);
> > -        svc->start_time = now;
> > -    }
> > -    else if ( delta < 0 )
> > +    if ( unlikely(delta <= 0) )
> >       {
> > 
> > +static void replenish_domain_budget(void* data)
> > +{
> > +    struct csched2_dom *sdom = data;
> > +    unsigned long flags;
> > +    s_time_t now;
> > +    LIST_HEAD(parked);
> > +
> > +    spin_lock_irqsave(&sdom->budget_lock, flags);
> > +
> > +    now = NOW();
> > +
> > +    /*
> > +     * Let's do the replenishment. Note, though, that a domain may
> > overrun,
> > +     * which means the budget would have gone below 0 (reasons may
> > be system
> > +     * overbooking, accounting issues, etc.). It also may happen
> > that we are
> > +     * handling the replenishment (much) later than we should
> > (reasons may
> > +     * again be overbooking, or issues with timers).
> > +     *
> > +     * Even in cases of overrun or delay, however, we expect that
> > in 99% of
> > +     * cases, doing just one replenishment will be good enough for
> > being able
> > +     * to unpark the vCPUs that are waiting for some budget.
> > +     */
> > +    do_replenish(sdom);
> > +
> > +    /*
> > +     * And now, the special cases:
> > +     * 1) if we are late enough to have skipped (at least) one
> > full period,
> > +     * what we must do is doing more replenishments. Note that,
> > however,
> > +     * every time we add tot_budget to the budget, we also move
> > next_repl
> > +     * away by CSCHED2_BDGT_REPL_PERIOD, to make sure the cap is
> > always
> > +     * respected.
> > +     */
> > +    if ( unlikely(sdom->next_repl <= now) )
> > +    {
> > +        do
> > +            do_replenish(sdom);
> > +        while ( sdom->next_repl <= now );
> > +    }
> 
> Just a bit confused. Have you seen this kind of scenario. Please can
> you 
> explain it.
> Is this condition necessary.
>
This was discussed (with George) during v1 review. It's a corner case,
which should never happen, and I in fact have never seen it happening
in my tests.

But we can't rule out that it won't occur, so it makes sense to deal
with it (instead of just ignoring it, causing the cap mechanism to
[temporarily] malfunction / become inaccurate).

> > +    /*
> > +     * 2) if we overrun by more than tot_budget, then
> > budget+tot_budget is
> > +     * still < 0, which means that we can't unpark the vCPUs.
> > Let's bail,
> > +     * and wait for future replenishments.
> > +     */
> > +    if ( unlikely(sdom->budget <= 0) )
> > +    {
> > +        spin_unlock_irqrestore(&sdom->budget_lock, flags);
> > +        goto out;
> > +    }
> 
> "if we overran by more than tot_budget in previous run", make is
> more 
> clear..
>
Mmm... perhaps, but not so much, IMO. It's quite clear to which time
window we are referring to already, and I don't feel like re-sending
for this.

Let's see if there are other comments/requests.

> Rest, looks good to me.
> 
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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