[Qemu-devel] Re: [PATCH] rtl8139: IO memory is not part of vmstate

[Alex Williamson]

On Wed, 2010-12-15 at 12:07 +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 14, 2010 at 08:41:55AM -0700, Alex Williamson wrote:
> > On Tue, 2010-12-14 at 14:32 +0200, Michael S. Tsirkin wrote:
> > > On Mon, Dec 13, 2010 at 10:00:48PM -0700, Alex Williamson wrote:
> > > > On Tue, 2010-12-14 at 06:43 +0200, Michael S. Tsirkin wrote:
> > > > > On Mon, Dec 13, 2010 at 12:15:08PM -0700, Alex Williamson wrote:
> > > > > > On Mon, 2010-12-13 at 21:06 +0200, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Dec 13, 2010 at 11:59:16AM -0700, Alex Williamson wrote:
> > > > > > > > On Mon, 2010-12-13 at 20:54 +0200, Michael S. Tsirkin wrote:
> > > > > > > > > On Mon, Dec 13, 2010 at 11:00:44AM -0700, Alex Williamson 
> > > > > > > > > wrote:
> > > > > > > > > > On Mon, 2010-12-13 at 19:50 +0200, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Mon, Dec 13, 2010 at 10:43:22AM -0700, Alex Williamson 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > So, unfortunately, I stand by my original patch.
> > > > > > > > > > > 
> > > > > > > > > > > What about the one that put -1 in saved index for a 
> > > > > > > > > > > hotplugged device?
> > > > > > > > > > 
> > > > > > > > > > There are still examples that don't work even without 
> > > > > > > > > > hotplug (example 2
> > > > > > > > > > and example 3 after the reboot).  That hack limits the 
> > > > > > > > > > damage, but still
> > > > > > > > > > leaves a latent bug for reboot and doesn't address the 
> > > > > > > > > > non-hotplug
> > > > > > > > > > scenarios.  So, I don't think it's worthwhile to pursue, 
> > > > > > > > > > and we
> > > > > > > > > > shouldn't pretend we can use it to avoid bumping the 
> > > > > > > > > > version_id.
> > > > > > > > > > Thanks,
> > > > > > > > > > 
> > > > > > > > > > Alex
> > > > > > > > > 
> > > > > > > > > I guess when we bump it we tell users: migration is completely
> > > > > > > > > borken to the old version, don't even try it.
> > > > > > > > > 
> > > > > > > > > Is there a way for libvirt to discover such incompatibilities
> > > > > > > > > and avoid the migration?
> > > > > > > > 
> > > > > > > > I don't know if libvirt has a way to query this in advance.  If 
> > > > > > > > a
> > > > > > > > migration is attempted, the target will report:
> > > > > > > > 
> > > > > > > > savevm: unsupported version 5 for '0000:00:03.0/rtl8139' v4
> > > > > > > > 
> > > > > > > > And the source will continue running.  We waste plenty of bits 
> > > > > > > > getting
> > > > > > > > to that point,
> > > > > > > 
> > > > > > > Yes, this happens after all of memory has been migrated.
> > > > > > 
> > > > > > Better late than never :^\
> > > > > 
> > > > > One other question: can we do the same by creating a new (empty)
> > > > > section? As was discussed in the past this is easier for
> > > > > downstreams to cherry-pick.
> > > > 
> > > > The only way I can think to do that would be to have a subsection that
> > > > is always included, but saves no data.  That would force a failure on
> > > > new->old migration, but I don't think it really matches the intended
> > > > purpose of subsections and feels like it's adding cruft for no gain.
> > > > Maybe I'm missing something.  Juan, is there any advantage to trapping
> > > > this in a subsection?  Thanks,
> > > > 
> > > > Alex
> > > 
> > > Maybe in this particular case the advantage is minimal.
> > > But it seems easier to stick to a rule of no more version
> > > bumps than argue about each case.
> > 
> > Do we have such a rule?
> 
> I think it's easier to have a hard rule than start arguing whether
> each change is a subsection or not for each patch.

We need to have these discussions to make sure we're doing everything we
can do preserve the ABI.  AIUI, subsections are only useful in helping
us with that if there's a worthwhile percentage of the time that the
subsection isn't needed in the migration stream.  Even then it's unclear
to me how high level tools are supposed to deal with this.  Do they
re-try the migration, hoping the subsection was only needed temporarily?
In this case, the subsection would be needed all the time, so I don't
think it's applicable and I don't see a dogmatic rule about using them
to have any value.

> What is the downside?

Some empty structures to do things in a completely convoluted way.
What's the upside?

> > If we have a subsection who's needed function
> > is return 1, I think that's a good indication that it's not appropriate
> > for a subsection and the end result is equivalent to bumping the main
> > driver vmstate version.
> 
> So if you feel strongly about it, go ahead, I'm just concerned
> we'll now need to argue version or subsection for each migration-related
> change.

Subsections don't magically preserve the migration ABI, so we still need
to have these discussions

> > It's convoluted to try to hide a one-way upgrade in a subsection.
> 
> It may be convoluted but I think it works better with downstreams.
> When we'll bump the version again in the future, it
> should be possible for a downstream to pick up
> just the next change, and skip this one.
> 
> But that's a general comment. In this case, I expect all
> downstream to be able to pick this bugfix with no real downsides.

If a subsection can maintain the migration ABI in a worthwhile
percentage of use cases and high level tools are signed up to understand
whether to retry a migration blocked by a subsection describing an
inflight DMA vs avoiding a basic incompatibility, sure, let's use them.
I don't think this problem fits or see how it makes life any easier for
downstreams.  Thanks,

Alex