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

[George Dunlap]

On 09/14/2017 05:20 PM, George Dunlap wrote:
> On 08/18/2017 04:50 PM, Dario Faggioli wrote:
>> This commit implements the Xen part of the cap mechanism for
>> Credit2.
>>
>> A cap is how much, in terms of % of physical CPU time, a domain
>> can execute at most.
>>
>> For instance, a domain that must not use more than 1/4 of
>> one physical CPU, must have a cap of 25%; one that must not
>> use more than 1+1/2 of physical CPU time, must be given a cap
>> of 150%.
>>
>> Caps are per domain, so it is all a domain's vCPUs, cumulatively,
>> that will be forced to execute no more than the decided amount.
>>
>> This is implemented by giving each domain a 'budget', and
>> using a (per-domain again) periodic timer. Values of budget
>> and 'period' are chosen so that budget/period is equal to the
>> cap itself.
>>
>> Budget is burned by the domain's vCPUs, in a similar way to
>> how credits are.
>>
>> When a domain runs out of budget, its vCPUs can't run any
>> longer. They can gain, when the budget is replenishment by
>> the timer, which event happens once every period.
>>
>> Blocking the vCPUs because of lack of budget happens by
>> means of a new (_VPF_parked) pause flag, so that, e.g.,
>> vcpu_runnable() still works. This is similar to what is
>> done in sched_rtds.c, as opposed to what happens in
>> sched_credit.c, where vcpu_pause() and vcpu_unpause()
>> (which means, among other things, more overhead).
>>
>> Note that, while adding new fields to csched2_vcpu and
>> csched2_dom, currently existing members are being moved
>> around, to achieve best placement inside cache lines.
>>
>> Note also that xenalyze and tools/xentrace/format are being
>> updated too.
>>
>> Signed-off-by: Dario Faggioli 
> 
> Looks good, with one minor nit...
> 
> 
>> +/*
>> + * NB: we give the whole remaining budget a domain has, to the first
>> + * vCPU that comes here and asks for it. This means that, in a 
>> domain
>> + * with a cap, only 1 vCPU is able to run, at any given time.
>> + * /THIS IS GOING TO CHANGE/ in subsequent patches, toward something
>> + * that allows much better fairness and parallelism. Proceeding in
>> + * two steps, is for making things easy to understand, when looking
>> + * at the signle commits.
> 
> *single
> 
> But I can fix that up on check-in.

Well turns out it gets clobbered in the 3rd patch anyway.  But at least
we a well-spelled commit history. :-)

 -George

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

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

[George Dunlap]

On 08/18/2017 04:50 PM, Dario Faggioli wrote:
> This commit implements the Xen part of the cap mechanism for
> Credit2.
> 
> A cap is how much, in terms of % of physical CPU time, a domain
> can execute at most.
> 
> For instance, a domain that must not use more than 1/4 of
> one physical CPU, must have a cap of 25%; one that must not
> use more than 1+1/2 of physical CPU time, must be given a cap
> of 150%.
> 
> Caps are per domain, so it is all a domain's vCPUs, cumulatively,
> that will be forced to execute no more than the decided amount.
> 
> This is implemented by giving each domain a 'budget', and
> using a (per-domain again) periodic timer. Values of budget
> and 'period' are chosen so that budget/period is equal to the
> cap itself.
> 
> Budget is burned by the domain's vCPUs, in a similar way to
> how credits are.
> 
> When a domain runs out of budget, its vCPUs can't run any
> longer. They can gain, when the budget is replenishment by
> the timer, which event happens once every period.
> 
> Blocking the vCPUs because of lack of budget happens by
> means of a new (_VPF_parked) pause flag, so that, e.g.,
> vcpu_runnable() still works. This is similar to what is
> done in sched_rtds.c, as opposed to what happens in
> sched_credit.c, where vcpu_pause() and vcpu_unpause()
> (which means, among other things, more overhead).
> 
> Note that, while adding new fields to csched2_vcpu and
> csched2_dom, currently existing members are being moved
> around, to achieve best placement inside cache lines.
> 
> Note also that xenalyze and tools/xentrace/format are being
> updated too.
> 
> Signed-off-by: Dario Faggioli 

Looks good, with one minor nit...


> +/*
> + * NB: we give the whole remaining budget a domain has, to the first
> + * vCPU that comes here and asks for it. This means that, in a domain
> + * with a cap, only 1 vCPU is able to run, at any given time.
> + * /THIS IS GOING TO CHANGE/ in subsequent patches, toward something
> + * that allows much better fairness and parallelism. Proceeding in
> + * two steps, is for making things easy to understand, when looking
> + * at the signle commits.

*single

But I can fix that up on check-in.

Reviewed-by: George Dunlap 

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

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

[Dario Faggioli]

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(>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(>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
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)

signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Anshul Makkar]

On 8/18/17 4:50 PM, Dario Faggioli wrote:
  
@@ -474,6 +586,12 @@ static inline struct csched2_runqueue_data *c2rqd(const struct scheduler *ops,

  return _priv(ops)->rqd[c2r(cpu)];
  }
  
+/* Does the domain of this vCPU have a cap? */

+static inline bool has_cap(const struct csched2_vcpu *svc)
+{
+return svc->budget != STIME_MAX;
+}
+
  /*
   * Hyperthreading (SMT) support.
   *
@@ -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 ??

+}
  
  start_credit = svc->credit;
  
@@ -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(>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.

+/*
+ * 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(>budget_lock, flags);
+goto out;
+}
"if we overran by more than tot_budget in previous run", make is more 
clear..

+
+/* Since we do more replenishments, make sure we didn't overshot. */
+sdom->budget = min(sdom->budget, sdom->tot_budget);
+
+/*
+ * As above, let's prepare the temporary list, out of the domain's
+ * parked_vcpus list, now that we hold the budget_lock. Then, drop such
+ * lock, and pass the list to the unparking function.
+ */
+list_splice_init(>parked_vcpus, );
+
+spin_unlock_irqrestore(>budget_lock, flags);
+
+unpark_parked_vcpus(sdom->dom->cpupool->sched, );
+
+ out:
+set_timer(sdom->repl_timer, sdom->next_repl);
+}
+
  #ifndef NDEBUG
  static inline void
  csched2_vcpu_check(struct vcpu *vc)
@@ -1658,6 +2035,9 @@ csched2_alloc_vdata(const struct scheduler *ops, struct 
vcpu *vc, void *dd)
  }
  svc->tickled_cpu = -1;
  
+


Rest, looks good to me.

Thanks
Anshul





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