It's always the same value.
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
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,
+ qemu_get_buffer_in_place(f, (uint8_t **)&postcopy_host_page,
TARGET_PAGE_SIZE);
}
break;
@@ -3265,8 +3263,8 @@ static int ram_load_postcopy(QEMUFile *f)
ret = postcopy_place_page_zero(mis, place_dest,
block);
} else {
- ret = postcopy_place_page(mis, place_dest,
- place_source, block);
+ ret = postcopy_place_page(mis, place_dest, postcopy_host_page,
+ block);
}
}
}
--
2.24.1
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" <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> 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.
Thanks,
> + qemu_get_buffer_in_place(f, (uint8_t **)&postcopy_host_page,
> TARGET_PAGE_SIZE);
> }
> break;
> @@ -3265,8 +3263,8 @@ static int ram_load_postcopy(QEMUFile *f)
> ret = postcopy_place_page_zero(mis, place_dest,
> block);
> } else {
> - ret = postcopy_place_page(mis, place_dest,
> - place_source, block);
> + ret = postcopy_place_page(mis, place_dest, postcopy_host_page,
> + block);
> }
> }
> }
> --
> 2.24.1
>
--
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" <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > 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
On 19.02.20 21:55, Peter Xu wrote:
> 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" <dgilbert@redhat.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> 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:
Yeah, it's especially hard to have a reliable test one can materialize.
I played with manual tests like this myself.
I'm thinking about testing this with a device that can trigger resizes
on demand, e.g., virtio-mem, for now on my private branch. But I'd
really like to have some test one can automate at one point ... however,
there seems to be no easy way to achieve that right now.
Thanks!
--
Thanks,
David / dhildenb
On Thu, Feb 20, 2020 at 02:22:48PM +0100, David Hildenbrand wrote: > > For resizing test, an easy way (I can think of) is to temporarily > > remove the size check below in your test branch: > > Yeah, it's especially hard to have a reliable test one can materialize. > I played with manual tests like this myself. Great! > > I'm thinking about testing this with a device that can trigger resizes > on demand, e.g., virtio-mem, for now on my private branch. But I'd > really like to have some test one can automate at one point ... however, > there seems to be no easy way to achieve that right now. Yes I totally agree. Thanks, -- Peter Xu
> Am 19.02.2020 um 21:47 schrieb Peter Xu <peterx@redhat.com>:
>
> 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" <dgilbert@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> 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 ...
Could have mentioned that in the cover letter, yes.
* David Hildenbrand (david@redhat.com) wrote:
>
>
> > Am 19.02.2020 um 21:47 schrieb Peter Xu <peterx@redhat.com>:
> >
> > 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" <dgilbert@redhat.com>
> >> Cc: Juan Quintela <quintela@redhat.com>
> >> Cc: Peter Xu <peterx@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >> 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 / dgilbert@redhat.com / Manchester, UK
© 2016 - 2026 Red Hat, Inc.