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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Sep 03, 2018 at 06:05:34PM +0100, Andrew Cooper wrote:
> 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@xxxxxxxxxx>
> >>>> ---
> >>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >>>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> >>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >>>> CC: Julien Grall <julien.grall@xxxxxxx>
> >>>> ---
> >>>>  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@xxxxxxxxxx>
> >>>
> >>>> +
> >>>> +    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).

Okay. I don't have further comments on this.

Wei.

> 
> ~Andrew

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