xen: sched: get rid of static private schedulers' structures

PATCH In review Dario Faggioli 3 participants 4 messages Feb 8, 2016 – Feb 23, 2016

Share & tools

Dario Faggioli

author

2016-02-08 13:19 UTC #0 lore raw

PATCH 1/1 xen: sched: get rid of static private schedulers' structures

In fact, they look rather useless: they are never referenced neither directly, nor via the sched_data pointer, as a dynamic copy that overrides them is allocated as the very first step of a scheduler's initialization.

While there, take the chance to also reset the sched_data pointer to NULL, upon scheduler de-initialization.

Signed-off-by Dario Faggioli <[email protected]>

---
Cc: George Dunlap <[email protected]>
Cc: Juergen Gross <[email protected]>

---
 xen/common/sched_arinc653.c |    3 ++-
 xen/common/sched_credit.c   |    7 +++----
 xen/common/sched_credit2.c  |    8 +++-----
 xen/common/sched_rt.c       |    7 +++----
 xen/include/xen/sched-if.h  |    2 +-
 5 files changed, 12 insertions(+), 15 deletions(-)

xen/common/sched_arinc653.c +2 -1

index 0606988..8a11a2f 100644
@@ -367,9 +367,10 @@ a653sched_init(struct scheduler *ops)
  * @param ops       Pointer to this instance of the scheduler structure
  */
 static void
