| Message ID | 1560244837-31477-1-git-send-email-chenbaodong@mxnavi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: superficial clean-ups | expand |
On 11/06/2019 10:20, Baodong Chen wrote: > * Remove redundant set 'DOMDYING_dead' > domain_create() will set this when fail, thus no need > set in arch_domain_create(). Its not redundant. It is necessary for correct cleanup. All of this logic will be rewritten when the destroy paths are fully idempotent, and while ARM is fairing well in this regard, the common and x86 needs more work. ~Andrew
On 6/11/19 10:20 AM, Baodong Chen wrote: > * Remove redundant set 'DOMDYING_dead' > domain_create() will set this when fail, thus no need > set in arch_domain_create(). > > * Set error when really happened > diff --git a/xen/common/schedule.c b/xen/common/schedule.c > index 86341bc..d6cdcf8 100644 > --- a/xen/common/schedule.c > +++ b/xen/common/schedule.c > @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) > return NULL; > > found: > - *perr = -ENOMEM; > if ( (sched = xmalloc(struct scheduler)) == NULL ) > + { > + *perr = -ENOMEM; > return NULL; > + } > memcpy(sched, schedulers[i], sizeof(*sched)); > if ( (*perr = SCHED_OP(sched, init)) != 0 ) I was going to say, this is a common idiom in the Xen code, and the compiler will end up re-organizing things such that the write doesn't happen anyway. But in this case, its' actually writing through a pointer before and after a function call; I don't think the compiler would actually be allowed to optimize this write away. So, I guess I'd be OK with this particular hunk. Dario, any opinions? -George
On 11.06.19 12:18, George Dunlap wrote: > On 6/11/19 10:20 AM, Baodong Chen wrote: >> * Remove redundant set 'DOMDYING_dead' >> domain_create() will set this when fail, thus no need >> set in arch_domain_create(). >> >> * Set error when really happened > >> diff --git a/xen/common/schedule.c b/xen/common/schedule.c >> index 86341bc..d6cdcf8 100644 >> --- a/xen/common/schedule.c >> +++ b/xen/common/schedule.c >> @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) >> return NULL; >> >> found: >> - *perr = -ENOMEM; >> if ( (sched = xmalloc(struct scheduler)) == NULL ) >> + { >> + *perr = -ENOMEM; >> return NULL; >> + } >> memcpy(sched, schedulers[i], sizeof(*sched)); >> if ( (*perr = SCHED_OP(sched, init)) != 0 ) > > I was going to say, this is a common idiom in the Xen code, and the > compiler will end up re-organizing things such that the write doesn't > happen anyway. But in this case, its' actually writing through a > pointer before and after a function call; I don't think the compiler > would actually be allowed to optimize this write away. > > So, I guess I'd be OK with this particular hunk. Dario, any opinions? I'd rather switch to PTR_ERR() here dropping the perr parameter. Juergen
On 11/06/2019 11:25, Juergen Gross wrote: > On 11.06.19 12:18, George Dunlap wrote: >> On 6/11/19 10:20 AM, Baodong Chen wrote: >>> * Remove redundant set 'DOMDYING_dead' >>> domain_create() will set this when fail, thus no need >>> set in arch_domain_create(). >>> >>> * Set error when really happened >> >>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c >>> index 86341bc..d6cdcf8 100644 >>> --- a/xen/common/schedule.c >>> +++ b/xen/common/schedule.c >>> @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned >>> int sched_id, int *perr) >>> return NULL; >>> found: >>> - *perr = -ENOMEM; >>> if ( (sched = xmalloc(struct scheduler)) == NULL ) >>> + { >>> + *perr = -ENOMEM; >>> return NULL; >>> + } >>> memcpy(sched, schedulers[i], sizeof(*sched)); >>> if ( (*perr = SCHED_OP(sched, init)) != 0 ) >> >> I was going to say, this is a common idiom in the Xen code, and the >> compiler will end up re-organizing things such that the write doesn't >> happen anyway. But in this case, its' actually writing through a >> pointer before and after a function call; I don't think the compiler >> would actually be allowed to optimize this write away. >> >> So, I guess I'd be OK with this particular hunk. Dario, any opinions? > > I'd rather switch to PTR_ERR() here dropping the perr parameter. +2 for this, but I was going to wait until core scheduling had gotten a bit further before suggesting cleanup which is guaranteed to collide. Sadly, it's fairly intrusive in the cpupool code as well. ~Andrew
On 11.06.19 12:27, Andrew Cooper wrote: > On 11/06/2019 11:25, Juergen Gross wrote: >> On 11.06.19 12:18, George Dunlap wrote: >>> On 6/11/19 10:20 AM, Baodong Chen wrote: >>>> * Remove redundant set 'DOMDYING_dead' >>>> domain_create() will set this when fail, thus no need >>>> set in arch_domain_create(). >>>> >>>> * Set error when really happened >>> >>>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c >>>> index 86341bc..d6cdcf8 100644 >>>> --- a/xen/common/schedule.c >>>> +++ b/xen/common/schedule.c >>>> @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned >>>> int sched_id, int *perr) >>>> return NULL; >>>> found: >>>> - *perr = -ENOMEM; >>>> if ( (sched = xmalloc(struct scheduler)) == NULL ) >>>> + { >>>> + *perr = -ENOMEM; >>>> return NULL; >>>> + } >>>> memcpy(sched, schedulers[i], sizeof(*sched)); >>>> if ( (*perr = SCHED_OP(sched, init)) != 0 ) >>> >>> I was going to say, this is a common idiom in the Xen code, and the >>> compiler will end up re-organizing things such that the write doesn't >>> happen anyway. But in this case, its' actually writing through a >>> pointer before and after a function call; I don't think the compiler >>> would actually be allowed to optimize this write away. >>> >>> So, I guess I'd be OK with this particular hunk. Dario, any opinions? >> >> I'd rather switch to PTR_ERR() here dropping the perr parameter. > > +2 for this, but I was going to wait until core scheduling had gotten a > bit further before suggesting cleanup which is guaranteed to collide. > > Sadly, it's fairly intrusive in the cpupool code as well. I can add this to my list of scheduler cleanups to do. Juergen
On 6/11/19 17:45, Andrew Cooper wrote: > On 11/06/2019 10:20, Baodong Chen wrote: >> * Remove redundant set 'DOMDYING_dead' >> domain_create() will set this when fail, thus no need >> set in arch_domain_create(). > Its not redundant. It is necessary for correct cleanup. Hello Andrew, Thanks for your comments. Your concern is: when the arch_domain_create() fails, some cleanup work need to done in this function. and 'DOMDYING_dead' flags maybe needed to judge for correct cleanup? If so, it's not redundant. I'm curious why 'DOMDYING_dead' been set by fail path both in arch_domain_create() and domain_create(). > > All of this logic will be rewritten when the destroy paths are fully > idempotent, and while ARM is fairing well in this regard, the common and > x86 needs more work. > > ~Andrew > > . >
On Tue, 2019-06-11 at 12:25 +0200, Juergen Gross wrote: > On 11.06.19 12:18, George Dunlap wrote: > > On 6/11/19 10:20 AM, Baodong Chen wrote: > > > --- a/xen/common/schedule.c > > > +++ b/xen/common/schedule.c > > > @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned > > > int sched_id, int *perr) > > > return NULL; > > > > > > found: > > > - *perr = -ENOMEM; > > > if ( (sched = xmalloc(struct scheduler)) == NULL ) > > > + { > > > + *perr = -ENOMEM; > > > return NULL; > > > + } > > > memcpy(sched, schedulers[i], sizeof(*sched)); > > > if ( (*perr = SCHED_OP(sched, init)) != 0 ) > > > > I was going to say, this is a common idiom in the Xen code, and the > > compiler will end up re-organizing things such that the write > > doesn't > > happen anyway. But in this case, its' actually writing through a > > pointer before and after a function call; I don't think the > > compiler > > would actually be allowed to optimize this write away. > > > > So, I guess I'd be OK with this particular hunk. Dario, any > > opinions? > I'm ok with it too, but... > I'd rather switch to PTR_ERR() here dropping the perr parameter. > ... I'd be even more ok with this! :-) Regards
On 11/06/2019 11:33, chenbaodong wrote: > > On 6/11/19 17:45, Andrew Cooper wrote: >> On 11/06/2019 10:20, Baodong Chen wrote: >>> * Remove redundant set 'DOMDYING_dead' >>> domain_create() will set this when fail, thus no need >>> set in arch_domain_create(). >> Its not redundant. It is necessary for correct cleanup. > > Hello Andrew, > > Thanks for your comments. > > Your concern is: when the arch_domain_create() fails, > > some cleanup work need to done in this function. > > and 'DOMDYING_dead' flags maybe needed to judge for correct cleanup? > > If so, it's not redundant. > > I'm curious why 'DOMDYING_dead' been set by fail path both in > arch_domain_create() > > and domain_create(). Because various cleanup paths BUG_ON(!d->is_dying), including ones before hitting the main error path in domain_create(). ~Andrew
On 6/11/19 18:53, Andrew Cooper wrote: > On 11/06/2019 11:33, chenbaodong wrote: >> On 6/11/19 17:45, Andrew Cooper wrote: >>> On 11/06/2019 10:20, Baodong Chen wrote: >>>> * Remove redundant set 'DOMDYING_dead' >>>> domain_create() will set this when fail, thus no need >>>> set in arch_domain_create(). >>> Its not redundant. It is necessary for correct cleanup. >> Hello Andrew, >> >> Thanks for your comments. >> >> Your concern is: when the arch_domain_create() fails, >> >> some cleanup work need to done in this function. >> >> and 'DOMDYING_dead' flags maybe needed to judge for correct cleanup? >> >> If so, it's not redundant. >> >> I'm curious why 'DOMDYING_dead' been set by fail path both in >> arch_domain_create() >> >> and domain_create(). > Because various cleanup paths BUG_ON(!d->is_dying), including ones > before hitting the main error path in domain_create(). Thanks for clarify, my fault, i missed (!d->is_dying) related check. And tested by force to run fail path in arch_domain_create(), but nothing happed in arch_domain_destory(). result is: Panic on CPU 0: Error creating domain 0 Seems the cleanup path run with success. Anyway, can leave it as what it was. > ~Andrew > . >
On 6/11/19 18:29, Juergen Gross wrote: > On 11.06.19 12:27, Andrew Cooper wrote: >> On 11/06/2019 11:25, Juergen Gross wrote: >>> On 11.06.19 12:18, George Dunlap wrote: >>>> On 6/11/19 10:20 AM, Baodong Chen wrote: >>>>> * Remove redundant set 'DOMDYING_dead' >>>>> domain_create() will set this when fail, thus no need >>>>> set in arch_domain_create(). >>>>> >>>>> * Set error when really happened >>>> >>>>> diff --git a/xen/common/schedule.c b/xen/common/schedule.c >>>>> index 86341bc..d6cdcf8 100644 >>>>> --- a/xen/common/schedule.c >>>>> +++ b/xen/common/schedule.c >>>>> @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned >>>>> int sched_id, int *perr) >>>>> return NULL; >>>>> found: >>>>> - *perr = -ENOMEM; >>>>> if ( (sched = xmalloc(struct scheduler)) == NULL ) >>>>> + { >>>>> + *perr = -ENOMEM; >>>>> return NULL; >>>>> + } >>>>> memcpy(sched, schedulers[i], sizeof(*sched)); >>>>> if ( (*perr = SCHED_OP(sched, init)) != 0 ) >>>> >>>> I was going to say, this is a common idiom in the Xen code, and the >>>> compiler will end up re-organizing things such that the write doesn't >>>> happen anyway. But in this case, its' actually writing through a >>>> pointer before and after a function call; I don't think the compiler >>>> would actually be allowed to optimize this write away. >>>> >>>> So, I guess I'd be OK with this particular hunk. Dario, any opinions? >>> >>> I'd rather switch to PTR_ERR() here dropping the perr parameter. >> >> +2 for this, but I was going to wait until core scheduling had gotten a >> bit further before suggesting cleanup which is guaranteed to collide. >> >> Sadly, it's fairly intrusive in the cpupool code as well. > > I can add this to my list of scheduler cleanups to do. Copy that. > > > Juergen > . >
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index ff330b3..c72be08 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -731,7 +731,6 @@ int arch_domain_create(struct domain *d, return 0; fail: - d->is_dying = DOMDYING_dead; arch_domain_destroy(d); return rc; diff --git a/xen/common/domain.c b/xen/common/domain.c index 90c6607..a6af5a6 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -358,10 +358,9 @@ struct domain *domain_create(domid_t domid, */ if ( !is_system_domain(d) ) { - err = -ENOMEM; d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus); if ( !d->vcpu ) - goto fail; + goto no_mem; d->max_vcpus = config->max_vcpus; } @@ -389,9 +388,8 @@ struct domain *domain_create(domid_t domid, rwlock_init(&d->vnuma_rwlock); - err = -ENOMEM; if ( !zalloc_cpumask_var(&d->dirty_cpumask) ) - goto fail; + goto no_mem; rangeset_domain_initialise(d); @@ -429,7 +427,7 @@ struct domain *domain_create(domid_t domid, d->iomem_caps = rangeset_new(d, "I/O Memory", RANGESETF_prettyprint_hex); d->irq_caps = rangeset_new(d, "Interrupts", 0); if ( !d->iomem_caps || !d->irq_caps ) - goto fail; + goto no_mem; if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 ) goto fail; @@ -449,11 +447,9 @@ struct domain *domain_create(domid_t domid, if ( (err = argo_init(d)) != 0 ) goto fail; - err = -ENOMEM; - d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE); if ( !d->pbuf ) - goto fail; + goto no_mem; if ( (err = sched_init_domain(d, 0)) != 0 ) goto fail; @@ -482,6 +478,9 @@ struct domain *domain_create(domid_t domid, return d; + no_mem: + err = -ENOMEM; + fail: ASSERT(err < 0); /* Sanity check paths leading here. */ err = err ?: -EILSEQ; /* Release build safety. */ diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 86341bc..d6cdcf8 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -1894,9 +1894,11 @@ struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr) return NULL; found: - *perr = -ENOMEM; if ( (sched = xmalloc(struct scheduler)) == NULL ) + { + *perr = -ENOMEM; return NULL; + } memcpy(sched, schedulers[i], sizeof(*sched)); if ( (*perr = SCHED_OP(sched, init)) != 0 ) {
* Remove redundant set 'DOMDYING_dead' domain_create() will set this when fail, thus no need set in arch_domain_create(). * Set error when really happened Signed-off-by: Baodong Chen <chenbaodong@mxnavi.com> --- xen/arch/arm/domain.c | 1 - xen/common/domain.c | 15 +++++++-------- xen/common/schedule.c | 4 +++- 3 files changed, 10 insertions(+), 10 deletions(-)