Re: [Xen-devel] [PATCH -v3 1/1] ratelimit: Implement rate limit for credit2 scheduler Rate limit assures that a vcpu will execute for a minimum amount of time before being put at the back of a queue or being preempted by higher priority thread.

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Mon, 2016-07-25 at 17:12 +0100, Anshul Makkar wrote:
> It introduces context-switch rate-limiting. The patch enables the VM
> to batch
> its work and prevents the system from spending most of its time in
> context
> switches because of a VM that is waking/sleeping at high rate.
> 
> ratelimit can be disabled by setting it to 0.
> 
Thanks Anshul. I've looked at the patch, and it seemed all right to me.

I decided to build and test it, and I've seem something that I found
weird.

But first of all, one thing that I'm sorry I'm mentioning now
(especially considering it was quite evident! :-/).

The subject, which will become the "title" of the commit, is way too
long. That must be a very concise headline of what the patch is about,
and should also have tags, specifying what areas of the codebase are
affected. So what do you think of this:

  xen: credit2: implement context switch rate-limiting.

On a more technical side, I think that...

> @@ -2116,9 +2147,22 @@ csched2_runtime(const struct scheduler *ops,
> int cpu, struct csched2_vcpu *snext
>       * 1) Run until snext's credit will be 0
>       * 2) But if someone is waiting, run until snext's credit is
> equal
>       * to his
> -     * 3) But never run longer than MAX_TIMER or shorter than
> MIN_TIMER.
> +     * 3) But never run longer than MAX_TIMER or shorter than
> MIN_TIMER or
> +     * the ratelimit time.
>       */
>  
> +    /* Calculate mintime */
> +    min_time = CSCHED2_MIN_TIMER;
> +    if ( prv->ratelimit_us )
> +    {
> +        s_time_t ratelimit_min = prv->ratelimit_us;
>
... this should be:

 s_time_t ratelimit_min = MICROSECS(prv->ratelimit_us);

I realized that by looking at traces and seeing entries for which
CSCHED2_MIN_TIMER was being returned, even if I had set
sched_ratelimit_us to a value greater than that.

I think the rest of the code is ok, and this is the only issue... but
I'll make the change above here locally and continue my testing, and
let you know if I find anything else.

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