Re: [PATCH v1 1/2] evl/sched: core: introduce schedule out handler

On 18.06.26 09:44, Philippe Gerum wrote:
> Jan Kiszka <[email protected]> writes:
> 
>> On 17.06.26 16:20, Philippe Gerum wrote:
>>> From: Philippe Gerum <[email protected]>
>>>
>>> Some scheduling classes may need a way to perform specific
>>> fixup/accounting work for a thread which is being scheduled out.  We
>>> cannot rely on the sched_pick() handler for this since the first one
>>> to successfully pick a thread prevents other handlers down the class
>>> hierarchy to run.
>>>
>>> To address this issue, we introduce the sched_out() handler which is
>>> called for the current thread when scheduled out. This handler is
>>> invoked unconditionally when present prior to picking a new thread.
>>>
>>> Signed-off-by: Philippe Gerum <[email protected]>
>>> ---
>>>  include/evl/sched.h     |  1 +
>>>  kernel/evl/sched/core.c | 20 +++++++++++++-------
>>>  2 files changed, 14 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/evl/sched.h b/include/evl/sched.h
>>> index ae9690860146..cc824c28004b 100644
>>> --- a/include/evl/sched.h
>>> +++ b/include/evl/sched.h
>>> @@ -120,6 +120,7 @@ struct evl_sched_class {
>>>  	void (*sched_dequeue)(struct evl_thread *thread);
>>>  	void (*sched_requeue)(struct evl_thread *thread);
>>>  	struct evl_thread *(*sched_pick)(struct evl_rq *rq);
>>> +	void (*sched_out)(struct evl_thread *thread);
>>>  	void (*sched_yield)(struct evl_thread *thread);
>>>  	void (*sched_migrate)(struct evl_thread *thread,
>>>  			      struct evl_rq *rq);
>>> diff --git a/kernel/evl/sched/core.c b/kernel/evl/sched/core.c
>>> index d1d025e06a5d..7127d4e52df9 100644
>>> --- a/kernel/evl/sched/core.c
>>> +++ b/kernel/evl/sched/core.c
>>> @@ -773,7 +773,8 @@ static inline void set_next_running(struct evl_rq *rq,
>>>  		evl_stop_timer(&rq->rrbtimer);
>>>  }
>>>  
>>> -static struct evl_thread *__pick_next_thread(struct evl_rq *rq)
>>> +static __always_inline struct evl_thread *
>>> +__pick_next_thread(struct evl_rq *rq)
>>>  {
>>>  	struct evl_sched_class *sched_class;
>>>  	struct evl_thread *curr = rq->curr;
>>> @@ -821,8 +822,14 @@ static struct evl_thread *__pick_next_thread(struct evl_rq *rq)
>>>  /* rq->curr->lock + rq->lock held, hard irqs off. */
>>>  static struct evl_thread *pick_next_thread(struct evl_rq *rq)
>>>  {
>>> -	struct evl_thread *next = __pick_next_thread(rq);
>>> +	struct evl_thread *next, *prev = rq->curr;
>>> +	struct evl_sched_class *prev_class = prev->sched_class;
>>>  
>>> +	if (prev_class->sched_out)
>>> +		prev_class->sched_out(prev);
>>> +
>>> +	next = __pick_next_thread(rq);
>>> +	trace_evl_pick_thread(next);
>>>  	set_next_running(rq, next);
>>>  
>>>  	return next;
>>> @@ -972,21 +979,20 @@ void __evl_schedule(void) /* oob or/and hard irqs off (CPU migration-safe) */
>>>  		return;
>>>  	}
>>>  
>>> +	prev = curr;
>>>  	next = pick_next_thread(this_rq);
>>> -	trace_evl_pick_thread(next);
>>> -	if (next == curr) {
>>> -		if (unlikely(next->state & EVL_T_ROOT)) {
>>> +	if (next == prev) {
>>> +		if (unlikely(prev->state & EVL_T_ROOT)) {
>>>  			if (this_rq->local_flags & RQ_TPROXY)
>>>  				evl_notify_proxy_tick(this_rq);
>>>  			if (this_rq->local_flags & RQ_TDEFER)
>>>  				evl_program_local_tick(&evl_mono_clock);
>>>  		}
>>>  		raw_spin_unlock(&this_rq->lock);
>>> -		raw_spin_unlock_irqrestore(&curr->lock, flags);
>>> +		raw_spin_unlock_irqrestore(&prev->lock, flags);
>>>  		return;
>>>  	}
>>> - 
>>> -	prev = curr;
>>
>> This hunk subtly look like a pure cosmetic change, without any logical
>> impact. If it does have one which I missed, it should be explained in
>> the commit message at least.
>>
> 
> It has none, as mentioned earlier in a previous post. The point of this
> change is to clarify the naming, introducing 'prev' as an alias to
> 'curr' in the swap sequence. So there is no point in documenting this.
> 

BTW, I would not make lock/unlock work against different variables
names, though they carry identical pointers. That is more confusing than
the current code.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center