Re: [PATCH 0/7] xen/sched: split scheduler vtable from scheduler

Hi Jürgen,

On 8/3/26 13:34, Jürgen Groß wrote:
> On 03.08.26 07:06, Furkan Caliskan wrote:
>> Each struct scheduler currently doubles as both a scheduler
>> backend's static vtable (name, opt_name, sched_id and every
>> function pointer) and the per-cpupool runtime object that
>> scheduler_alloc() allocates. Because these are the same type,
>> scheduler_alloc() memcpy()s the entire vtable into a fresh heap
>> allocation for every cpupool it creates. With N cpupools running
>> the same scheduler, this duplicates N copies of identical function
>> pointers and identifying fields that never differ between
>> instances - the only fields that are genuinely per-cpupool are
>> sched_data and cpupool.
>>
>> This series splits the vtable out into its own type, struct
>> sched_ops, so it can be shared by every cpupool using a given
>> scheduler instead of copied per cpupool. struct scheduler is left
>> holding only what is actually per-instance: a pointer to the
>> shared sched_ops, plus sched_data and cpupool.
>>
>> The series is structured as introduce/migrate/remove, so that
>> every commit builds and boots on its own:
>>
>>    - The first patch adds struct sched_ops, REGISTER_SCHED_OPS(),
>>      and a sched_ops_array[] alongside the existing schedulers[],
>>      extending every lookup path (scheduler_alloc(),
>>      sched_get_by_name(), scheduler_init()) to search both arrays.
>>      This is purely additive - no scheduler uses it yet.
>>
>>    - The next five patches each migrate one scheduler backend
>>      (credit, credit2, rtds, arinc653, null) from struct scheduler
>>      to struct sched_ops. Each is small, mechanical, and
>>      independently bisectable, with no behavioral difference, since
>>      scheduler_alloc() builds an identical runtime struct scheduler
>>      regardless of which array a match is found in.
>>
>>    - The final patch removes the old schedulers[] and
>>      REGISTER_SCHEDULER() path now that nothing uses it, shrinks
>>      struct scheduler down to { ops, sched_data, cpupool }, and
>>      updates every accessor in private.h accordingly.
>>
>> Furkan Caliskan (7):
>>    xen/sched: introduce struct sched_ops as a shared scheduler vtable
>>    xen/sched: credit: migrate to new sched_ops
>>    xen/sched: credit2: migrate to new sched_ops
>>    xen/sched: rtds: migrate to new sched_ops
>>    xen/sched: arinc653: migrate to new sched_ops
>>    xen/sched: null: migrate to new sched_ops
>>    xen/sched: remove old scheduler registration, shrink struct scheduler
>>
>>   xen/arch/arm/xen.lds.S      |   2 +-
>>   xen/arch/ppc/xen.lds.S      |   2 +-
>>   xen/arch/riscv/xen.lds.S    |   2 +-
>>   xen/arch/x86/xen.lds.S      |   2 +-
>>   xen/common/sched/arinc653.c |  11 +---
>>   xen/common/sched/core.c     |  80 ++++++++++++++++-------------
>>   xen/common/sched/cpupool.c  |   6 +--
>>   xen/common/sched/credit.c   |   5 +-
>>   xen/common/sched/credit2.c  |   5 +-
>>   xen/common/sched/null.c     |   5 +-
>>   xen/common/sched/private.h  | 100 +++++++++++++++++++-----------------
>>   xen/common/sched/rt.c       |   5 +-
>>   xen/include/xen/xen.lds.h   |   8 +--
>>   13 files changed, 116 insertions(+), 117 deletions(-)
>>
> 
> You have a series here which is adding 116 lines and removing 117.
> 
> Patch 7 alone is removing 248 lines while adding 67 lines.
> 
> So in the end there is a single patch in this series which has more code
> churn than the complete series when added in one go.
> 
> IOW: making this just a single patch would be easier to review than the
> last patch alone, let alone all the temporary modifications which would
> be gone when merging all patches into one. And with that you could even
> drop some of the renaming you did (e.g. in the linker file), making the
> diff even smaller.
> 
> I agree with the overall goal, but I'm sparing my time doing a thorough
> review of the series in this shape.
> 
> 
> Juergen

My first instinct was actually to just send this as one patch. I split 
it up because I wanted each scheduler's conversion to be its own small, 
bisectable commit, but you're right that it's not worth it here. 

I will squash it into a single patch and resend. But I would still like 
to rename SCHEDULER_ARRAY to SCHED_OPS_ARRAY in the per-arch linker files 
and other related variable names in other files, since they now hold 
sched_ops entries rather than struct scheduler ones, and I think the name 
should reflect that. 

Thanks for the feedback,

Furkan Caliskan