Re: [PATCH] credi2-ratelimit: Implement rate limit for credit2 scheduler

[anshul makkar <anshul.makkar@citrix.com>]

On 13/07/16 09:53, Dario Faggioli wrote:
> On Tue, 2016-07-12 at 17:16 +0100, George Dunlap wrote:
>> On Wed, Jul 6, 2016 at 6:33 PM, Makkar anshul.makkar@citrix.com
>> <Anshul> wrote:
>>>
>>> --- a/xen/common/sched_credit2.c
>>> +++ b/xen/common/sched_credit2.c
>>> +#define MAX_TSLICE(t1, t2)  \
>>> +                   ({ typeof (t1) _t1 = (t1); \
>>> +                      typeof (t1) _t2 = (t2); \
>>> +                      _t1 > _t2 ? _t1 < 0 ? 0 : _t1 : _t2 < 0 ? 0
>>> : _t2; })
>> Hiding the zero-checking behind this seems a bit strange to me.  I
>> think if the algorithm is properly laid out, we probably don't need
>> this at all (see my proposed alternate below).
>>
> I think it's more "overflow-avoidance" than "zero-checking", and as I
> said, that's probably better done by means of subtraction(s).
>
> In any case, I agree that, if we don't need it, that's even better. :-)
Agreed. I am going to remove it. Its not needed with modified algo.
>

>> This is all really confusing.  What about something like this (also
>> attached)?  The basic idea is to calculate min_time, then go through
>> the normal algorithm, then clip it once at the end.
>>
> Yes, this matches my idea and my comment, and I think the code provided
> by George makes sense. Only one thing:
>
>> @@ -1675,9 +1711,19 @@ csched2_runtime(const struct scheduler *ops,
>> int cpu, struct csched2_vcpu *snext
>> +     * or your the ratelimit time.
>>        */
>> +    /* Calculate mintime */
>> +    min_time = CSCHED2_MIN_TIMER;
>> +    if ( prv->ratelimit_us ) {
>> +        s_time_t ratelimit_min = snext->vcpu-
>>> runstate.state_entry_time +
>> +            MICROSECS(prv->ratelimit_us) - now;
>>
> Here snext can indeed be someone which was running already (e.g., we're
> choosing current again), in which case runstate.state_entry-time-now
> would indeed tell us how long it's actually been running, and the
> formula (coupled with the if below) is correct.
>
> But it also can be someone which is runnable (e.g., we're choosing
> someone from the runqueue and preempting current), in which case
> runstate.state_entry_time tells when it became runnable, and
> state_entry_time-now is how long it's been runnable, which is not what
> we want here.
>
> In think, in such a case, we want ratelimit_min to just be equal to
> prv->ratelimit_us. So, maybe, something like this:
>
>   /* Caluclate mintime */
>   min_time = CSCHED2_MIN_TIMER;
>   if ( prv->ratelimit_us )
>   {
>       s_time_t ratelimit_min = prv->ratelimit_us;
>       if ( snext->vcpu->is_running )     // XXX or is it better snext == curr_on_cpu(cpu)
>           ratelimit_min = snext->vcpu->runstate.state_entry_time +
>                           MICROSECS(prv->ratelimit_us) - now;
>       if ( ratelimit_min > min_time )
>           min_time = ratelimit_min;
>   }
>
Agreed.
>>> @@ -2353,6 +2431,8 @@ csched2_init(struct scheduler *ops)
>>>           prv->runq_map[i] = -1;
>>>           prv->rqd[i].id = -1;
>>>       }
>>> +    /* initialize ratelimit */
>>> +    prv->ratelimit_us = 1000 * CSCHED2_MIN_TIMER;
>> Is there any reason this isn't using sched_ratelimit_us (the
>> hypervisor command-line option, which the credit scheduler is using)?
>>
> Yeah, I guess just using that is the best thing to start with.
Agreed.
>
> Regards,
> Dario
>
Will post the reworked patch.

Anshul


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