[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/6] xen/sched: Improvements to the {alloc, free}_domdata() interfaces
Ping Dornerworks. Any comment (on this and the next patch), or are you happy for these trivial changes to fall under general scheduler maintainership? ~Andrew On 28/02/18 14:14, Andrew Cooper wrote: > 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 <andrew.cooper3@xxxxxxxxxx> > --- > CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx> > CC: Dario Faggioli <dfaggioli@xxxxxxxx> > CC: Meng Xu <mengxu@xxxxxxxxxxxxx> > CC: Josh Whitehead <josh.whitehead@xxxxxxxxxxxxxxx> > CC: Robert VanVossen <robert.vanvossen@xxxxxxxxxxxxxxx> > --- > 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 982845b..17e765d 100644 > --- a/xen/common/sched_arinc653.c > +++ b/xen/common/sched_arinc653.c > @@ -455,34 +455,6 @@ a653sched_free_vdata(const struct scheduler *ops, void > *priv) > } > > /** > - * 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 > * > * @param ops Pointer to this instance of the scheduler structure > @@ -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 f8212db..e2133df 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -1282,7 +1282,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); > @@ -1290,7 +1290,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 > @@ -1302,8 +1302,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 b094b3c..29a24d6 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 b4a24ba..4dd405b 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 a202802..e4ff5c1 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 b788426..08a31b6 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 c4a4935..56e7d0c 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; > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |