xen/sched: remove sched_init_pdata()

Commit Message

Jürgen Groß Feb. 10, 2020, 3:39 p.m. UTC

sched_init_pdata() is used nowhere, it can be removed. Same applies to
the .init_pdata hook of the per-scheduler interface.

Signed-off-by: Juergen Gross <[e-mail]>
---
 xen/common/sched/credit.c  | 12 ------------
 xen/common/sched/credit2.c | 21 ---------------------
 xen/common/sched/null.c    | 10 ----------
 xen/common/sched/private.h |  8 --------
 xen/common/sched/rt.c      | 31 -------------------------------
 5 files changed, 82 deletions(-)

Comments

Dario Faggioli Feb. 11, 2020, 10:37 a.m. UTC | #1

On Mon, 2020-02-10 at 16:39 +0100, Juergen Gross wrote:
> sched_init_pdata() is used nowhere, it can be removed. Same applies
> to
> the .init_pdata hook of the per-scheduler interface.
>
Right, and that appear to be the case since
f855dd962523b6cb47a92037bdd28b1485141abe ("sched: add minimalistic idle
scheduler for free cpus"), which removed all call sites.

And that is because switching to a scheduler always happens via
switch_sched from the idle scheduler, and it's there that we do all the
initializations, right?

This change is obviously doing the right thing, removing code that is
never called! :-)

Can we, though:
- add a mention to the commit above and a quick explanation of things 
  in the changelog?
- update the following comments too:
  1) in cpu_schedule_callback()

"* This happens by calling the deinit_pdata and free_pdata hooks, in this                                          
 * order. If no per-pCPU memory was allocated, there is no need to                                                 
 * provide an implementation of free_pdata. deinit_pdata may, however,                                             
 * be necessary/useful in this case too (e.g., it can undo something done                                          
 * on scheduler wide data structure during init_pdata). Both deinit_pdata                                          
 * and free_pdata are called during CPU_DEAD."

  2) schedule_cpu_add()

"*  - a valid instance of per-CPU scheduler specific data, as it is
 *    allocated by sched_alloc_pdata(). Note that we do not want to
 *    initialize it yet (i.e., we are not calling sched_init_pdata()).
 *    That will be done by the target scheduler, in sched_switch_sched(),
 *    in proper ordering and with locking."

Regards

Jürgen Groß Feb. 11, 2020, 10:39 a.m. UTC | #2

On 11.02.20 11:37, Dario Faggioli wrote:
> On Mon, 2020-02-10 at 16:39 +0100, Juergen Gross wrote:
>> sched_init_pdata() is used nowhere, it can be removed. Same applies
>> to
>> the .init_pdata hook of the per-scheduler interface.
>>
> Right, and that appear to be the case since
> f855dd962523b6cb47a92037bdd28b1485141abe ("sched: add minimalistic idle
> scheduler for free cpus"), which removed all call sites.
> 
> And that is because switching to a scheduler always happens via
> switch_sched from the idle scheduler, and it's there that we do all the
> initializations, right?
> 
> This change is obviously doing the right thing, removing code that is
> never called! :-)
> 
> Can we, though:
> - add a mention to the commit above and a quick explanation of things
>    in the changelog?

Okay.

> - update the following comments too:
>    1) in cpu_schedule_callback()
> 
> "* This happens by calling the deinit_pdata and free_pdata hooks, in this
>   * order. If no per-pCPU memory was allocated, there is no need to
>   * provide an implementation of free_pdata. deinit_pdata may, however,
>   * be necessary/useful in this case too (e.g., it can undo something done
>   * on scheduler wide data structure during init_pdata). Both deinit_pdata
>   * and free_pdata are called during CPU_DEAD."
> 
>    2) schedule_cpu_add()
> 
> "*  - a valid instance of per-CPU scheduler specific data, as it is
>   *    allocated by sched_alloc_pdata(). Note that we do not want to
>   *    initialize it yet (i.e., we are not calling sched_init_pdata()).
>   *    That will be done by the target scheduler, in sched_switch_sched(),
>   *    in proper ordering and with locking."

Oh, I missed those. Will modify the comments.


Juergen

Patch

diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index 05946eea6e..93d89da278 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -614,17 +614,6 @@  init_pdata(struct csched_private *prv, struct csched_pcpu *spc, int cpu)
     spc->nr_runnable = 0;
 }
 
