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

On 15.06.26 11:59, Philippe Gerum wrote:
> 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.
> 

Makes sense, now.

> 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!
> 

Tests aren't perfect either: sched-quota was "tested" before, and my
current proposal apparently missed the corner-cases we just discussed above.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center