[Xen-devel] [PATCH] xen: superficial clean-ups

[Xen-devel] [PATCH] xen: superficial clean-ups

[Baodong Chen]

* 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(-)

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[Andrew Cooper]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[chenbaodong]

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
>
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1559 bytes --]

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
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[Andrew Cooper]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[chenbaodong]

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
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen: superficial clean-ups

[chenbaodong]

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
> .
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel