Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: "Julien Grall" <julien.grall@arm.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Jan Beulich" <JBeulich@suse.com>,
	Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH 2/5] xen/domain: Break __domain_destroy() out of domain_create() and complete_domain_destroy()
Date: Mon, 3 Sep 2018 18:05:34 +0100	[thread overview]
Message-ID: <3e614297-0c27-20e6-d939-62a8ed3a2b62@citrix.com> (raw)
In-Reply-To: <20180903170159.cibbzhgmayxtwmbs@zion.uk.xensource.com>

On 03/09/18 18:01, Wei Liu wrote:
> On Mon, Sep 03, 2018 at 05:58:02PM +0100, Andrew Cooper wrote:
>> On 03/09/18 17:54, Wei Liu wrote:
>>> On Mon, Sep 03, 2018 at 03:46:57PM +0100, Andrew Cooper wrote:
>>>> This is the first step in making the destroy path idepotent, and using it in
>>> "idempotent".
>>>
>>>> place of the ad-hoc cleanup paths in the create path.
>>>>
>>>> To begin with, the trivial free operations are broken out.  The rest of the
>>>> cleanup code will be moved as it is demonstrated (or made) to be idempotent.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>>> CC: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>  xen/common/domain.c | 29 ++++++++++++++++++++++-------
>>>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>>> index 43ab926..2253c2d 100644
>>>> --- a/xen/common/domain.c
>>>> +++ b/xen/common/domain.c
>>>> @@ -260,6 +260,23 @@ static int __init parse_extra_guest_irqs(const char *s)
>>>>  }
>>>>  custom_param("extra_guest_irqs", parse_extra_guest_irqs);
>>>>  
>>>> +/*
>>>> + * Destroy a domain once all references to it have been dropped.  Used either
>>>> + * from the RCU path, or from the domain_create() error path before the domain
>>>> + * is inserted into the domlist.
>>>> + */
>>>> +static void __domain_destroy(struct domain *d)
>>>> +{
>>>> +    BUG_ON(!d->is_dying);
>>>> +    BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
>>>> +
>>>> +    xfree(d->pbuf);
>>> With this changed to XFREE here:
>> This is the one place where it doesn't matter.  d goes fully out of
>> scope before the end of this function.
> That's fair enough.
>
>>> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
>>>
>>>> +
>>>> +    free_cpumask_var(d->dirty_cpumask);
>>> On making things idempotent: this function seems to be a candidate.
>> I don't understand.  One implementation is xfree() under the hood, and
>> the other is a no-op because no allocation took place.
> I mean it would probably be useful to make free_cpumask_var idempotent
> by using XFREE so multiple calls to it will not free dangling pointer.

Ah - that's complicated because of the (lack of) indirection of the
parameter.

There is FREE_CPUMASK_VAR() which DTRT, but see above for why it isn't
used.  (There is a similar FREE_XENHEAP_PAGE helper).

~Andrew

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