-a653sched_deinit(const struct scheduler *ops)
+a653sched_deinit(struct scheduler *ops)
 {
     xfree(SCHED_PRIV(ops));
+    ops->sched_data = NULL;
 }
 
 /**

xen/common/sched_credit.c +3 -4

index 671bbee..25a4773 100644
@@ -1959,13 +1959,14 @@ csched_init(struct scheduler *ops)
 }
 
 static void
-csched_deinit(const struct scheduler *ops)
+csched_deinit(struct scheduler *ops)
 {
     struct csched_private *prv;
 
     prv = CSCHED_PRIV(ops);
     if ( prv != NULL )
     {
+        ops->sched_data = NULL;
         free_cpumask_var(prv->cpus);
         free_cpumask_var(prv->idlers);
         xfree(prv);

@@ -1995,13 +1996,11 @@ static void csched_tick_resume(const struct scheduler *ops, unsigned int cpu)
             - now % MICROSECS(prv->tick_period_us) );
 }
 
-static struct csched_private _csched_priv;
-
 static const struct scheduler sched_credit_def = {
     .name           = "SMP Credit Scheduler",
     .opt_name       = "credit",
     .sched_id       = XEN_SCHEDULER_CREDIT,
-    .sched_data     = &_csched_priv,
+    .sched_data     = NULL,
 
     .init_domain    = csched_dom_init,
     .destroy_domain = csched_dom_destroy,

xen/common/sched_credit2.c +3 -5

index 78220a7..68a8f1c 100644
@@ -2183,22 +2183,20 @@ csched2_init(struct scheduler *ops)
 }
 
 static void
-csched2_deinit(const struct scheduler *ops)
+csched2_deinit(struct scheduler *ops)
 {
     struct csched2_private *prv;
 
     prv = CSCHED2_PRIV(ops);
+    ops->sched_data = NULL;
     xfree(prv);
 }
 
-
-static struct csched2_private _csched2_priv;
-
 static const struct scheduler sched_credit2_def = {
     .name           = "SMP Credit Scheduler rev2",
     .opt_name       = "credit2",
     .sched_id       = XEN_SCHEDULER_CREDIT2,
-    .sched_data     = &_csched2_priv,
+    .sched_data     = NULL,
 
     .init_domain    = csched2_dom_init,
     .destroy_domain = csched2_dom_destroy,

xen/common/sched_rt.c +3 -4

index 2e5430f..d82d69c 100644
@@ -462,7 +462,7 @@ rt_init(struct scheduler *ops)
 }
 
 static void
-rt_deinit(const struct scheduler *ops)
+rt_deinit(struct scheduler *ops)
 {
     struct rt_private *prv = rt_priv(ops);

@@ -473,6 +473,7 @@ rt_deinit(const struct scheduler *ops)
         xfree(_cpumask_scratch);
         _cpumask_scratch = NULL;
     }
+    ops->sched_data = NULL;
     xfree(prv);
 }

@@ -1168,13 +1169,11 @@ rt_dom_cntl(
     return rc;
 }
 
-static struct rt_private _rt_priv;
-
 static const struct scheduler sched_rtds_def = {
     .name           = "SMP RTDS Scheduler",
     .opt_name       = "rtds",
     .sched_id       = XEN_SCHEDULER_RTDS,
-    .sched_data     = &_rt_priv,
+    .sched_data     = NULL,
 
     .dump_cpu_state = rt_dump_pcpu,
     .dump_settings  = rt_dump,

xen/include/xen/sched-if.h +1 -1

index 66dc9c8..825f1ad 100644
@@ -126,7 +126,7 @@ struct scheduler {
     int          (*global_init)    (void);
 
     int          (*init)           (struct scheduler *);
-    void         (*deinit)         (const struct scheduler *);
+    void         (*deinit)         (struct scheduler *);
 
     void         (*free_vdata)     (const struct scheduler *, void *);
     void *       (*alloc_vdata)    (const struct scheduler *, struct vcpu *,

Jan Beulich

2016-02-09 11:27 UTC #1 lore raw

xen/common/sched_credit.c static void csched_tick_resume(const struct scheduler *ops, unsigned int cpu) · line 2003

Quoted 13 lines

Quoted 1 line

Quoted 1 line · On 08.02.16 at 14:19, <[email protected]> wrote:

xen/common/sched_credit.c +1 -3

@@ -1995,13 +1996,11 @@ static void csched_tick_resume(const struct scheduler *ops, unsigned int cpu)
             - now % MICROSECS(prv->tick_period_us) );
 }
 
-static struct csched_private _csched_priv;
-
 static const struct scheduler sched_credit_def = {
     .name           = "SMP Credit Scheduler",
     .opt_name       = "credit",
     .sched_id       = XEN_SCHEDULER_CREDIT,
-    .sched_data     = &_csched_priv,
+    .sched_data     = NULL,

You're maintainer of this code, so you know whether you really want these NULL initializers, but the look pretty pointless to me.

Jan

Dario Faggioli

author

2016-02-09 12:25 UTC #2 lore raw

Quoted 18 lines · Jan Beulich

Quoted 8 lines
Quoted 1 line
Quoted 1 line · On 08.02.16 at 14:19, <[email protected]> wrote:
xen/common/sched_credit.c +0 -0
@@ -1995,13 +1996,11 @@ static void csched_tick_resume(const struct
 scheduler *ops, unsigned int cpu)
              - now % MICROSECS(prv->tick_period_us) );
  }
xen/common/sched_credit.c +0 -2
-static struct csched_private _csched_priv;
-
Quoted 6 lines
static const struct scheduler sched_credit_def = { .name           = "SMP Credit Scheduler", .opt_name       = "credit",
     .sched_id       = XEN_SCHEDULER_CREDIT, -    .sched_data     = &_csched_priv,
+    .sched_data     = NULL,
You're maintainer of this code, so you know whether you really want these NULL initializers, but the look pretty pointless to me.

Yes, I've also been unsure about what to do here.

The reason why I put them there is that, when looking at the code, it is important to be able to quickly get the idea of where sched_data points, at any given time. Therefore, being a little bit more verbose than necessary would be, in this case, actually helpful.

Anyway, this is not a super strong opinion, so I can be talked into getting rid of them, if others think we better don't have them too.

Thanks and Regards,
Dario

Signature · 4 lines

<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

George Dunlap

Acked-by on v1 1/1

2016-02-23 12:29 UTC #3 lore raw

In reply to Dario Faggioli

Quoted 10 lines · Dario Faggioli

In fact, they look rather useless: they are never referenced neither directly, nor via the sched_data pointer, as a dynamic copy that overrides them is allocated as the very first step of a scheduler's initialization.
While there, take the chance to also reset the sched_data pointer to NULL, upon scheduler de-initialization.
Signed-off-by Dario Faggioli <[email protected]>

Acked-by George Dunlap <[email protected]>

Sorry for the delay

Quoted 142 lines