-static void
-csched_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
-{
-    unsigned long flags;
-    struct csched_private *prv = CSCHED_PRIV(ops);
-
-    spin_lock_irqsave(&prv->lock, flags);
-    init_pdata(prv, pdata, cpu);
-    spin_unlock_irqrestore(&prv->lock, flags);
-}
-
 /* Change the scheduler of cpu to us (Credit). */
 static spinlock_t *
 csched_switch_sched(struct scheduler *new_ops, unsigned int cpu,
@@ -2273,7 +2262,6 @@  static const struct scheduler sched_credit_def = {
     .alloc_udata    = csched_alloc_udata,
     .free_udata     = csched_free_udata,
     .alloc_pdata    = csched_alloc_pdata,
-    .init_pdata     = csched_init_pdata,
     .deinit_pdata   = csched_deinit_pdata,
     .free_pdata     = csched_free_pdata,
     .switch_sched   = csched_switch_sched,
diff --git a/xen/common/sched/credit2.c b/xen/common/sched/credit2.c
index 231f87d960..b965cd1c7b 100644
--- a/xen/common/sched/credit2.c
+++ b/xen/common/sched/credit2.c
@@ -3818,26 +3818,6 @@  init_pdata(struct csched2_private *prv, struct csched2_pcpu *spc,
     return spc->runq_id;
 }
 
-static void
-csched2_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
-{
-    struct csched2_private *prv = csched2_priv(ops);
-    spinlock_t *old_lock;
-    unsigned long flags;
-    unsigned rqi;
-
-    write_lock_irqsave(&prv->lock, flags);
-    old_lock = pcpu_schedule_lock(cpu);
-
-    rqi = init_pdata(prv, pdata, cpu);
-    /* Move the scheduler lock to the new runq lock. */
-    get_sched_res(cpu)->schedule_lock = &prv->rqd[rqi].lock;
-
-    /* _Not_ pcpu_schedule_unlock(): schedule_lock may have changed! */
-    spin_unlock(old_lock);
-    write_unlock_irqrestore(&prv->lock, flags);
-}
-
 /* Change the scheduler of cpu to us (Credit2). */
 static spinlock_t *
 csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu,
@@ -4085,7 +4065,6 @@  static const struct scheduler sched_credit2_def = {
     .alloc_udata    = csched2_alloc_udata,
     .free_udata     = csched2_free_udata,
     .alloc_pdata    = csched2_alloc_pdata,
-    .init_pdata     = csched2_init_pdata,
     .deinit_pdata   = csched2_deinit_pdata,
     .free_pdata     = csched2_free_pdata,
     .switch_sched   = csched2_switch_sched,
diff --git a/xen/common/sched/null.c b/xen/common/sched/null.c
index 8c3101649d..82d5d1baab 100644
--- a/xen/common/sched/null.c
+++ b/xen/common/sched/null.c
@@ -166,15 +166,6 @@  static void init_pdata(struct null_private *prv, struct null_pcpu *npc,
     npc->unit = NULL;
 }
 
-static void null_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
-{
-    struct null_private *prv = null_priv(ops);
-
-    ASSERT(pdata);
-
-    init_pdata(prv, pdata, cpu);
-}
-
 static void null_deinit_pdata(const struct scheduler *ops, void *pcpu, int cpu)
 {
     struct null_private *prv = null_priv(ops);
@@ -1042,7 +1033,6 @@  static const struct scheduler sched_null_def = {
     .deinit         = null_deinit,
     .alloc_pdata    = null_alloc_pdata,
     .free_pdata     = null_free_pdata,
-    .init_pdata     = null_init_pdata,
     .switch_sched   = null_switch_sched,
     .deinit_pdata   = null_deinit_pdata,
 
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 2a94179baa..367811a12f 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -306,7 +306,6 @@  struct scheduler {
                                     struct sched_unit *, void *);
     void         (*free_pdata)     (const struct scheduler *, void *, int);
     void *       (*alloc_pdata)    (const struct scheduler *, int);
-    void         (*init_pdata)     (const struct scheduler *, void *, int);
     void         (*deinit_pdata)   (const struct scheduler *, void *, int);
 
     /* Returns ERR_PTR(-err) for error, NULL for 'nothing needed'. */
@@ -408,13 +407,6 @@  static inline void sched_free_pdata(const struct scheduler *s, void *data,
         s->free_pdata(s, data, cpu);
 }
 
-static inline void sched_init_pdata(const struct scheduler *s, void *data,
-                                    int cpu)
-{
-    if ( s->init_pdata )
-        s->init_pdata(s, data, cpu);
-}
-
 static inline void sched_deinit_pdata(const struct scheduler *s, void *data,
                                       int cpu)
 {
diff --git a/xen/common/sched/rt.c b/xen/common/sched/rt.c
index 66585ed50a..c24cd2ac32 100644
--- a/xen/common/sched/rt.c
+++ b/xen/common/sched/rt.c
@@ -713,36 +713,6 @@  rt_deinit(struct scheduler *ops)
     xfree(prv);
 }
 
-/*
- * Point per_cpu spinlock to the global system lock;
- * All cpu have same global system lock
- */
-static void
-rt_init_pdata(const struct scheduler *ops, void *pdata, int cpu)
-{
-    struct rt_private *prv = rt_priv(ops);
-    spinlock_t *old_lock;
-    unsigned long flags;
-
-    old_lock = pcpu_schedule_lock_irqsave(cpu, &flags);
-
-    /*
-     * TIMER_STATUS_invalid means we are the first cpu that sees the timer
-     * allocated but not initialized, and so it's up to us to initialize it.
-     */
-    if ( prv->repl_timer.status == TIMER_STATUS_invalid )
-    {
-        init_timer(&prv->repl_timer, repl_timer_handler, (void *)ops, cpu);
-        dprintk(XENLOG_DEBUG, "RTDS: timer initialized on cpu %u\n", cpu);
-    }
-
-    /* Move the scheduler lock to our global runqueue lock.  */
-    get_sched_res(cpu)->schedule_lock = &prv->lock;
-
-    /* _Not_ pcpu_schedule_unlock(): per_cpu().schedule_lock changed! */
-    spin_unlock_irqrestore(old_lock, flags);
-}
-
 /* Change the scheduler of cpu to us (RTDS). */
 static spinlock_t *
 rt_switch_sched(struct scheduler *new_ops, unsigned int cpu,
@@ -1568,7 +1538,6 @@  static const struct scheduler sched_rtds_def = {
     .dump_settings  = rt_dump,
     .init           = rt_init,
     .deinit         = rt_deinit,
-    .init_pdata     = rt_init_pdata,
     .switch_sched   = rt_switch_sched,
     .deinit_pdata   = rt_deinit_pdata,
     .alloc_domdata  = rt_alloc_domdata,