Re: [PATCH RFC 2/4] xen: add bitmap to indicate per-domain state changes

[Juergen Gross <jgross@xxxxxxxx>]

On 22.11.21 11:41, Jan Beulich wrote:
> On 14.09.2021 14:35, Juergen Gross wrote:
> > Add a bitmap with one bit per possible domid indicating the respective
> > domain has changed its state (created, deleted, dying, crashed,
> > shutdown).
> >
> > Registering the VIRQ_DOM_EXC event will result in setting the bits for
> > all existing domains and resetting all other bits.
> Generally I view VIRQ_DOM_EXC as overly restrictive already - what if both
> a xenstore domain and a control domain want respective notification? Hence
The general idea was that in this case the control domain should
register a related Xenstore watch.

> similarly I'm not convinced a single, global instance will do here. Which
> in turn raises the question whether the approach chosen may not take us
> far enough, and we wouldn't instead want a more precise notification model
> (i.e. not just "something has changed").
Yes, that would be the job of Xenstore. It would provide the more
fine grained watches for that purpose.

> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -87,6 +87,22 @@ bool __read_mostly vmtrace_available;
> >   /* Unique domain identifier, protected by domctl lock. */
> >   static uint64_t unique_id;
> >   
> > +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);
> While not really making a difference to the size of the bitmap, afaict up
> to DOMID_FIRST_RESERVED would do. Neither system domains nor idle ones
> will reach, in particular, the set_bit() in domain_create(). And none of
> them can be subject to destruction.
I'd be fine either way.

> Also I think this could do with a brief comment as to what causes bits
> to be set. This would avoid readers having to go hunt down all the
> set_bit() (or the commit introducing the bitmap).
Okay.

> > +void domain_reset_states(void)
> > +{
> > +    struct domain *d;
> > +
> > +    bitmap_zero(dom_state_changed, DOMID_MASK + 1);
> While this looks to be fine with the present updates of the bitmap, I
> still wonder about the non-atomicity here vs the atomic updates
> everywhere else. It feels like there's some locking needed to be future
> proof. Since you come here from VIRQ_DOM_EXC binding, it could be that
> domain's per-domain lock.
Fine with me.

> > @@ -1141,6 +1161,8 @@ static void complete_domain_destroy(struct rcu_head *head)
> >   
> >       xfree(d->vcpu);
> >   
> > +    set_bit(d->domain_id, dom_state_changed);
> > +
> >       _domain_destroy(d);
> >   
> >       send_global_virq(VIRQ_DOM_EXC);
> Wouldn't this better be in domain_destroy() immediately after the domain
> has been taken off the list, and hence is no longer "discoverable"?
In this case the call of send_global_virq() should be moved, too,
I guess?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature