Re: Xen 4.18 release: Reminder about code freeze

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Fri, Oct 13, 2023 at 2:00 PM Julien Grall <julien@xxxxxxx> wrote:
> >>> If the problem is the delay between the xenevtchn_notify() in
> >>> introduce_domain() and the domain_conn_reset() afterwards in
> >>> do_domain(), would it make sense instead to move the notification into
> >>> do_introduce(), after the domain_conn_reset()?  It is, after all, in
> >>> response to XS_INTRODUCE that we want to send the notification, not in
> >>> dom0_init() or read_state_connection() (which seems to be more about
> >>> restoring a domain).
> >>
> >> I understand that the event channel notification was specifically added
> >> for dom0less. But I don't see why we don't want to send it to dom0 as well.
> >>
> >> Technically, dom0 has exactly the same problem as dom0less domains it
> >> boots before Xenstored is running and therefore it may need to know when
> >> it is ready to receive commands.
> >
> > It seemed to work fine before 2022, when the notification was
> > introduced.
>
> Indeed. But my point is that in theory the behavior between dom0 and
> dom0less domain should not be different. With your proposal we would
> continue to diverge with seems rather strange.
>
> Do you at least agree that on the paper, Xenstored should notify it is
> ready to accept commands the same way for every domain?

I don't know enough about the start-of-day to know why dom0 did fine
without a notification for nearly 20 years, but dom0less domUs don't.
To answer your question I'd need the documentation that Andrew wants;
or at least the answers to the two questions which you deferred to
Juergen.  I assume that dom0 doesn't really need it, because it did
fine without it; I assume that dom0less domUs do really need it,
because otherwise people wouldn't have spent the effort to introduce
it.

> > Personally I'd just take the patch as I wrote it, restoring dom0's
> > pre-2022 behavior (after review and testing of course); but if you
> > wanted to test & resubmit with a similar notification inside
> > dom0_init(), I wouldn't object to it.
>
> Just to clarify, you suggest the revert because you are concerned that
> it could break dom0. Is that correct?

I'm not sure why you're calling it a revert.  The initial patch that
introduced it didn't mention wanting to include dom0 specifically; on
the contrary it said:

"The extra notification is harmless for domains that don't require it."

My reasons for leaving dom0 notification out of my patch were:

1. It's awkward to keep the dom0 notification without doing a lot of
refactoring (which should be avoided at this point in the release) or
making things kind of ugly and difficult to maintain (lots of specific
if statements)

2. Since dom0 did fine for 20 years without it, it seemed that it
wasn't necessary

3. Since nobody seems to actually know what's going on, there's a
chance it's actually harmful (although that chance is relatively
small, given the amount of time the extra notification has been in the
tree).

> > Naturally it would be ideal if we could avoid the code duplication;
> > but that's not possible without a lot more refactoring, which I don't
> > think we should be doing at this stage in the release.
>
> You can by moving the code just at the end of introduce_domain().

At the end of introduce_domain(), your if statement would have to look
like this:

if (!restore) {
   if (!is_master_domain)
    domain_conn_reset():
  interface->connection = XENSTORE_CONNECTED;
  xenevtchn_notify(...);
}

This is all not terribly maintainable; you're hoisting the
domain_conn_reset() thing into a shared codepath, where the other two
callers don't need it, just to avoid potentially duplicating two
probably unnecessary lines in dom0_init().  Given the kinds of bugs
that have already arisen because of putting things into this
multiplexed function (the current one we're trying to fix, as well as
49dd52fb131), I'd prefer to keep things simple.

I wouldn't nack such a patch.  But I would have a harder time arguing
in favor of such a patch, than a patch like the one I've written
(perhaps with a duplicated notification in dom0_init() if you really
think it's necessary).

 -George