Re: [PATCH 2/3] xen/domain: Introduce domain_teardown()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 22/12/2020 10:53, Jan Beulich wrote:
> 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.
We are not really creating an "empty container". There are at least one 
initial pool of memory allocated for HAP (256 pages) which need to be 
freed if we fail to create the domain.
There is a second pool in discussion for the IOMMU (see [1]) which will 
also initially allocate 256 pages.
To me it looks like domain_create() is getting quite big and preemption 
is going to be required sooner or later.
Cheers,

[1] <20201005094905.2929-6-paul@xxxxxxx>

> Jan
--
Julien Grall