Re: [PATCH 1/2] evl/sched: Add sched_out handler to sched_class

Jan Kiszka <[email protected]> writes:

> On 15.06.26 08:42, Philippe Gerum wrote:
>> Philippe Gerum <[email protected]> writes:
>> 
>
> You also need to update tg->run_start. Otherwise, you will overcharge
> the next time this group is checked here.
>

Yes, this needs fixing.

>> +	} else {
>> +		tg->run_budget = 0;
>> +		evl_stop_timer(&qs->limit_timer);
>> +	}
>> +}
>> +
>> +static struct evl_thread *quota_pick(struct evl_rq *rq)
>> +{
>> +	struct evl_thread *next, *curr = rq->curr;
>> +	struct evl_sched_quota *qs = &rq->quota;
>> +	struct evl_quota_group *tg;
>> +
>>  pick:
>> -	next = evl_get_schedq(&rq->fifo.runnable);
>> +	next = evl_get_schedq(&qs->runnable);
>>  	if (next == NULL) {
>>  		evl_stop_timer(&qs->limit_timer);
>>  		return NULL;
>>  	}
>>  
>> -	/*
>> -	 * As we basically piggyback on the SCHED_FIFO runqueue, make
>> -	 * sure to detect non-quota threads.
>> -	 */
>>  	tg = next->quota;
>> -	if (tg == NULL)
>> -		return next;
>> -
>> -	tg->run_start = now;
>> +	tg->nr_active--;
>>  
>>  	/*
>>  	 * Don't consider budget if kicked, we have to allow this
>> @@ -439,25 +437,29 @@ static struct evl_thread *quota_pick(struct evl_rq *rq)
>>  	 */
>>  	if (next->info & EVL_T_KICKED) {
>>  		evl_stop_timer(&qs->limit_timer);
>> -		goto out;
>> +		return next;
>>  	}
>>  
>> +	/*
>> +	 * __pick_next_thread() might have requeued the current
>> +	 * thread which is still leading the pack.
>> +	 */
>> +	if (curr == next)
>> +		return next;
>> +
>>  	if (ktime_to_ns(tg->run_budget) == 0) {
>> -		/* Flush expired group members as we go. */
>> +		/* Park expired group members as we go. */
>>  		list_add_tail(&next->quota_expired, &tg->expired);
>>  		goto pick;
>>  	}
>>  
>> -	if (otg == tg && evl_timer_is_running(&qs->limit_timer))
>> -		/* Same group, leave the running timer untouched. */
>> -		goto out;
>> -
>> -	/* Arm limit timer for the new running group. */
>> -	evl_start_timer(&qs->limit_timer,
>> -			ktime_add(now, tg->run_budget),
>> -			EVL_INFINITE);
>> -out:
>> -	tg->nr_active--;
>> +	/* Arm new limit timer on change of running group. */
>> +	if (curr->quota != tg || !evl_timer_is_running(&qs->limit_timer)) {
>> +		tg->run_start = evl_ktime_monotonic();
>> +		evl_start_timer(&qs->limit_timer,
>> +				ktime_add(tg->run_start, tg->run_budget),
>> +				EVL_INFINITE);
>> +	}
>
> I still don't fully get how the limit timer should be started on
> setparam over the current thread. Will curr be NULL in that case, thus
> curr != next? Is that the reason to go for a separate runqueue?
>

Nope, that one is only to make things clearer, since there is no upside
in sharing the runqueues (this was inherited from SCHED_SPORADIC for no
obvious reason with hindsight). At some point, I noticed that it would
even have introduced a bug in a previous tentative implementation using
sched_class as a discriminator between threads, to figure out whether we
should call the sched_out() handler. So, the simpler, the better:
distinct classes, distinct runqueues.

Regarding the limit timer issue, sched_pick() should not shortcut on
curr == next. IOW, the rest of the code should run even in this case. By
turning on the resched bit (which the quota_setparam() caller does), we
would end up in sched_pick() with either of those outcomes out of
quota_setparam():

- 'current' switched group, in which case we would arm a limit timer in
  sched_pick() because curr->quota != next->quota (provided the shortcut
  on curr == next is gone).

- 'current' stays in the same group, so the limit timer still runs unless
  the budget was already fully consumed.

So I would revise my latest patch as follows:

diff --git a/kernel/evl/sched/quota.c b/kernel/evl/sched/quota.c
index 3ac650c08ccb..8f1321d46cac 100644
--- a/kernel/evl/sched/quota.c
+++ b/kernel/evl/sched/quota.c
@@ -407,6 +407,7 @@ static void quota_out(struct evl_thread *thread)
 	now = evl_ktime_monotonic();
 	consumed = ktime_sub(now, tg->run_start);
 	if (consumed < tg->run_budget) {
+		tg->run_start = now;
 		tg->run_budget = ktime_sub(tg->run_budget, consumed);
 	} else {
 		tg->run_budget = 0;
@@ -440,20 +441,13 @@ static struct evl_thread *quota_pick(struct evl_rq *rq)
 		return next;
 	}
 
-	/*
-	 * __pick_next_thread() might have requeued the current
-	 * thread which is still leading the pack.
-	 */
-	if (curr == next)
-		return next;
-
 	if (ktime_to_ns(tg->run_budget) == 0) {
 		/* Park expired group members as we go. */
 		list_add_tail(&next->quota_expired, &tg->expired);
 		goto pick;
 	}
 
-	/* Arm new limit timer on change of running group. */
+	/* Arm new limit timer if need be. */
 	if (curr->quota != tg || !evl_timer_is_running(&qs->limit_timer)) {
 		tg->run_start = evl_ktime_monotonic();
 		evl_start_timer(&qs->limit_timer,

>
> Looks ok otherwise, tests pass as well.
>

Uh? Ok. That was absolutely unintentional at this stage!

-- 
Philippe.