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

[Jürgen Groß <jgross@xxxxxxxx>]

On 03.08.26 13:12, Furkan Çalışkan wrote:
> 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.
Now THIS could be done in a separate patch.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature