Re: [PATCH v2 1/2] xen/sched: split scheduler vtable from struct scheduler

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?