On 17/08/16 18:18, Dario Faggioli wrote:
> In both Credit1 and Credit2, if a vcpu yields, let it...
> well... yield!
>
> In fact, context switch rate limiting has been primarily
> introduced to avoid too heavy context switch rate due to
> interrupts, and, in general, asynchronous events.
>
> In a vcpu "voluntarily" yields, we really should let it
> give up the cpu for a while. For instance, the reason may
> be that it's about to start spinning, and there's few
> point in forcing a vcpu to spin for (potentially) the
> entire rate-limiting period.
>
> Signed-off-by: Dario Faggioli
> ---
> Cc: George Dunlap
> Cc: Anshul Makkar
> ---
> xen/common/sched_credit.c | 20 +++
> xen/common/sched_credit2.c | 47
> +++-
> 2 files changed, 37 insertions(+), 30 deletions(-)
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 3f439a0..ca04732 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1771,9 +1771,18 @@ csched_schedule(
> * cpu and steal it.
> */
>
> -/* If we have schedule rate limiting enabled, check to see
> - * how long we've run for. */
> -if ( !tasklet_work_scheduled
> +/*
> + * If we have schedule rate limiting enabled, check to see
> + * how long we've run for.
> + *
> + * If scurr is yielding, however, we don't let rate limiting kick in.
> + * In fact, it may be the case that scurr is about to spin, and there's
> + * no point forcing it to do so until rate limiting expires.
> + *
> + * While there, take the chance for clearing the yield flag at once.
> + */
> +if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_YIELD, >flags)
It looks like YIELD is implemented by putting it lower in the runqueue
in __runqueue_insert(). But here you're clearing the flag before the
insert happens -- won't this effectively disable yield() for credit1?
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 569174b..c8e0ee7 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -2267,36 +2267,40 @@ runq_candidate(struct csched2_runqueue_data *rqd,
> struct list_head *iter;
> struct csched2_vcpu *snext = NULL;
> struct csched2_private *prv = CSCHED2_PRIV(per_cpu(scheduler, cpu));
> -int yield_bias = 0;
> -
> -/* Default to current if runnable, idle otherwise */
> -if ( vcpu_runnable(scurr->vcpu) )
> -{
> -/*
> - * The way we actually take yields into account is like this:
> - * if scurr is yielding, when comparing its credits with other
> - * vcpus in the runqueue, act like those other vcpus had yield_bias
> - * more credits.
> - */
> -if ( unlikely(scurr->flags & CSFLAG_vcpu_yield) )
> -yield_bias = CSCHED2_YIELD_BIAS;
> -
> -snext = scurr;
> -}
> -else
> -snext = CSCHED2_VCPU(idle_vcpu[cpu]);
> +/*
> + * The way we actually take yields into account is like this:
> + * if scurr is yielding, when comparing its credits with other vcpus in
> + * the runqueue, act like those other vcpus had yield_bias more credits.
> + */
> +int yield_bias = __test_and_clear_bit(__CSFLAG_vcpu_yield,
> >flags) ?
> + CSCHED2_YIELD_BIAS : 0;
>
> /*
> * Return the current vcpu if it has executed for less than ratelimit.
> * Adjuststment for the selected vcpu's credit and decision
> * for how long it will run will be taken in csched2_runtime.
> + *
> + * Note that, if scurr is yielding, we don't let rate limiting kick in.
> + * In fact, it may be the case that scurr is about to spin, and there's
> + * no point forcing it to do so until rate limiting expires.
> + *
> + * To check whether we are yielding, it's enough to look at yield_bias
> + * (as CSCHED2_YIELD_BIAS can't be zero). Also, note that the yield flag
> + * has been cleared already above.
> */
> -if ( prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
> +if ( !yield_bias &&
> + prv->ratelimit_us && !is_idle_vcpu(scurr->vcpu) &&
> vcpu_runnable(scurr->vcpu) &&
> (now - scurr->vcpu->runstate.state_entry_time) <
>MICROSECS(prv->ratelimit_us) )
> return scurr;
>
> +/* Default to current if runnable, idle otherwise */
> +if ( vcpu_runnable(scurr->vcpu) )
> +snext = scurr;
> +else
> +snext = CSCHED2_VCPU(idle_vcpu[cpu]);
This looks good, but the code re-organization probably goes better in
the previous patch. Since you're re-sending anyway, would you mind
moving it there?
I'm not sure the credit2 yield-ratelimit needs to be in a separate
patch; since you're implementing yield in credit2 from scratch you could
just implement it all in one go. But since you have