From 340edc3902c1757a0e1f4391930366fb25a05df3 Mon Sep 17 00:00:00 2001 From: Andrew Cooper Date: Tue, 27 Feb 2018 16:48:19 +0000 Subject: [PATCH] xen/sched: Improvements to the {alloc,free}_domdata() interfaces The main purpose of this change is for the subsequent cleanup it enables, but it stands on its own merits. In principle, these hooks are optional, but the SCHED_OP() default aliases a memory allocation failure, which causes arinc653 to play the dangerous game of passing its priv pointer back, and remembering not to actually free it. Redefine alloc_domdata to use ERR_PTR() for errors, NULL for nothing, and non-NULL for a real allocation, which allows the hook to become properly optional. Redefine free_domdata to be idempotent. For arinc653, this means the dummy hooks can be dropped entirely. For the other schedulers, this means returning ERR_PTR(-ENOMEM) instead of NULL for memory allocation failures, and modifying the free hooks to cope with a NULL pointer. While making the alterations, drop some spurious casts to void *. Introduce and use proper wrappers for sched_{alloc,free}_domdata(). These are strictly better than SCHED_OP(), as the source code is visible to grep/cscope/tags, the generated code is better, and there can be proper per-hook defaults and checks. Callers of the alloc hooks are switched to using IS_ERR(), rather than checking for NULL. Signed-off-by: Andrew Cooper Reviewed-by: George Dunlap Acked-by: Meng Xu Reviewed-by: Dario Faggioli --- xen/common/sched_arinc653.c | 31 ------------------------------- xen/common/sched_credit.c | 8 ++++---- xen/common/sched_credit2.c | 24 +++++++++++++----------- xen/common/sched_null.c | 22 +++++++++++++--------- xen/common/sched_rt.c | 21 +++++++++++++-------- xen/common/schedule.c | 12 ++++++------ xen/include/xen/sched-if.h | 27 ++++++++++++++++++++++++++- 7 files changed, 75 insertions(+), 70 deletions(-) diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c index 982845b550..17e765d2cb 100644 --- a/xen/common/sched_arinc653.c +++ b/xen/common/sched_arinc653.c @@ -454,34 +454,6 @@ a653sched_free_vdata(const struct scheduler *ops, void *priv) update_schedule_vcpus(ops); } -/** - * This function allocates scheduler-specific data for a domain - * - * We do not actually make use of any per-domain data but the hypervisor - * expects a non-NULL return value - * - * @param ops Pointer to this instance of the scheduler structure - * - * @return Pointer to the allocated data - */ -static void * -a653sched_alloc_domdata(const struct scheduler *ops, struct domain *dom) -{ - /* return a non-NULL value to keep schedule.c happy */ - return SCHED_PRIV(ops); -} - -/** - * This function frees scheduler-specific data for a domain - * - * @param ops Pointer to this instance of the scheduler structure - */ -static void -a653sched_free_domdata(const struct scheduler *ops, void *data) -{ - /* nop */ -} - /** * Xen scheduler callback function to sleep a VCPU * @@ -740,9 +712,6 @@ static const struct scheduler sched_arinc653_def = { .free_vdata = a653sched_free_vdata, .alloc_vdata = a653sched_alloc_vdata, - .free_domdata = a653sched_free_domdata, - .alloc_domdata = a653sched_alloc_domdata, - .init_domain = NULL, .destroy_domain = NULL, diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index a2c5d43e33..d9ae23dee3 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -1281,7 +1281,7 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom) sdom = xzalloc(struct csched_dom); if ( sdom == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); /* Initialize credit and weight */ INIT_LIST_HEAD(&sdom->active_vcpu); @@ -1289,7 +1289,7 @@ csched_alloc_domdata(const struct scheduler *ops, struct domain *dom) sdom->dom = dom; sdom->weight = CSCHED_DEFAULT_WEIGHT; - return (void *)sdom; + return sdom; } static int @@ -1301,8 +1301,8 @@ csched_dom_init(const struct scheduler *ops, struct domain *dom) return 0; sdom = csched_alloc_domdata(ops, dom); - if ( sdom == NULL ) - return -ENOMEM; + if ( IS_ERR(sdom) ) + return PTR_ERR(sdom); dom->sched_priv = sdom; diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index b094b3c2be..29a24d6cb6 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -3012,7 +3012,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom) sdom = xzalloc(struct csched2_dom); if ( sdom == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); /* Initialize credit, cap and weight */ INIT_LIST_HEAD(&sdom->sdom_elem); @@ -3032,7 +3032,7 @@ csched2_alloc_domdata(const struct scheduler *ops, struct domain *dom) write_unlock_irqrestore(&prv->lock, flags); - return (void *)sdom; + return sdom; } static int @@ -3044,8 +3044,8 @@ csched2_dom_init(const struct scheduler *ops, struct domain *dom) return 0; sdom = csched2_alloc_domdata(ops, dom); - if ( sdom == NULL ) - return -ENOMEM; + if ( IS_ERR(sdom) ) + return PTR_ERR(sdom); dom->sched_priv = sdom; @@ -3055,19 +3055,21 @@ csched2_dom_init(const struct scheduler *ops, struct domain *dom) static void csched2_free_domdata(const struct scheduler *ops, void *data) { - unsigned long flags; struct csched2_dom *sdom = data; struct csched2_private *prv = csched2_priv(ops); - kill_timer(&sdom->repl_timer); - - write_lock_irqsave(&prv->lock, flags); + if ( sdom ) + { + unsigned long flags; - list_del_init(&sdom->sdom_elem); + kill_timer(&sdom->repl_timer); - write_unlock_irqrestore(&prv->lock, flags); + write_lock_irqsave(&prv->lock, flags); + list_del_init(&sdom->sdom_elem); + write_unlock_irqrestore(&prv->lock, flags); - xfree(data); + xfree(sdom); + } } static void diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c index b4a24baf8e..4dd405b2e5 100644 --- a/xen/common/sched_null.c +++ b/xen/common/sched_null.c @@ -231,7 +231,7 @@ static void * null_alloc_domdata(const struct scheduler *ops, ndom = xzalloc(struct null_dom); if ( ndom == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); ndom->dom = d; @@ -239,20 +239,24 @@ static void * null_alloc_domdata(const struct scheduler *ops, list_add_tail(&ndom->ndom_elem, &null_priv(ops)->ndom); spin_unlock_irqrestore(&prv->lock, flags); - return (void*)ndom; + return ndom; } static void null_free_domdata(const struct scheduler *ops, void *data) { - unsigned long flags; struct null_dom *ndom = data; struct null_private *prv = null_priv(ops); - spin_lock_irqsave(&prv->lock, flags); - list_del_init(&ndom->ndom_elem); - spin_unlock_irqrestore(&prv->lock, flags); + if ( ndom ) + { + unsigned long flags; + + spin_lock_irqsave(&prv->lock, flags); + list_del_init(&ndom->ndom_elem); + spin_unlock_irqrestore(&prv->lock, flags); - xfree(data); + xfree(ndom); + } } static int null_dom_init(const struct scheduler *ops, struct domain *d) @@ -263,8 +267,8 @@ static int null_dom_init(const struct scheduler *ops, struct domain *d) return 0; ndom = null_alloc_domdata(ops, d); - if ( ndom == NULL ) - return -ENOMEM; + if ( IS_ERR(ndom) ) + return PTR_ERR(ndom); d->sched_priv = ndom; diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c index a20280235e..e4ff5c19c9 100644 --- a/xen/common/sched_rt.c +++ b/xen/common/sched_rt.c @@ -820,7 +820,7 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom) sdom = xzalloc(struct rt_dom); if ( sdom == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); INIT_LIST_HEAD(&sdom->sdom_elem); sdom->dom = dom; @@ -836,14 +836,19 @@ rt_alloc_domdata(const struct scheduler *ops, struct domain *dom) static void rt_free_domdata(const struct scheduler *ops, void *data) { - unsigned long flags; struct rt_dom *sdom = data; struct rt_private *prv = rt_priv(ops); - spin_lock_irqsave(&prv->lock, flags); - list_del_init(&sdom->sdom_elem); - spin_unlock_irqrestore(&prv->lock, flags); - xfree(data); + if ( sdom ) + { + unsigned long flags; + + spin_lock_irqsave(&prv->lock, flags); + list_del_init(&sdom->sdom_elem); + spin_unlock_irqrestore(&prv->lock, flags); + + xfree(sdom); + } } static int @@ -856,8 +861,8 @@ rt_dom_init(const struct scheduler *ops, struct domain *dom) return 0; sdom = rt_alloc_domdata(ops, dom); - if ( sdom == NULL ) - return -ENOMEM; + if ( IS_ERR(sdom) ) + return PTR_ERR(sdom); dom->sched_priv = sdom; diff --git a/xen/common/schedule.c b/xen/common/schedule.c index b7884263f2..08a31b69de 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -318,14 +318,14 @@ int sched_move_domain(struct domain *d, struct cpupool *c) return -EBUSY; } - domdata = SCHED_OP(c->sched, alloc_domdata, d); - if ( domdata == NULL ) - return -ENOMEM; + domdata = sched_alloc_domdata(c->sched, d); + if ( IS_ERR(domdata) ) + return PTR_ERR(domdata); vcpu_priv = xzalloc_array(void *, d->max_vcpus); if ( vcpu_priv == NULL ) { - SCHED_OP(c->sched, free_domdata, domdata); + sched_free_domdata(c->sched, domdata); return -ENOMEM; } @@ -337,7 +337,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c) for_each_vcpu ( d, v ) xfree(vcpu_priv[v->vcpu_id]); xfree(vcpu_priv); - SCHED_OP(c->sched, free_domdata, domdata); + sched_free_domdata(c->sched, domdata); return -ENOMEM; } } @@ -393,7 +393,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c) domain_unpause(d); - SCHED_OP(old_ops, free_domdata, old_domdata); + sched_free_domdata(old_ops, old_domdata); xfree(vcpu_priv); diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h index c4a493587a..56e7d0c2b2 100644 --- a/xen/include/xen/sched-if.h +++ b/xen/include/xen/sched-if.h @@ -146,8 +146,11 @@ struct scheduler { void * (*alloc_pdata) (const struct scheduler *, int); void (*init_pdata) (const struct scheduler *, void *, int); void (*deinit_pdata) (const struct scheduler *, void *, int); - void (*free_domdata) (const struct scheduler *, void *); + + /* Returns ERR_PTR(-err) for error, NULL for 'nothing needed'. */ void * (*alloc_domdata) (const struct scheduler *, struct domain *); + /* Idempotent. */ + void (*free_domdata) (const struct scheduler *, void *); void (*switch_sched) (struct scheduler *, unsigned int, void *, void *); @@ -181,6 +184,28 @@ struct scheduler { void (*tick_resume) (const struct scheduler *, unsigned int); }; +static inline void *sched_alloc_domdata(const struct scheduler *s, + struct domain *d) +{ + if ( s->alloc_domdata ) + return s->alloc_domdata(s, d); + else + return NULL; +} + +static inline void sched_free_domdata(const struct scheduler *s, + void *data) +{ + if ( s->free_domdata ) + s->free_domdata(s, data); + else + /* + * Check that if there isn't a free_domdata hook, we haven't got any + * data we're expected to deal with. + */ + ASSERT(!data); +} + #define REGISTER_SCHEDULER(x) static const struct scheduler *x##_entry \ __used_section(".data.schedulers") = &x; -- 2.39.5