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

[Dr. David Alan Gilbert]

* David Hildenbrand (address@hidden) wrote:
> 
> 
> > Am 19.02.2020 um 21:47 schrieb Peter Xu <address@hidden>:
> > 
> > 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.
> 
> Very right, will drop this patch! Thanks!
> 
> > 
> > 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.
> 
> I test all code I share. This survives „make check“. I assume all tests send 
> small pages where „matches_target_page_size==true“, so the tests did not 
> catch this.
> 
> I even spent the last day getting avocado-vt to work and ran multiple 
> (obviously not all) migration tests, including postcopy, so your suggestions 
> have already been considered ...

A test on Power or aarch might catch this one; where they normally
have larger pages.

Dave

> Could have mentioned that in the cover letter, yes.
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK