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

[Philippe Gerum <rpm@xenomai.org>]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 18.06.26 09:44, Philippe Gerum wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> On 17.06.26 16:20, Philippe Gerum wrote:
>>>> From: Philippe Gerum <rpm@xenomai.org>
>>>>
>>>> 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 <rpm@xenomai.org>
>>>> ---
>>>>  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.
>

I understand your point of view, I simply disagree with the scope of the
requirement as you state it. So let's move on.

-- 
Philippe.