| Message ID | 20160318190416.8117.57233.stgit@Solace.station (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 18, 2016 at 3:04 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > The .alloc_pdata scheduler hook must, before this change, > be implemented by all schedulers --even those ones that > don't need to allocate anything. > > Make it possible to just use the SCHED_OP(), like for > the other hooks, by using ERR_PTR() and IS_ERR() for > error reporting. This: > - makes NULL a variant of success; > - allows for errors other than ENOMEM to be properly > communicated (if ever necessary). > > This, in turn, means that schedulers not needing to > allocate any per-pCPU data, can avoid implementing the > hook. In fact, the artificial implementation of > .alloc_pdata in the ARINC653 is removed (and, while there, > nuke .free_pdata too, as it is equally useless). > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Robert VanVossen <robert.vanvossen@dornerworks.com> > Cc: Josh Whitehead <josh.whitehead@dornerworks.com> > Cc: Meng Xu <mengxu@cis.upenn.edu> > Cc: Jan Beulich <JBeulich@suse.com> > Cc: Juergen Gross <jgross@suse.com> > --- > static void > diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c > index d98bfb6..ac8019f 100644 > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -665,7 +665,7 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu) > spin_unlock_irqrestore(old_lock, flags); > > if ( !alloc_cpumask_var(&_cpumask_scratch[cpu]) ) > - return NULL; > + return ERR_PTR(-ENOMEM); > > if ( prv->repl_timer == NULL ) > { > @@ -673,13 +673,12 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu) > prv->repl_timer = xzalloc(struct timer); > > if ( prv->repl_timer == NULL ) > - return NULL; > + return ERR_PTR(-ENOMEM); > > init_timer(prv->repl_timer, repl_timer_handler, (void *)ops, cpu); > } > > - /* 1 indicates alloc. succeed in schedule.c */ > - return (void *)1; > + return NULL; > } > > static void > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index 0627eb5..1adc0e2 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int cpu) > if ( idle_vcpu[cpu] == NULL ) > return -ENOMEM; > > - if ( (ops.alloc_pdata != NULL) && > - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) ) > - return -ENOMEM; > + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); > + if ( IS_ERR(sd->sched_priv) ) > + return PTR_ERR(sd->sched_priv); > > return 0; > } > @@ -1503,8 +1503,7 @@ static void cpu_schedule_down(unsigned int cpu) > struct schedule_data *sd = &per_cpu(schedule_data, cpu); > struct scheduler *sched = per_cpu(scheduler, cpu); > > - if ( sd->sched_priv != NULL ) > - SCHED_OP(sched, free_pdata, sd->sched_priv, cpu); > + SCHED_OP(sched, free_pdata, sd->sched_priv, cpu); > SCHED_OP(sched, free_vdata, idle_vcpu[cpu]->sched_priv); > > idle_vcpu[cpu]->sched_priv = NULL; > @@ -1599,9 +1598,8 @@ void __init scheduler_init(void) > idle_domain->max_vcpus = nr_cpu_ids; > if ( alloc_vcpu(idle_domain, 0, 0) == NULL ) > BUG(); > - if ( ops.alloc_pdata && > - !(this_cpu(schedule_data).sched_priv = ops.alloc_pdata(&ops, 0)) ) > - BUG(); > + this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0); > + BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv)); > SCHED_OP(&ops, init_pdata, this_cpu(schedule_data).sched_priv, 0); > } > > @@ -1644,8 +1642,8 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c) > > idle = idle_vcpu[cpu]; > ppriv = SCHED_OP(new_ops, alloc_pdata, cpu); > - if ( ppriv == NULL ) > - return -ENOMEM; > + if ( IS_ERR(ppriv) ) > + return PTR_ERR(ppriv); > SCHED_OP(new_ops, init_pdata, ppriv, cpu); > vpriv = SCHED_OP(new_ops, alloc_vdata, idle, idle->domain->sched_priv); > if ( vpriv == NULL ) > diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h > index 70c08c6..560cba5 100644 > --- a/xen/include/xen/sched-if.h > +++ b/xen/include/xen/sched-if.h > @@ -9,6 +9,7 @@ > #define __XEN_SCHED_IF_H__ > > #include <xen/percpu.h> > +#include <xen/err.h> > > /* A global pointer to the initial cpupool (POOL0). */ > extern struct cpupool *cpupool0; > For the sched_rt.c and schedule.c, Reviewed-by: Meng Xu <mengxu@cis.upenn.edu> Thanks, Meng
>>> On 18.03.16 at 20:04, <dario.faggioli@citrix.com> wrote: > --- a/xen/include/xen/sched-if.h > +++ b/xen/include/xen/sched-if.h > @@ -9,6 +9,7 @@ > #define __XEN_SCHED_IF_H__ > > #include <xen/percpu.h> > +#include <xen/err.h> > > /* A global pointer to the initial cpupool (POOL0). */ > extern struct cpupool *cpupool0; There is no visible use in this header of what err.h defines - why does it get included all of the sudden? Jan
On 18/03/16 20:04, Dario Faggioli wrote: > The .alloc_pdata scheduler hook must, before this change, > be implemented by all schedulers --even those ones that > don't need to allocate anything. > > Make it possible to just use the SCHED_OP(), like for > the other hooks, by using ERR_PTR() and IS_ERR() for > error reporting. This: > - makes NULL a variant of success; > - allows for errors other than ENOMEM to be properly > communicated (if ever necessary). > > This, in turn, means that schedulers not needing to > allocate any per-pCPU data, can avoid implementing the > hook. In fact, the artificial implementation of > .alloc_pdata in the ARINC653 is removed (and, while there, > nuke .free_pdata too, as it is equally useless). > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > Cc: Robert VanVossen <robert.vanvossen@dornerworks.com> > Cc: Josh Whitehead <josh.whitehead@dornerworks.com> > Cc: Meng Xu <mengxu@cis.upenn.edu> > Cc: Jan Beulich <JBeulich@suse.com> > Cc: Juergen Gross <jgross@suse.com> > --- > Changes from v1: > * use IS_ERR() and friends to deal with the return value > of alloc_pdata, as suggested during review. > --- > xen/common/sched_arinc653.c | 31 ------------------------------- > xen/common/sched_credit.c | 4 ++-- > xen/common/sched_credit2.c | 2 +- > xen/common/sched_rt.c | 7 +++---- > xen/common/schedule.c | 18 ++++++++---------- > xen/include/xen/sched-if.h | 1 + > 6 files changed, 15 insertions(+), 48 deletions(-) > > diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c > index 8a11a2f..b79fcdf 100644 > --- a/xen/common/sched_arinc653.c > +++ b/xen/common/sched_arinc653.c > @@ -456,34 +456,6 @@ a653sched_free_vdata(const struct scheduler *ops, void *priv) > } > > /** > - * This function allocates scheduler-specific data for a physical CPU > - * > - * We do not actually make use of any per-CPU 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_pdata(const struct scheduler *ops, int cpu) > -{ > - /* return a non-NULL value to keep schedule.c happy */ > - return SCHED_PRIV(ops); > -} > - > -/** > - * This function frees scheduler-specific data for a physical CPU > - * > - * @param ops Pointer to this instance of the scheduler structure > - */ > -static void > -a653sched_free_pdata(const struct scheduler *ops, void *pcpu, int cpu) > -{ > - /* nop */ > -} > - > -/** > * This function allocates scheduler-specific data for a domain > * > * We do not actually make use of any per-domain data but the hypervisor > @@ -737,9 +709,6 @@ static const struct scheduler sched_arinc653_def = { > .free_vdata = a653sched_free_vdata, > .alloc_vdata = a653sched_alloc_vdata, > > - .free_pdata = a653sched_free_pdata, > - .alloc_pdata = a653sched_alloc_pdata, > - > .free_domdata = a653sched_free_domdata, > .alloc_domdata = a653sched_alloc_domdata, > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index 305889a..288749f 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -532,12 +532,12 @@ csched_alloc_pdata(const struct scheduler *ops, int cpu) > /* Allocate per-PCPU info */ > spc = xzalloc(struct csched_pcpu); > if ( spc == NULL ) > - return NULL; > + return ERR_PTR(-ENOMEM); > > if ( !alloc_cpumask_var(&spc->balance_mask) ) > { > xfree(spc); > - return NULL; > + return ERR_PTR(-ENOMEM); > } > > spin_lock_irqsave(&prv->lock, flags); > diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c > index 7ddad38..36dc9ee 100644 > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -2044,7 +2044,7 @@ csched2_alloc_pdata(const struct scheduler *ops, int cpu) > printk("%s: cpu %d not online yet, deferring initializatgion\n", > __func__, cpu); > > - return (void *)1; > + return NULL; > } > > static void > diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c > index d98bfb6..ac8019f 100644 > --- a/xen/common/sched_rt.c > +++ b/xen/common/sched_rt.c > @@ -665,7 +665,7 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu) > spin_unlock_irqrestore(old_lock, flags); > > if ( !alloc_cpumask_var(&_cpumask_scratch[cpu]) ) > - return NULL; > + return ERR_PTR(-ENOMEM); > > if ( prv->repl_timer == NULL ) > { > @@ -673,13 +673,12 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu) > prv->repl_timer = xzalloc(struct timer); > > if ( prv->repl_timer == NULL ) > - return NULL; > + return ERR_PTR(-ENOMEM); > > init_timer(prv->repl_timer, repl_timer_handler, (void *)ops, cpu); > } > > - /* 1 indicates alloc. succeed in schedule.c */ > - return (void *)1; > + return NULL; > } > > static void > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index 0627eb5..1adc0e2 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int cpu) > if ( idle_vcpu[cpu] == NULL ) > return -ENOMEM; > > - if ( (ops.alloc_pdata != NULL) && > - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) ) > - return -ENOMEM; > + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); > + if ( IS_ERR(sd->sched_priv) ) > + return PTR_ERR(sd->sched_priv); Calling xfree() with an IS_ERR() value might be a bad idea. Either you need to set sd->sched_priv to NULL in error case or you modify xfree() to return immediately not only in the NULL case, but in the IS_ERR() case as well. Juergen > > return 0; > } > @@ -1503,8 +1503,7 @@ static void cpu_schedule_down(unsigned int cpu) > struct schedule_data *sd = &per_cpu(schedule_data, cpu); > struct scheduler *sched = per_cpu(scheduler, cpu); > > - if ( sd->sched_priv != NULL ) > - SCHED_OP(sched, free_pdata, sd->sched_priv, cpu); > + SCHED_OP(sched, free_pdata, sd->sched_priv, cpu); > SCHED_OP(sched, free_vdata, idle_vcpu[cpu]->sched_priv); > > idle_vcpu[cpu]->sched_priv = NULL; > @@ -1599,9 +1598,8 @@ void __init scheduler_init(void) > idle_domain->max_vcpus = nr_cpu_ids; > if ( alloc_vcpu(idle_domain, 0, 0) == NULL ) > BUG(); > - if ( ops.alloc_pdata && > - !(this_cpu(schedule_data).sched_priv = ops.alloc_pdata(&ops, 0)) ) > - BUG(); > + this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0); > + BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv)); > SCHED_OP(&ops, init_pdata, this_cpu(schedule_data).sched_priv, 0); > } > > @@ -1644,8 +1642,8 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c) > > idle = idle_vcpu[cpu]; > ppriv = SCHED_OP(new_ops, alloc_pdata, cpu); > - if ( ppriv == NULL ) > - return -ENOMEM; > + if ( IS_ERR(ppriv) ) > + return PTR_ERR(ppriv); > SCHED_OP(new_ops, init_pdata, ppriv, cpu); > vpriv = SCHED_OP(new_ops, alloc_vdata, idle, idle->domain->sched_priv); > if ( vpriv == NULL ) > diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h > index 70c08c6..560cba5 100644 > --- a/xen/include/xen/sched-if.h > +++ b/xen/include/xen/sched-if.h > @@ -9,6 +9,7 @@ > #define __XEN_SCHED_IF_H__ > > #include <xen/percpu.h> > +#include <xen/err.h> > > /* A global pointer to the initial cpupool (POOL0). */ > extern struct cpupool *cpupool0; > >
>>> On 21.03.16 at 15:48, <JGross@suse.com> wrote: > On 18/03/16 20:04, Dario Faggioli wrote: >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int cpu) >> if ( idle_vcpu[cpu] == NULL ) >> return -ENOMEM; >> >> - if ( (ops.alloc_pdata != NULL) && >> - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) ) >> - return -ENOMEM; >> + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); >> + if ( IS_ERR(sd->sched_priv) ) >> + return PTR_ERR(sd->sched_priv); > > Calling xfree() with an IS_ERR() value might be a bad idea. > Either you need to set sd->sched_priv to NULL in error case or you > modify xfree() to return immediately not only in the NULL case, but > in the IS_ERR() case as well. The latter option is a no-go imo. Jan
On 21/03/16 14:22, Jan Beulich wrote: >>>> On 18.03.16 at 20:04, <dario.faggioli@citrix.com> wrote: >> --- a/xen/include/xen/sched-if.h >> +++ b/xen/include/xen/sched-if.h >> @@ -9,6 +9,7 @@ >> #define __XEN_SCHED_IF_H__ >> >> #include <xen/percpu.h> >> +#include <xen/err.h> >> >> /* A global pointer to the initial cpupool (POOL0). */ >> extern struct cpupool *cpupool0; > > There is no visible use in this header of what err.h defines - why > does it get included all of the sudden? I'm guessing it's so that all the files that use the scheduler interface automatically get IS_ERR and PTR_ERR without having to include xen/err.h directly. But of course that means files like sched_arinc653.c and sched_credit2.c end up including xen/err.c even though they don't use those macros. Would you prefer the other files include it directly instead? -George
On 18/03/16 19:04, Dario Faggioli wrote: > The .alloc_pdata scheduler hook must, before this change, > be implemented by all schedulers --even those ones that > don't need to allocate anything. > > Make it possible to just use the SCHED_OP(), like for > the other hooks, by using ERR_PTR() and IS_ERR() for > error reporting. This: > - makes NULL a variant of success; > - allows for errors other than ENOMEM to be properly > communicated (if ever necessary). > > This, in turn, means that schedulers not needing to > allocate any per-pCPU data, can avoid implementing the > hook. In fact, the artificial implementation of > .alloc_pdata in the ARINC653 is removed (and, while there, > nuke .free_pdata too, as it is equally useless). > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> With the xfree issue Juergen pointed out fixed, this looks good to me. -George
>>> On 23.03.16 at 18:36, <george.dunlap@citrix.com> wrote: > On 21/03/16 14:22, Jan Beulich wrote: >>>>> On 18.03.16 at 20:04, <dario.faggioli@citrix.com> wrote: >>> --- a/xen/include/xen/sched-if.h >>> +++ b/xen/include/xen/sched-if.h >>> @@ -9,6 +9,7 @@ >>> #define __XEN_SCHED_IF_H__ >>> >>> #include <xen/percpu.h> >>> +#include <xen/err.h> >>> >>> /* A global pointer to the initial cpupool (POOL0). */ >>> extern struct cpupool *cpupool0; >> >> There is no visible use in this header of what err.h defines - why >> does it get included all of the sudden? > > I'm guessing it's so that all the files that use the scheduler interface > automatically get IS_ERR and PTR_ERR without having to include xen/err.h > directly. > > But of course that means files like sched_arinc653.c and sched_credit2.c > end up including xen/err.c even though they don't use those macros. > Would you prefer the other files include it directly instead? Since there are other files than just xen/common/sched*.c which include this header - yes, I'd prefer that. In general I think that avoiding the need to include headers needed in multiple sources is fine for private headers (i.e. such outside of xen/include/), but isn't in ones available for general consumption. Jan
On Thu, 2016-03-24 at 03:43 -0600, Jan Beulich wrote: > > > > On 23.03.16 at 18:36, <george.dunlap@citrix.com> wrote: > > > > But of course that means files like sched_arinc653.c and > > sched_credit2.c > > end up including xen/err.c even though they don't use those macros. > > Would you prefer the other files include it directly instead? > Since there are other files than just xen/common/sched*.c which > include this header - yes, I'd prefer that. > Ok. Thanks and Regards, Dario
On Mon, 2016-03-21 at 09:07 -0600, Jan Beulich wrote: > > > > On 21.03.16 at 15:48, <JGross@suse.com> wrote: > > On 18/03/16 20:04, Dario Faggioli wrote: > > > @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int > > > cpu) > > > if ( idle_vcpu[cpu] == NULL ) > > > return -ENOMEM; > > > > > > - if ( (ops.alloc_pdata != NULL) && > > > - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) > > > ) > > > - return -ENOMEM; > > > + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); > > > + if ( IS_ERR(sd->sched_priv) ) > > > + return PTR_ERR(sd->sched_priv); > > Calling xfree() with an IS_ERR() value might be a bad idea. > > Either you need to set sd->sched_priv to NULL in error case or you > > modify xfree() to return immediately not only in the NULL case, but > > in the IS_ERR() case as well. > The latter option is a no-go imo. > Ok, I'll do: sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); if ( IS_ERR(sd->sched_priv) ) { int err = PTR_ERR(sd->sched_priv); sd->sched_priv = NULL; return err; } Is this ok? And, just to be sure I got your point (Juergen), you're referring to the situation where cpu_schedule_up() fails, we go to CPU_UP_CANCELLED, which calls cpu_schedule_donw(), which calls free_pdata on sd->sched_priv (inside which we may reach an xfree()), aren't you? In fact, alloc_pdata is also called in schedule_cpu_switch(), but in that case, I don't see anyone calling xfree() if alloc_pdata fails... Am I missing it? Thanks and Regards, Dario -- <<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)
On 01/04/16 19:01, Dario Faggioli wrote: > On Mon, 2016-03-21 at 09:07 -0600, Jan Beulich wrote: >>>>> On 21.03.16 at 15:48, <JGross@suse.com> wrote: >>> On 18/03/16 20:04, Dario Faggioli wrote: >>>> @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int >>>> cpu) >>>> if ( idle_vcpu[cpu] == NULL ) >>>> return -ENOMEM; >>>> >>>> - if ( (ops.alloc_pdata != NULL) && >>>> - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) >>>> ) >>>> - return -ENOMEM; >>>> + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); >>>> + if ( IS_ERR(sd->sched_priv) ) >>>> + return PTR_ERR(sd->sched_priv); >>> Calling xfree() with an IS_ERR() value might be a bad idea. >>> Either you need to set sd->sched_priv to NULL in error case or you >>> modify xfree() to return immediately not only in the NULL case, but >>> in the IS_ERR() case as well. >> The latter option is a no-go imo. >> > Ok, I'll do: > > sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); > if ( IS_ERR(sd->sched_priv) ) > { > int err = PTR_ERR(sd->sched_priv); > > sd->sched_priv = NULL; > return err; > } > > Is this ok? Yes, that's what I would prefer. > And, just to be sure I got your point (Juergen), you're referring to > the situation where cpu_schedule_up() fails, we go to CPU_UP_CANCELLED, > which calls cpu_schedule_donw(), which calls free_pdata on > sd->sched_priv (inside which we may reach an xfree()), aren't you? Basically, yes. > In fact, alloc_pdata is also called in schedule_cpu_switch(), but in > that case, I don't see anyone calling xfree() if alloc_pdata fails... > Am I missing it? I just want to avoid the situation where sched_priv would contain a non-NULL value not pointing to an allocated area. That's always dangerous, even if with current code nothing bad might happen. Juergen
>>> On 01.04.16 at 19:01, <dario.faggioli@citrix.com> wrote: > On Mon, 2016-03-21 at 09:07 -0600, Jan Beulich wrote: >> > > > On 21.03.16 at 15:48, <JGross@suse.com> wrote: >> > On 18/03/16 20:04, Dario Faggioli wrote: >> > > @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int >> > > cpu) >> > > if ( idle_vcpu[cpu] == NULL ) >> > > return -ENOMEM; >> > > >> > > - if ( (ops.alloc_pdata != NULL) && >> > > - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) >> > > ) >> > > - return -ENOMEM; >> > > + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); >> > > + if ( IS_ERR(sd->sched_priv) ) >> > > + return PTR_ERR(sd->sched_priv); >> > Calling xfree() with an IS_ERR() value might be a bad idea. >> > Either you need to set sd->sched_priv to NULL in error case or you >> > modify xfree() to return immediately not only in the NULL case, but >> > in the IS_ERR() case as well. >> The latter option is a no-go imo. >> > Ok, I'll do: > > sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); > if ( IS_ERR(sd->sched_priv) ) > { > int err = PTR_ERR(sd->sched_priv); > > sd->sched_priv = NULL; > return err; > } > > Is this ok? Depends: Can ->sched_priv be looked at by another CPU between the first and second assignments? If yes, you need to use an intermediary local variable. Jan
On Mon, 2016-04-04 at 00:13 -0600, Jan Beulich wrote: > > > > On 01.04.16 at 19:01, <dario.faggioli@citrix.com> wrote: > > Ok, I'll do: > > > > sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); > > if ( IS_ERR(sd->sched_priv) ) > > { > > int err = PTR_ERR(sd->sched_priv); > > > > sd->sched_priv = NULL; > > return err; > > } > > > > Is this ok? > Depends: Can ->sched_priv be looked at by another CPU between > the first and second assignments? If yes, you need to use an > intermediary local variable. > Mmm... good point. I don't see this happening, but using a local variable is more "defensive programming oriented", so I'm going for it. Regards, Dario
diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c index 8a11a2f..b79fcdf 100644 --- a/xen/common/sched_arinc653.c +++ b/xen/common/sched_arinc653.c @@ -456,34 +456,6 @@ a653sched_free_vdata(const struct scheduler *ops, void *priv) } /** - * This function allocates scheduler-specific data for a physical CPU - * - * We do not actually make use of any per-CPU 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_pdata(const struct scheduler *ops, int cpu) -{ - /* return a non-NULL value to keep schedule.c happy */ - return SCHED_PRIV(ops); -} - -/** - * This function frees scheduler-specific data for a physical CPU - * - * @param ops Pointer to this instance of the scheduler structure - */ -static void -a653sched_free_pdata(const struct scheduler *ops, void *pcpu, int cpu) -{ - /* nop */ -} - -/** * This function allocates scheduler-specific data for a domain * * We do not actually make use of any per-domain data but the hypervisor @@ -737,9 +709,6 @@ static const struct scheduler sched_arinc653_def = { .free_vdata = a653sched_free_vdata, .alloc_vdata = a653sched_alloc_vdata, - .free_pdata = a653sched_free_pdata, - .alloc_pdata = a653sched_alloc_pdata, - .free_domdata = a653sched_free_domdata, .alloc_domdata = a653sched_alloc_domdata, diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index 305889a..288749f 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -532,12 +532,12 @@ csched_alloc_pdata(const struct scheduler *ops, int cpu) /* Allocate per-PCPU info */ spc = xzalloc(struct csched_pcpu); if ( spc == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); if ( !alloc_cpumask_var(&spc->balance_mask) ) { xfree(spc); - return NULL; + return ERR_PTR(-ENOMEM); } spin_lock_irqsave(&prv->lock, flags); diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 7ddad38..36dc9ee 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -2044,7 +2044,7 @@ csched2_alloc_pdata(const struct scheduler *ops, int cpu) printk("%s: cpu %d not online yet, deferring initializatgion\n", __func__, cpu); - return (void *)1; + return NULL; } static void diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c index d98bfb6..ac8019f 100644 --- a/xen/common/sched_rt.c +++ b/xen/common/sched_rt.c @@ -665,7 +665,7 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu) spin_unlock_irqrestore(old_lock, flags); if ( !alloc_cpumask_var(&_cpumask_scratch[cpu]) ) - return NULL; + return ERR_PTR(-ENOMEM); if ( prv->repl_timer == NULL ) { @@ -673,13 +673,12 @@ rt_alloc_pdata(const struct scheduler *ops, int cpu) prv->repl_timer = xzalloc(struct timer); if ( prv->repl_timer == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); init_timer(prv->repl_timer, repl_timer_handler, (void *)ops, cpu); } - /* 1 indicates alloc. succeed in schedule.c */ - return (void *)1; + return NULL; } static void diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 0627eb5..1adc0e2 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1491,9 +1491,9 @@ static int cpu_schedule_up(unsigned int cpu) if ( idle_vcpu[cpu] == NULL ) return -ENOMEM; - if ( (ops.alloc_pdata != NULL) && - ((sd->sched_priv = ops.alloc_pdata(&ops, cpu)) == NULL) ) - return -ENOMEM; + sd->sched_priv = SCHED_OP(&ops, alloc_pdata, cpu); + if ( IS_ERR(sd->sched_priv) ) + return PTR_ERR(sd->sched_priv); return 0; } @@ -1503,8 +1503,7 @@ static void cpu_schedule_down(unsigned int cpu) struct schedule_data *sd = &per_cpu(schedule_data, cpu); struct scheduler *sched = per_cpu(scheduler, cpu); - if ( sd->sched_priv != NULL ) - SCHED_OP(sched, free_pdata, sd->sched_priv, cpu); + SCHED_OP(sched, free_pdata, sd->sched_priv, cpu); SCHED_OP(sched, free_vdata, idle_vcpu[cpu]->sched_priv); idle_vcpu[cpu]->sched_priv = NULL; @@ -1599,9 +1598,8 @@ void __init scheduler_init(void) idle_domain->max_vcpus = nr_cpu_ids; if ( alloc_vcpu(idle_domain, 0, 0) == NULL ) BUG(); - if ( ops.alloc_pdata && - !(this_cpu(schedule_data).sched_priv = ops.alloc_pdata(&ops, 0)) ) - BUG(); + this_cpu(schedule_data).sched_priv = SCHED_OP(&ops, alloc_pdata, 0); + BUG_ON(IS_ERR(this_cpu(schedule_data).sched_priv)); SCHED_OP(&ops, init_pdata, this_cpu(schedule_data).sched_priv, 0); } @@ -1644,8 +1642,8 @@ int schedule_cpu_switch(unsigned int cpu, struct cpupool *c) idle = idle_vcpu[cpu]; ppriv = SCHED_OP(new_ops, alloc_pdata, cpu); - if ( ppriv == NULL ) - return -ENOMEM; + if ( IS_ERR(ppriv) ) + return PTR_ERR(ppriv); SCHED_OP(new_ops, init_pdata, ppriv, cpu); vpriv = SCHED_OP(new_ops, alloc_vdata, idle, idle->domain->sched_priv); if ( vpriv == NULL ) diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h index 70c08c6..560cba5 100644 --- a/xen/include/xen/sched-if.h +++ b/xen/include/xen/sched-if.h @@ -9,6 +9,7 @@ #define __XEN_SCHED_IF_H__ #include <xen/percpu.h> +#include <xen/err.h> /* A global pointer to the initial cpupool (POOL0). */ extern struct cpupool *cpupool0;
The .alloc_pdata scheduler hook must, before this change, be implemented by all schedulers --even those ones that don't need to allocate anything. Make it possible to just use the SCHED_OP(), like for the other hooks, by using ERR_PTR() and IS_ERR() for error reporting. This: - makes NULL a variant of success; - allows for errors other than ENOMEM to be properly communicated (if ever necessary). This, in turn, means that schedulers not needing to allocate any per-pCPU data, can avoid implementing the hook. In fact, the artificial implementation of .alloc_pdata in the ARINC653 is removed (and, while there, nuke .free_pdata too, as it is equally useless). Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Robert VanVossen <robert.vanvossen@dornerworks.com> Cc: Josh Whitehead <josh.whitehead@dornerworks.com> Cc: Meng Xu <mengxu@cis.upenn.edu> Cc: Jan Beulich <JBeulich@suse.com> Cc: Juergen Gross <jgross@suse.com> --- Changes from v1: * use IS_ERR() and friends to deal with the return value of alloc_pdata, as suggested during review. --- xen/common/sched_arinc653.c | 31 ------------------------------- xen/common/sched_credit.c | 4 ++-- xen/common/sched_credit2.c | 2 +- xen/common/sched_rt.c | 7 +++---- xen/common/schedule.c | 18 ++++++++---------- xen/include/xen/sched-if.h | 1 + 6 files changed, 15 insertions(+), 48 deletions(-)