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

Philippe Gerum <[email protected]> writes:

> Jan Kiszka <[email protected]> writes:
>
>> 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.
>>
>
> I understand your point of view, I simply disagree with the scope of the
> requirement as you state it. So let's move on.

Meanwhile, I just received a second opinion stating that this change
seems vaguely troubling, raising interior alarm bells. I must be the
only one to find it trivial, so ok. I'll split that, moving it to a
separate patch.

-- 
Philippe.