Re: Xen 4.18 release: Reminder about code freeze

[Julien Grall <julien@xxxxxxx>]

On 13/10/2023 14:30, George Dunlap wrote:
> 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.  
Because we are going to remove a behavior that is present in at least a 
release. So...
> 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."
... even if this is what the commit message says, we can't tell whether 
someone would start to rely on it. I definitely see use-cases for it.
But I agree that the chance they are actualy used is slim as we didn't 
document 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).
Your reasoning makes sense. I don't fully agree with it, but at the end 
of the day we need to have a solution for dom0less guests... So if the 
others agree with you, then I will disagree and commit.
That said, I think the commit message should explain why removing dom0 
notification is ok.
Cheers,

--
Julien Grall