diff mbox series

xen: superficial clean-ups

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

Commit Message

chenbaodong June 11, 2019, 9:20 a.m. UTC
* 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(-)

Comments

Andrew Cooper June 11, 2019, 9:45 a.m. UTC | #1
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
George Dunlap June 11, 2019, 10:18 a.m. UTC | #2
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
Jürgen Groß June 11, 2019, 10:25 a.m. UTC | #3
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
Andrew Cooper June 11, 2019, 10:27 a.m. UTC | #4
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
Jürgen Groß June 11, 2019, 10:29 a.m. UTC | #5
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
chenbaodong June 11, 2019, 10:33 a.m. UTC | #6
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
>
> .
>
Dario Faggioli June 11, 2019, 10:34 a.m. UTC | #7
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
Andrew Cooper June 11, 2019, 10:53 a.m. UTC | #8
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
chenbaodong June 12, 2019, 12:51 a.m. UTC | #9
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
> .
>
chenbaodong June 12, 2019, 1:05 a.m. UTC | #10
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 mbox series

Patch

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 )
     {