Re: [PATCH v2 1/2] xen/sched: split scheduler vtable from struct scheduler
Stewart Hildebrand <[email protected]> Fri, 7 Aug 2026 10:08:32 +0200
| Newsgroups | gmane.comp.emulators.xen.devel |
|---|---|
| Message-ID | <[email protected]> |
On 8/4/26 07:53, Furkan Caliskan wrote: > struct scheduler currently serves two purposes: it is the static > vtable a scheduler backend defines (name, opt_name, sched_id, and > all its function pointers), and it is also the per-cpupool runtime > object scheduler_alloc() allocates. Being the same type forces > scheduler_alloc() to memcpy() the whole vtable into a fresh > allocation per cpupool, duplicating identical function pointers > across every cpupool using the same scheduler. > > Split 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. scheduler_alloc() now stores a pointer to > the matching sched_ops instance instead of copying its fields, and > every accessor in private.h is updated from s->field to > s->ops->field to match. > > Every in-tree scheduler backend (credit, credit2, rtds, arinc653, > null) is converted from struct scheduler to struct sched_ops. > > A handful of call sites elsewhere read a scheduler's name, > opt_name or sched_id directly and are updated to go through > ->ops as well. > > Signed-off-by: Furkan Caliskan <[email protected]> > --- > xen/common/sched/arinc653.c | 9 +--- > xen/common/sched/core.c | 52 +++++++++++--------- > xen/common/sched/cpupool.c | 7 +-- > xen/common/sched/credit.c | 3 +- > xen/common/sched/credit2.c | 3 +- > xen/common/sched/null.c | 3 +- > xen/common/sched/private.h | 98 +++++++++++++++++++------------------ > xen/common/sched/rt.c | 3 +- > 8 files changed, 89 insertions(+), 89 deletions(-) > > diff --git a/xen/common/sched/arinc653.c b/xen/common/sched/arinc653.c > index 32c596a23c..746963806e 100644 > --- a/xen/common/sched/arinc653.c > +++ b/xen/common/sched/arinc653.c > @@ -702,17 +702,10 @@ a653sched_adjust_global(const struct scheduler *ops, > } > #endif /* CONFIG_SYSCTL */ > > -/** > - * This structure defines our scheduler for Xen. > - * The entries tell Xen where to find our scheduler-specific > - * callback functions. > - * The symbol must be visible to the rest of Xen at link time. > - */ The removal of the comment about symbol visibility is a definite improvement, and I'm okay with the overall comment removal. However, next time I'd prefer this to be mentioned in the commit message. For now, I don't want to delay this any further, so for ARINC 653: Acked-by: Stewart Hildebrand <[email protected]> I will still give some additional remarks below, but I don't consider them blocking since they are nit/cosmetic and Juergen already gave his R-b. > -static const struct scheduler sched_arinc653_def = { > +static const struct sched_ops sched_arinc653_def = { > .name = "ARINC 653 Scheduler", > .opt_name = "arinc653", > .sched_id = XEN_SCHEDULER_ARINC653, > - .sched_data = NULL, > > .init = a653sched_init, > .deinit = a653sched_deinit, > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c > index 9ccf5811bf..5cdae0415c 100644 > --- a/xen/common/sched/core.c > +++ b/xen/common/sched/core.c > @@ -87,7 +87,7 @@ DEFINE_PER_CPU(cpumask_t, cpumask_scratch); > /* How many urgent vcpus. */ > DEFINE_PER_CPU(atomic_t, sched_urgent_count); > > -extern const struct scheduler *__start_schedulers_array[], *__end_schedulers_array[]; > +extern const struct sched_ops *__start_schedulers_array[], *__end_schedulers_array[]; > #define NUM_SCHEDULERS (__end_schedulers_array - __start_schedulers_array) > #define schedulers __start_schedulers_array > > @@ -127,10 +127,9 @@ static void cf_check sched_idle_schedule( > unit->next_task = sched_idle_unit(cpu); > } > > -static struct scheduler sched_idle_ops = { > +static struct sched_ops sched_idle_sched_ops = { > .name = "Idle Scheduler", > .opt_name = "idle", > - .sched_data = NULL, > > .pick_resource = sched_idle_res_pick, > .do_schedule = sched_idle_schedule, > @@ -139,6 +138,11 @@ static struct scheduler sched_idle_ops = { > .free_udata = sched_idle_free_udata, > }; > > +static struct scheduler sched_idle_ops = { > + .ops = &sched_idle_sched_ops, > + .sched_data = NULL, > +}; > + > static inline struct vcpu *unit2vcpu_cpu(const struct sched_unit *unit, > unsigned int cpu) > { > @@ -2081,7 +2085,7 @@ long do_set_timer_op(s_time_t timeout) > /* scheduler_id - fetch ID of current scheduler */ > int scheduler_id(void) > { > - return operations.sched_id; > + return operations.ops->sched_id; > } > #endif > > @@ -2090,7 +2094,7 @@ long sched_adjust(struct domain *d, struct xen_domctl_scheduler_op *op) > { > long ret; > > - if ( op->sched_id != dom_scheduler(d)->sched_id ) > + if ( op->sched_id != dom_scheduler(d)->ops->sched_id ) > return -EINVAL; > > switch ( op->cmd ) > @@ -2132,7 +2136,7 @@ long sched_adjust_global(struct xen_sysctl_scheduler_op *op) > > rcu_read_lock(&sched_res_rculock); > > - rc = ((op->sched_id == pool->sched->sched_id) > + rc = ((op->sched_id == pool->sched->ops->sched_id) > ? sched_adjust_cpupool(pool->sched, op) : -EINVAL); > > rcu_read_unlock(&sched_res_rculock); > @@ -2299,7 +2303,7 @@ static struct sched_unit *do_schedule(struct sched_unit *prev, s_time_t now, > struct sched_unit *next; > > /* get policy-specific decision on scheduling... */ > - sched->do_schedule(sched, prev, now, sched_tasklet_check(cpu)); > + sched->ops->do_schedule(sched, prev, now, sched_tasklet_check(cpu)); > > next = prev->next_task; > > @@ -2989,10 +2993,9 @@ void scheduler_enable(void) > } > > static inline > -const struct scheduler *__init sched_get_by_name(const char *sched_name) > +const struct sched_ops *__init sched_ops_get_by_name(const char* sched_name) Nit: whitespace: const char *sched_name > { > unsigned int i; > - Nit: please retain the newline here > for ( i = 0; i < NUM_SCHEDULERS; i++ ) > if ( schedulers[i] && !strcmp(schedulers[i]->opt_name, sched_name) ) > return schedulers[i]; > @@ -3002,16 +3005,15 @@ const struct scheduler *__init sched_get_by_name(const char *sched_name) > > int __init sched_get_id_by_name(const char *sched_name) > { > - const struct scheduler *scheduler = sched_get_by_name(sched_name); > - > - return scheduler ? scheduler->sched_id : -1; > + const struct sched_ops *ops = sched_ops_get_by_name(sched_name); Nit: newline > + return ops ? ops->sched_id : -1; > } > > /* Initialise the data structures. */ > void __init scheduler_init(void) > { > struct domain *idle_domain; > - const struct scheduler *scheduler; > + const struct sched_ops *ops; > int i; > > scheduler_enable(); > @@ -3044,21 +3046,23 @@ void __init scheduler_init(void) > } > } > > - scheduler = sched_get_by_name(opt_sched); > - if ( !scheduler ) > + ops = sched_ops_get_by_name(opt_sched); > + if ( !ops ) > { > printk("Could not find scheduler: %s\n", opt_sched); > - scheduler = sched_get_by_name(CONFIG_SCHED_DEFAULT); > - BUG_ON(!scheduler); > - printk("Using '%s' (%s)\n", scheduler->name, scheduler->opt_name); > + ops = sched_ops_get_by_name(CONFIG_SCHED_DEFAULT); > + BUG_ON(!ops); > + printk("Using '%s' (%s)\n", ops->name, ops->opt_name); > } > - operations = *scheduler; > + > + operations.ops = ops; > > if ( cpu_schedule_up(0) ) > BUG(); > register_cpu_notifier(&cpu_schedule_nfb); > > - printk("Using scheduler: %s (%s)\n", operations.name, operations.opt_name); > + printk("Using scheduler: %s (%s)\n", > + operations.ops->name, operations.ops->opt_name); > if ( sched_init(&operations) ) > panic("scheduler returned error on init\n"); > > @@ -3411,12 +3415,14 @@ struct scheduler *scheduler_alloc(unsigned int sched_id) > for ( i = 0; i < NUM_SCHEDULERS; i++ ) > if ( schedulers[i] && schedulers[i]->sched_id == sched_id ) > goto found; > + > return ERR_PTR(-ENOENT); > > found: > - if ( (sched = xmalloc(struct scheduler)) == NULL ) > + if ( (sched = xzalloc(struct scheduler)) == NULL ) The change to xzalloc deserves a mention in the commit message. > return ERR_PTR(-ENOMEM); > - memcpy(sched, schedulers[i], sizeof(*sched)); > + sched->ops = schedulers[i]; > + > if ( (ret = sched_init(sched)) != 0 ) > { > xfree(sched); > @@ -3447,7 +3453,7 @@ void schedule_dump(struct cpupool *c) > { > sched = c->sched; > cpus = c->res_valid; > - printk("Scheduler: %s (%s)\n", sched->name, sched->opt_name); > + printk("Scheduler: %s (%s)\n", sched->ops->name, sched->ops->opt_name); > sched_dump_settings(sched); > } > else > diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c > index 081e1053eb..640578201f 100644 > --- a/xen/common/sched/cpupool.c > +++ b/xen/common/sched/cpupool.c > @@ -338,7 +338,8 @@ static struct cpupool *cpupool_create(unsigned int poolid, > spin_unlock(&cpupool_lock); > > debugtrace_printk("Created cpupool %u with scheduler %s (%s)\n", > - c->cpupool_id, c->sched->name, c->sched->opt_name); > + c->cpupool_id, c->sched->ops->name, > + c->sched->ops->opt_name); > > return c; > > @@ -862,7 +863,7 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op) > if ( c == NULL ) > break; > op->cpupool_id = c->cpupool_id; > - op->sched_id = c->sched->sched_id; > + op->sched_id = c->sched->ops->sched_id; > op->n_dom = c->n_dom; > ret = cpumask_to_xenctl_bitmap(&op->cpumap, c->cpu_valid); > cpupool_put(c); > @@ -1294,7 +1295,7 @@ struct cpupool *__init cpupool_create_pool(unsigned int pool_id, int sched_id) > struct cpupool *pool; > > if ( sched_id < 0 ) > - sched_id = scheduler_get_default()->sched_id; > + sched_id = scheduler_get_default()->ops->sched_id; > > pool = cpupool_create(pool_id, sched_id); > > diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c > index 4dde2ede12..8df746bf6b 100644 > --- a/xen/common/sched/credit.c > +++ b/xen/common/sched/credit.c > @@ -2277,11 +2277,10 @@ csched_deinit(struct scheduler *ops) > } > } > > -static const struct scheduler sched_credit_def = { > +static const struct sched_ops sched_credit_def = { > .name = "SMP Credit Scheduler", > .opt_name = "credit", > .sched_id = XEN_SCHEDULER_CREDIT, > - .sched_data = NULL, > > .global_init = csched_global_init, > > diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c > index 95946634d1..4949606881 100644 > --- a/xen/common/sched/credit2.c > +++ b/xen/common/sched/credit2.c > @@ -4230,11 +4230,10 @@ csched2_deinit(struct scheduler *ops) > xfree(prv); > } > > -static const struct scheduler sched_credit2_def = { > +static const struct sched_ops sched_credit2_def = { > .name = "SMP Credit Scheduler rev2", > .opt_name = "credit2", > .sched_id = XEN_SCHEDULER_CREDIT2, > - .sched_data = NULL, > > .global_init = csched2_global_init, > > diff --git a/xen/common/sched/null.c b/xen/common/sched/null.c > index 952bb47444..b3c6651fb1 100644 > --- a/xen/common/sched/null.c > +++ b/xen/common/sched/null.c > @@ -1037,11 +1037,10 @@ static void cf_check null_dump(const struct scheduler *ops) > spin_unlock_irqrestore(&prv->lock, flags); > } > > -static const struct scheduler sched_null_def = { > +static const struct sched_ops sched_null_def = { > .name = "null Scheduler", > .opt_name = "null", > .sched_id = XEN_SCHEDULER_NULL, > - .sched_data = NULL, > > .init = null_init, > .deinit = null_deinit, > diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h > index d6884550cd..0c5181891c 100644 > --- a/xen/common/sched/private.h > +++ b/xen/common/sched/private.h > @@ -294,12 +294,10 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu) > return NULL; > } > > -struct scheduler { > - const char *name; /* full name for this scheduler */ > - const char *opt_name; /* option name for this scheduler */ > - unsigned int sched_id; /* ID for this scheduler */ > - void *sched_data; /* global data pointer */ > - struct cpupool *cpupool;/* points to this scheduler's pool */ > +struct sched_ops { > + const char *name; /* full name for this sched_ops */ > + const char *opt_name; /* option name for this sched_ops */ > + unsigned int sched_id; /* ID for this sched_ops */ > > int (*global_init) (void); > > @@ -366,127 +364,133 @@ struct scheduler { > struct sched_resource *sr); > }; > > +struct scheduler { > + const struct sched_ops *ops; /* shared, read-only dispatch table */ > + void *sched_data; /* global data pointer */ Isn't it a scheduler instance data pointer, not global?