Re: [PATCH v1 07/13] migrate/ram: Get rid of "place_source" in ram_load_postcopy()

[Peter Xu]

On Wed, Feb 19, 2020 at 03:47:30PM -0500, Peter Xu wrote:
> On Wed, Feb 19, 2020 at 05:17:19PM +0100, David Hildenbrand wrote:
> > It's always the same value.
> 
> I guess not, because...
> 
> > 
> > Cc: "Dr. David Alan Gilbert" <address@hidden>
> > Cc: Juan Quintela <address@hidden>
> > Cc: Peter Xu <address@hidden>
> > Signed-off-by: David Hildenbrand <address@hidden>
> > ---
> >  migration/ram.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/migration/ram.c b/migration/ram.c
> > index cbd54947fb..75014717f6 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3119,7 +3119,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >          ram_addr_t addr;
> >          void *host = NULL;
> >          void *page_buffer = NULL;
> > -        void *place_source = NULL;
> >          RAMBlock *block = NULL;
> >          uint8_t ch;
> >          int len;
> > @@ -3188,7 +3187,6 @@ static int ram_load_postcopy(QEMUFile *f)
> >                  place_needed = true;
> >                  target_pages = 0;
> >              }
> > -            place_source = postcopy_host_page;
> >          }
> >  
> >          switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > @@ -3220,7 +3218,7 @@ static int ram_load_postcopy(QEMUFile *f)
> >                   * buffer to make sure the buffer is valid when
> >                   * placing the page.
> >                   */
> > -                qemu_get_buffer_in_place(f, (uint8_t **)&place_source,
> 
> ... it can be modified inside the call.
> 
> I feel like this patch could even fail the QEMU unit test.  It would
> be good to mention what tests have been carried out in the cover
> letter or with RFC tag if no test is done yet.
> 
> For a series like this, I'll try at least the unit tests and smoke on
> both precopy and postcopy.  The resizing test would be even better but
> seems untrivial, so maybe optional.

For resizing test, an easy way (I can think of) is to temporarily
remove the size check below in your test branch:

diff --git a/exec.c b/exec.c
index 8e9cc3b47c..17dc660281 100644
--- a/exec.c
+++ b/exec.c
@@ -2128,10 +2128,6 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, 
Error **errp)
 
     newsize = HOST_PAGE_ALIGN(newsize);
 
-    if (block->used_length == newsize) {
-        return 0;
-    }
-
     if (!(block->flags & RAM_RESIZEABLE)) {
         error_setg_errno(errp, EINVAL,
                          "Length mismatch: %s: 0x" RAM_ADDR_FMT

Then reboot the guest during migration to see whether precopy can be
cancelled smoothly. And same to postcopy.  Thanks,

-- 
Peter Xu