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

Again, it is suboptimal style to fold refactorings into logical code
changes. If you split this up into two commits, properly explaining the
purpose of that renaming, things become readable to 3rd parties.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center