[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/3] xen/domain: Introduce domain_teardown()
On 22.12.2020 11:25, Julien Grall wrote: > On 22/12/2020 07:50, Jan Beulich wrote: >> On 21.12.2020 19:45, Andrew Cooper wrote: >>> On 21/12/2020 18:36, Julien Grall wrote: >>>>> @@ -553,6 +606,9 @@ struct domain *domain_create(domid_t domid, >>>>> if ( init_status & INIT_watchdog ) >>>>> watchdog_domain_destroy(d); >>>>> + /* Must not hit a continuation in this context. */ >>>>> + ASSERT(domain_teardown(d) == 0); >>>> The ASSERT() will become a NOP in production build, so >>>> domain_teardown_down() will not be called. >>> >>> Urgh - its not really a nop, but it's evaluation isn't symmetric between >>> debug and release builds. I'll need an extra local variable. >> >> Or use ASSERT_UNREACHABLE(). (I admit I don't really like the >> resulting constructs, and would like to propose an alternative, >> even if I fear it'll be controversial.) >> >>>> However, I think it would be better if we pass an extra argument to >>>> indicated wheter the code is allowed to preempt. This would make the >>>> preemption check more obvious in evtchn_destroy() compare to the >>>> current d->is_dying != DOMDYING_dead. >>> >>> We can have a predicate if you'd prefer, but plumbing an extra parameter >>> is wasteful, and can only cause confusion if it is out of sync with >>> d->is_dying. >> >> I agree here - it wasn't so long ago that event_channel.c gained >> a DOMDYING_dead check, and I don't see why we shouldn't extend >> this approach to here and elsewhere. > > I think the d->is_dying != DOMYING_dead is difficult to understand even > with the comment on top. This was ok in one place, but now it will > spread everywhere. So at least, I would suggest to introduce a wrapper > that is better named. > > There is also a futureproof concern. At the moment, we are considering > the preemption will not be needed in domain_create(). I am ready to bet > that the assumption is going to be broken sooner or later. This is a fair consideration, yet I'm having trouble seeing what it might be that would cause domain_create() to require preemption. The function is supposed to only produce an empty container. But yes, if e.g. vCPU creation was to move here, the situation would indeed change. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |