[PATCH v1 02/10] qemu_domain.c: align memory modules before calculating 'initialmem'

[Peter Krempa]

On Mon, Nov 16, 2020 at 20:43:09 +0100, Andrea Bolognani wrote:
> On Fri, 2020-11-13 at 16:23 -0300, Daniel Henrique Barboza wrote:
> > On 11/13/20 10:51 AM, Andrea Bolognani wrote:
> > > I only skimmed the remaining patches and didn't dig into the code as
> > > much, or as recently, as you have, but from a high-level perspective
> > > I don't see why you wouldn't be able to simply move the existing
> > > rounding logic from the command line generator to PostParse? It's not
> > > like the former has access to additional information that the latter
> > > can't get to, right?
> > 
> > I was looking into the code and I think that might have the wrong idea here.
> > Apparently we're not aligning memory during migration or snapshot restore.
> > This specific line in qemu_command.c got my attention:
> > 
> > -- qemuBuildCommandLine() --
> > 
> >      if (!migrateURI && !snapshot && qemuDomainAlignMemorySizes(def) < 0)
> >          return NULL;
> > 
> > ------
> > 
> > I investigated the history behind this logic and found the following commit:
> > 
> > commit c7d7ba85a6242d789ba3f4dae313e950fbb638c5
> > Author: Peter Krempa <pkrempa at redhat.com>
> > Date:   Thu Sep 17 08:14:05 2015 +0200
> > 
> >      qemu: command: Align memory sizes only on fresh starts
> >      
> >      When we are starting a qemu process for an incomming migration or
> >      snapshot reloading we should not modify the memory sizes in the domain
> >      since we could potentially change the guest ABI that was tediously
> >      checked before. Additionally the function now updates the initial memory
> >      size according to the NUMA node size, which should not happen if we are
> >      restoring state.
> >      
> >      Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1252685
> > 
> > ---------
> > 
> > This means that the changes made in this series will not break migration, since
> > the alignment of 'initialmem' is not being triggered in those cases. Which
> > is good.
> > 
> > However, you also brought up in an earlier reply that these changes might break
> > "the guest ABI across guest boots (if libvirt is upgraded in between them)". This
> > can't be helped I think - an older ppc64 guest with 4GiB of 'currentMemory' in the
> > XML, that is ending up having 4.256GiB (extra 256MiB) due to the way alignment was
> > being done, will have exactly 4GiB of 'initialmem' after these changes. My point is
> > that we're giving the exact memory the guest is demanding, as intended by the domain
> > XML, in a fresh guest start. This might be considered an ABI break probably, but
> > why would one complain that Libvirt is now giving precisely what was asked for?
> > Avoiding granting extra 256MBs of mem for domains seems worth it, given that we're
> > not impacting live domains or migration.
> 
> In general, changing guest ABI between boots is not something that we
> want to happen.
> 
> I have trouble keeping all the details of memory alignment inside my
> head and I can't quite spend the time necessary to swap them back in
> right now, so please apologies if I'm being vague and of course
> correct me if I'm wrong... Having Peter in the thread will also
> surely help with that :)

Either you have high hopes or my sarcasm detector is failing.

Just as one data point: the 'PostParse' callback happen _always_. Even
when migrating the VM. My patch according to the commit message is
specifically avoiding the alignment on migration. Thus the code can't be
moved to the post parse callback.

The outcome documented in the NEWS just mentions that it's to update the
live XML.

Neither of the commit messages of this series mentions that there is an
issue with migration so the series needs to be re-evaluated in that
light.