| Message ID | 20201201082128.15239-7-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: support per-cpupool scheduling granularity | expand |
On 01.12.2020 09:21, Juergen Gross wrote: > Instead of a pointer to an error variable as parameter just use > ERR_PTR() to return the cause of an error in cpupool_create(). > > This propagates to scheduler_alloc(), too. > > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one small question: > --- a/xen/common/sched/core.c > +++ b/xen/common/sched/core.c > @@ -3233,26 +3233,25 @@ struct scheduler *scheduler_get_default(void) > return &ops; > } > > -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) > +struct scheduler *scheduler_alloc(unsigned int sched_id) > { > int i; > + int ret; I guess you didn't merge this with i's declaration because of a plan/hope for i to be converted to unsigned int? Jan
On 02.12.20 09:58, Jan Beulich wrote: > On 01.12.2020 09:21, Juergen Gross wrote: >> Instead of a pointer to an error variable as parameter just use >> ERR_PTR() to return the cause of an error in cpupool_create(). >> >> This propagates to scheduler_alloc(), too. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one small question: > >> --- a/xen/common/sched/core.c >> +++ b/xen/common/sched/core.c >> @@ -3233,26 +3233,25 @@ struct scheduler *scheduler_get_default(void) >> return &ops; >> } >> >> -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) >> +struct scheduler *scheduler_alloc(unsigned int sched_id) >> { >> int i; >> + int ret; > > I guess you didn't merge this with i's declaration because of a > plan/hope for i to be converted to unsigned int? The main reason is I don't like overloading variables this way. Any sane compiler will do that for me as it will discover that the two variables are not alive at the same time, so the generated code should be the same, while the written code stays more readable this way. Juergen
On 02.12.2020 10:56, Jürgen Groß wrote: > On 02.12.20 09:58, Jan Beulich wrote: >> On 01.12.2020 09:21, Juergen Gross wrote: >>> Instead of a pointer to an error variable as parameter just use >>> ERR_PTR() to return the cause of an error in cpupool_create(). >>> >>> This propagates to scheduler_alloc(), too. >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with one small question: >> >>> --- a/xen/common/sched/core.c >>> +++ b/xen/common/sched/core.c >>> @@ -3233,26 +3233,25 @@ struct scheduler *scheduler_get_default(void) >>> return &ops; >>> } >>> >>> -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) >>> +struct scheduler *scheduler_alloc(unsigned int sched_id) >>> { >>> int i; >>> + int ret; >> >> I guess you didn't merge this with i's declaration because of a >> plan/hope for i to be converted to unsigned int? > > The main reason is I don't like overloading variables this way. That's no what I asked. I was wondering why the new decl wasn't int i, ret; Jan
On 02.12.20 11:46, Jan Beulich wrote: > On 02.12.2020 10:56, Jürgen Groß wrote: >> On 02.12.20 09:58, Jan Beulich wrote: >>> On 01.12.2020 09:21, Juergen Gross wrote: >>>> Instead of a pointer to an error variable as parameter just use >>>> ERR_PTR() to return the cause of an error in cpupool_create(). >>>> >>>> This propagates to scheduler_alloc(), too. >>>> >>>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> with one small question: >>> >>>> --- a/xen/common/sched/core.c >>>> +++ b/xen/common/sched/core.c >>>> @@ -3233,26 +3233,25 @@ struct scheduler *scheduler_get_default(void) >>>> return &ops; >>>> } >>>> >>>> -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) >>>> +struct scheduler *scheduler_alloc(unsigned int sched_id) >>>> { >>>> int i; >>>> + int ret; >>> >>> I guess you didn't merge this with i's declaration because of a >>> plan/hope for i to be converted to unsigned int? >> >> The main reason is I don't like overloading variables this way. > > That's no what I asked. I was wondering why the new decl wasn't > > int i, ret; Oh, then I completely misunderstood your question, sorry. I had no special reason when writing the patch, but I think changing i to be unsigned is a good idea. I'll do that in the next iteration. Juergen
On Tue, 2020-12-01 at 09:21 +0100, Juergen Gross wrote: > Instead of a pointer to an error variable as parameter just use > ERR_PTR() to return the cause of an error in cpupool_create(). > Yes... And thanks! Happy to see more of this happening and more of the ad-hoc error handling going away. > This propagates to scheduler_alloc(), too. > > Signed-off-by: Juergen Gross <jgross@suse.com> > Reviewed-by: Dario Faggioli <dfaggioli@suse.com> Regards
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index 6063f6d9ea..a429fc7640 100644 --- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -3233,26 +3233,25 @@ struct scheduler *scheduler_get_default(void) return &ops; } -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) +struct scheduler *scheduler_alloc(unsigned int sched_id) { int i; + int ret; struct scheduler *sched; for ( i = 0; i < NUM_SCHEDULERS; i++ ) if ( schedulers[i] && schedulers[i]->sched_id == sched_id ) goto found; - *perr = -ENOENT; - return NULL; + return ERR_PTR(-ENOENT); found: - *perr = -ENOMEM; if ( (sched = xmalloc(struct scheduler)) == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); memcpy(sched, schedulers[i], sizeof(*sched)); - if ( (*perr = sched_init(sched)) != 0 ) + if ( (ret = sched_init(sched)) != 0 ) { xfree(sched); - sched = NULL; + sched = ERR_PTR(ret); } return sched; diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c index 714cd47ae9..0db7d77219 100644 --- a/xen/common/sched/cpupool.c +++ b/xen/common/sched/cpupool.c @@ -240,15 +240,15 @@ void cpupool_put(struct cpupool *pool) * - poolid already used * - unknown scheduler */ -static struct cpupool *cpupool_create( - unsigned int poolid, unsigned int sched_id, int *perr) +static struct cpupool *cpupool_create(unsigned int poolid, + unsigned int sched_id) { struct cpupool *c; struct cpupool *q; + int ret; - *perr = -ENOMEM; if ( (c = alloc_cpupool_struct()) == NULL ) - return NULL; + return ERR_PTR(-ENOMEM); /* One reference for caller, one reference for cpupool_destroy(). */ atomic_set(&c->refcnt, 2); @@ -267,7 +267,7 @@ static struct cpupool *cpupool_create( list_add_tail(&c->list, &q->list); if ( q->cpupool_id == poolid ) { - *perr = -EEXIST; + ret = -EEXIST; goto err; } } @@ -294,15 +294,15 @@ static struct cpupool *cpupool_create( } if ( poolid == 0 ) - { c->sched = scheduler_get_default(); - } else + c->sched = scheduler_alloc(sched_id); + if ( IS_ERR(c->sched) ) { - c->sched = scheduler_alloc(sched_id, perr); - if ( c->sched == NULL ) - goto err; + ret = PTR_ERR(c->sched); + goto err; } + c->sched->cpupool = c; c->gran = opt_sched_granularity; c->sched_gran = sched_granularity; @@ -312,15 +312,16 @@ static struct cpupool *cpupool_create( debugtrace_printk("Created cpupool %u with scheduler %s (%s)\n", c->cpupool_id, c->sched->name, c->sched->opt_name); - *perr = 0; return c; err: list_del(&c->list); spin_unlock(&cpupool_lock); + free_cpupool_struct(c); - return NULL; + + return ERR_PTR(ret); } /* * destroys the given cpupool @@ -767,7 +768,7 @@ static void cpupool_cpu_remove_forced(unsigned int cpu) */ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op) { - int ret; + int ret = 0; struct cpupool *c; switch ( op->op ) @@ -779,8 +780,10 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op) poolid = (op->cpupool_id == XEN_SYSCTL_CPUPOOL_PAR_ANY) ? CPUPOOLID_NONE: op->cpupool_id; - c = cpupool_create(poolid, op->sched_id, &ret); - if ( c != NULL ) + c = cpupool_create(poolid, op->sched_id); + if ( IS_ERR(c) ) + ret = PTR_ERR(c); + else { op->cpupool_id = c->cpupool_id; cpupool_put(c); @@ -1003,12 +1006,11 @@ static struct notifier_block cpu_nfb = { static int __init cpupool_init(void) { unsigned int cpu; - int err; cpupool_gran_init(); - cpupool0 = cpupool_create(0, 0, &err); - BUG_ON(cpupool0 == NULL); + cpupool0 = cpupool_create(0, 0); + BUG_ON(IS_ERR(cpupool0)); cpupool_put(cpupool0); register_cpu_notifier(&cpu_nfb); diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h index 6953cefa6e..92d0d49610 100644 --- a/xen/common/sched/private.h +++ b/xen/common/sched/private.h @@ -597,7 +597,7 @@ void sched_rm_cpu(unsigned int cpu); const cpumask_t *sched_get_opt_cpumask(enum sched_gran opt, unsigned int cpu); void schedule_dump(struct cpupool *c); struct scheduler *scheduler_get_default(void); -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr); +struct scheduler *scheduler_alloc(unsigned int sched_id); void scheduler_free(struct scheduler *sched); int cpu_disable_scheduler(unsigned int cpu); int schedule_cpu_add(unsigned int cpu, struct cpupool *c);
Instead of a pointer to an error variable as parameter just use ERR_PTR() to return the cause of an error in cpupool_create(). This propagates to scheduler_alloc(), too. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new patch --- xen/common/sched/core.c | 13 ++++++------- xen/common/sched/cpupool.c | 38 ++++++++++++++++++++------------------ xen/common/sched/private.h | 2 +- 3 files changed, 27 insertions(+), 26 deletions(-)