Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
On Thu, Feb 13, 2020 at 10:17:04AM +, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> ram_discard_range() unmap page for specific range. To be specific, this
>> clears related page table entries so that userfault would be triggered.
>> But this step is not necessary at the very beginning.
>>
>> ram_postcopy_incoming_init() is called when destination gets ADVISE
>> command. ADVISE command is sent when migration thread just starts, which
>> implies destination is not running yet. This means no page fault
>> happened and memory region's page tables entries are empty.
>>
>> This patch removes the discard at the beginning.
>>
>> Signed-off-by: Wei Yang
>> ---
>> migration/postcopy-ram.c | 46
>> migration/postcopy-ram.h | 7 --
>> migration/ram.c | 16 --
>> migration/ram.h | 1 -
>> migration/savevm.c | 4
>> 5 files changed, 74 deletions(-)
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 5da6de8c8b..459be8e780 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -443,32 +443,6 @@ out:
>> return ret;
>> }
>>
>> -/*
>> - * Setup an area of RAM so that it *can* be used for postcopy later; this
>> - * must be done right at the start prior to pre-copy.
>> - * opaque should be the MIS.
>> - */
>> -static int init_range(RAMBlock *rb, void *opaque)
>> -{
>> -const char *block_name = qemu_ram_get_idstr(rb);
>> -void *host_addr = qemu_ram_get_host_addr(rb);
>> -ram_addr_t offset = qemu_ram_get_offset(rb);
>> -ram_addr_t length = qemu_ram_get_used_length(rb);
>> -trace_postcopy_init_range(block_name, host_addr, offset, length);
>> -
>> -/*
>> - * We need the whole of RAM to be truly empty for postcopy, so things
>> - * like ROMs and any data tables built during init must be zero'd
>> - * - we're going to get the copy from the source anyway.
>> - * (Precopy will just overwrite this data, so doesn't need the discard)
>> - */
>
>But this comment explains why we want to do the discard; we want to make
>sure that any memory that's been populated by the destination during the
>init process is discarded and replaced by content from the source.
>
OK, you are right. I missed the init stage.
>Dave
>
--
Wei Yang
Help you, Help me
Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
Hi, David and Juan
Does it look good to you?
On Mon, Oct 07, 2019 at 05:10:08PM +0800, Wei Yang wrote:
>ram_discard_range() unmap page for specific range. To be specific, this
>clears related page table entries so that userfault would be triggered.
>But this step is not necessary at the very beginning.
>
>ram_postcopy_incoming_init() is called when destination gets ADVISE
>command. ADVISE command is sent when migration thread just starts, which
>implies destination is not running yet. This means no page fault
>happened and memory region's page tables entries are empty.
>
>This patch removes the discard at the beginning.
>
>Signed-off-by: Wei Yang
>---
> migration/postcopy-ram.c | 46
> migration/postcopy-ram.h | 7 --
> migration/ram.c | 16 --
> migration/ram.h | 1 -
> migration/savevm.c | 4
> 5 files changed, 74 deletions(-)
>
>diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>index 5da6de8c8b..459be8e780 100644
>--- a/migration/postcopy-ram.c
>+++ b/migration/postcopy-ram.c
>@@ -443,32 +443,6 @@ out:
> return ret;
> }
>
>-/*
>- * Setup an area of RAM so that it *can* be used for postcopy later; this
>- * must be done right at the start prior to pre-copy.
>- * opaque should be the MIS.
>- */
>-static int init_range(RAMBlock *rb, void *opaque)
>-{
>-const char *block_name = qemu_ram_get_idstr(rb);
>-void *host_addr = qemu_ram_get_host_addr(rb);
>-ram_addr_t offset = qemu_ram_get_offset(rb);
>-ram_addr_t length = qemu_ram_get_used_length(rb);
>-trace_postcopy_init_range(block_name, host_addr, offset, length);
>-
>-/*
>- * We need the whole of RAM to be truly empty for postcopy, so things
>- * like ROMs and any data tables built during init must be zero'd
>- * - we're going to get the copy from the source anyway.
>- * (Precopy will just overwrite this data, so doesn't need the discard)
>- */
>-if (ram_discard_range(block_name, 0, length)) {
>-return -1;
>-}
>-
>-return 0;
>-}
>-
> /*
> * At the end of migration, undo the effects of init_range
> * opaque should be the MIS.
>@@ -506,20 +480,6 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
> return 0;
> }
>
>-/*
>- * Initialise postcopy-ram, setting the RAM to a state where we can go into
>- * postcopy later; must be called prior to any precopy.
>- * called from arch_init's similarly named ram_postcopy_incoming_init
>- */
>-int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>-{
>-if (foreach_not_ignored_block(init_range, NULL)) {
>-return -1;
>-}
>-
>-return 0;
>-}
>-
> /*
> * Manage a single vote to the QEMU balloon inhibitor for all postcopy usage,
> * last caller wins.
>@@ -1282,12 +1242,6 @@ bool
>postcopy_ram_supported_by_host(MigrationIncomingState *mis)
> return false;
> }
>
>-int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>-{
>-error_report("postcopy_ram_incoming_init: No OS support");
>-return -1;
>-}
>-
> int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> {
> assert(0);
>diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
>index c0ccf64a96..1c79c6e51f 100644
>--- a/migration/postcopy-ram.h
>+++ b/migration/postcopy-ram.h
>@@ -22,13 +22,6 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState
>*mis);
> */
> int postcopy_ram_incoming_setup(MigrationIncomingState *mis);
>
>-/*
>- * Initialise postcopy-ram, setting the RAM to a state where we can go into
>- * postcopy later; must be called prior to any precopy.
>- * called from ram.c's similarly named ram_postcopy_incoming_init
>- */
>-int postcopy_ram_incoming_init(MigrationIncomingState *mis);
>-
> /*
> * At the end of a migration where postcopy_ram_incoming_init was called.
> */
>diff --git a/migration/ram.c b/migration/ram.c
>index dfc50d57d5..9a853703d8 100644
>--- a/migration/ram.c
>+++ b/migration/ram.c
>@@ -4015,22 +4015,6 @@ static int ram_load_cleanup(void *opaque)
> return 0;
> }
>
>-/**
>- * ram_postcopy_incoming_init: allocate postcopy data structures
>- *
>- * Returns 0 for success and negative if there was one error
>- *
>- * @mis: current migration incoming state
>- *
>- * Allocate data structures etc needed by incoming migration with
>- * postcopy-ram. postcopy-ram's similarly names
>- * postcopy_ram_incoming_init does the work.
>- */
>-int ram_postcopy_incoming_init(MigrationIncomingState *mis)
>-{
>-return postcopy_ram_incoming_init(mis);
>-}
>-
> /**
> * ram_load_postco
Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
Oops, this one seems to be missed.
On Mon, Oct 07, 2019 at 05:10:08PM +0800, Wei Yang wrote:
>ram_discard_range() unmap page for specific range. To be specific, this
>clears related page table entries so that userfault would be triggered.
>But this step is not necessary at the very beginning.
>
>ram_postcopy_incoming_init() is called when destination gets ADVISE
>command. ADVISE command is sent when migration thread just starts, which
>implies destination is not running yet. This means no page fault
>happened and memory region's page tables entries are empty.
>
>This patch removes the discard at the beginning.
>
>Signed-off-by: Wei Yang
>---
> migration/postcopy-ram.c | 46
> migration/postcopy-ram.h | 7 --
> migration/ram.c | 16 --
> migration/ram.h | 1 -
> migration/savevm.c | 4
> 5 files changed, 74 deletions(-)
>
>diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>index 5da6de8c8b..459be8e780 100644
>--- a/migration/postcopy-ram.c
>+++ b/migration/postcopy-ram.c
>@@ -443,32 +443,6 @@ out:
> return ret;
> }
>
>-/*
>- * Setup an area of RAM so that it *can* be used for postcopy later; this
>- * must be done right at the start prior to pre-copy.
>- * opaque should be the MIS.
>- */
>-static int init_range(RAMBlock *rb, void *opaque)
>-{
>-const char *block_name = qemu_ram_get_idstr(rb);
>-void *host_addr = qemu_ram_get_host_addr(rb);
>-ram_addr_t offset = qemu_ram_get_offset(rb);
>-ram_addr_t length = qemu_ram_get_used_length(rb);
>-trace_postcopy_init_range(block_name, host_addr, offset, length);
>-
>-/*
>- * We need the whole of RAM to be truly empty for postcopy, so things
>- * like ROMs and any data tables built during init must be zero'd
>- * - we're going to get the copy from the source anyway.
>- * (Precopy will just overwrite this data, so doesn't need the discard)
>- */
>-if (ram_discard_range(block_name, 0, length)) {
>-return -1;
>-}
>-
>-return 0;
>-}
>-
> /*
> * At the end of migration, undo the effects of init_range
> * opaque should be the MIS.
>@@ -506,20 +480,6 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
> return 0;
> }
>
>-/*
>- * Initialise postcopy-ram, setting the RAM to a state where we can go into
>- * postcopy later; must be called prior to any precopy.
>- * called from arch_init's similarly named ram_postcopy_incoming_init
>- */
>-int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>-{
>-if (foreach_not_ignored_block(init_range, NULL)) {
>-return -1;
>-}
>-
>-return 0;
>-}
>-
> /*
> * Manage a single vote to the QEMU balloon inhibitor for all postcopy usage,
> * last caller wins.
>@@ -1282,12 +1242,6 @@ bool
>postcopy_ram_supported_by_host(MigrationIncomingState *mis)
> return false;
> }
>
>-int postcopy_ram_incoming_init(MigrationIncomingState *mis)
>-{
>-error_report("postcopy_ram_incoming_init: No OS support");
>-return -1;
>-}
>-
> int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> {
> assert(0);
>diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
>index c0ccf64a96..1c79c6e51f 100644
>--- a/migration/postcopy-ram.h
>+++ b/migration/postcopy-ram.h
>@@ -22,13 +22,6 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState
>*mis);
> */
> int postcopy_ram_incoming_setup(MigrationIncomingState *mis);
>
>-/*
>- * Initialise postcopy-ram, setting the RAM to a state where we can go into
>- * postcopy later; must be called prior to any precopy.
>- * called from ram.c's similarly named ram_postcopy_incoming_init
>- */
>-int postcopy_ram_incoming_init(MigrationIncomingState *mis);
>-
> /*
> * At the end of a migration where postcopy_ram_incoming_init was called.
> */
>diff --git a/migration/ram.c b/migration/ram.c
>index dfc50d57d5..9a853703d8 100644
>--- a/migration/ram.c
>+++ b/migration/ram.c
>@@ -4015,22 +4015,6 @@ static int ram_load_cleanup(void *opaque)
> return 0;
> }
>
>-/**
>- * ram_postcopy_incoming_init: allocate postcopy data structures
>- *
>- * Returns 0 for success and negative if there was one error
>- *
>- * @mis: current migration incoming state
>- *
>- * Allocate data structures etc needed by incoming migration with
>- * postcopy-ram. postcopy-ram's similarly names
>- * postcopy_ram_incoming_init does the work.
>- */
>-int ram_postcopy_incoming_init(MigrationIncomingState *mis)
>-{
>-return postcopy_ram_incoming_init(mis);
>-}
>-
> /**
> * ram_load_postcopy: load a page
Re: [PATCH 0/2] not use multifd during postcopy
On Thu, Jan 09, 2020 at 10:50:25AM +0100, Juan Quintela wrote: >Wei Yang wrote: >> On Mon, Dec 16, 2019 at 10:35:39AM +0800, Wei Yang wrote: >>>Would this one be picked up this time? >> >> Happy new year to all. >> >> Can I ask the plan for this patch set? > >queued > Thanks :-) -- Wei Yang Help you, Help me
Re: [Patch v2 0/6] migration/postcopy: enable compress during postcopy
On Wed, Dec 18, 2019 at 07:55:38PM +, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> Would this one be picked up in this version? > >I think that one is on Juan's list for the pull he's going to do soon. > >Dave > Happy New Year to all~ May I ask the plan for this patch set? -- Wei Yang Help you, Help me
Re: [PATCH 0/2] not use multifd during postcopy
On Mon, Dec 16, 2019 at 10:35:39AM +0800, Wei Yang wrote: >Would this one be picked up this time? Happy new year to all. Can I ask the plan for this patch set? > >On Sat, Oct 26, 2019 at 07:19:58AM +0800, Wei Yang wrote: >>We don't support multifd during postcopy, but user still could enable >>both multifd and postcopy. This leads to migration failure. >> >>Patch 1 does proper cleanup, otherwise we may have data corruption. >>Patch 2 does the main job. >> >>BTW, current multifd synchronization method needs a cleanup. Will send another >>patch set. >> >>Wei Yang (2): >> migration/multifd: clean pages after filling packet >> migration/multifd: not use multifd during postcopy >> >> migration/ram.c | 15 ++++++- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >>-- >>2.17.1 > >-- >Wei Yang >Help you, Help me -- Wei Yang Help you, Help me
Re: [PATCH v2 0/6] migration/multifd: a new mechanism for send thread sync
Ping for comment. On Sat, Oct 26, 2019 at 08:45:14AM +0800, Wei Yang wrote: >Current send thread could work while the sync mechanism has some problem: > > * has spuriously wakeup > * number of channels_ready will *overflow* the number of real channels > >The reason is: > > * if MULTIFD_FLAG_SYNC is set in the middle of send thread running, there >is one more spurious wakeup > * if MULTIFD_FLAG_SYNC is set when send thread is not running, there is one >more channels_ready be triggered > >To solve this situation, one new mechanism is introduced to synchronize send >threads. The idea is simple, a new field *sync* is introduced to indicate a >synchronization is required. > >--- >v2: rebase on latest code > >Wei Yang (6): > migration/multifd: move Params update and pages cleanup into >multifd_send_fill_packet() > migration/multifd: notify channels_ready when send thread starts > migration/multifd: use sync field to synchronize send threads > migration/multifd: used must not be 0 for a pending job > migration/multifd: use boolean for pending_job is enough > migration/multifd: there is no spurious wakeup now > > migration/ram.c | 74 +++------ > 1 file changed, 47 insertions(+), 27 deletions(-) > >-- >2.17.1 -- Wei Yang Help you, Help me
Re: [PATCH 0/2] not use multifd during postcopy
Would this one be picked up this time? On Sat, Oct 26, 2019 at 07:19:58AM +0800, Wei Yang wrote: >We don't support multifd during postcopy, but user still could enable >both multifd and postcopy. This leads to migration failure. > >Patch 1 does proper cleanup, otherwise we may have data corruption. >Patch 2 does the main job. > >BTW, current multifd synchronization method needs a cleanup. Will send another >patch set. > >Wei Yang (2): > migration/multifd: clean pages after filling packet > migration/multifd: not use multifd during postcopy > > migration/ram.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > >-- >2.17.1 -- Wei Yang Help you, Help me
Re: [Patch v2 0/6] migration/postcopy: enable compress during postcopy
Would this one be picked up in this version? On Thu, Nov 07, 2019 at 08:39:01PM +0800, Wei Yang wrote: >This patch set tries enable compress during postcopy. > >postcopy requires to place a whole host page, while migration thread migrate >memory in target page size. This makes postcopy need to collect all target >pages in one host page before placing via userfaultfd. > >To enable compress during postcopy, there are two problems to solve: > >1. Random order for target page arrival >2. Target pages in one host page arrives without interrupt by target > page from other host page > >The first one is handled by counting the number of target pages arrived >instead of the last target page arrived. > >The second one is handled by: > >1. Flush compress thread for each host page >2. Wait for decompress thread for before placing host page > >With the combination of these two changes, compress is enabled during >postcopy. > >--- >v2: > * use uintptr_t to calculate place_dest > * check target pages belongs to the same host page > >Wei Yang (6): > migration/postcopy: reduce memset when it is zero page and >matches_target_page_size > migration/postcopy: wait for decompress thread in precopy > migration/postcopy: count target page number to decide the >place_needed > migration/postcopy: set all_zero to true on the first target page > migration/postcopy: enable random order target page arrival > migration/postcopy: enable compress during postcopy > > migration/migration.c | 11 --- > migration/ram.c | 67 +-- > 2 files changed, 52 insertions(+), 26 deletions(-) > >-- >2.17.1 -- Wei Yang Help you, Help me
Re: [PATCH v2 1/6] migration/multifd: move Params update and pages cleanup into multifd_send_fill_packet()
On Tue, Nov 19, 2019 at 11:57:22AM +0100, Juan Quintela wrote: >Wei Yang wrote: >> Fill data and update/cleanup related field in one place. Also make the >> code a little clean. >> >> Signed-off-by: Wei Yang > >Reviewed-by: Juan Quintela > >right cleanup. > Hi, Juan Do you have other comments on the following patches? -- Wei Yang Help you, Help me
Re: [PATCH 2/2] migration/multifd: not use multifd during postcopy
On Tue, Nov 19, 2019 at 11:55:52AM +0100, Juan Quintela wrote: >Wei Yang wrote: >> We don't support multifd during postcopy, but user still could enable >> both multifd and postcopy. This leads to migration failure. >> >> Skip multifd during postcopy. >> >> Signed-off-by: Wei Yang > >Reviewed-by: Juan Quintela > >I was working in a different implementation, but I agree with the idea. > >My patch series try to decide during negotiation if multifd + everything >else is setup or not. > Look forward your approach :-) >Thanks, Juan. -- Wei Yang Help you, Help me
Re: [PATCH 0/2] not use multifd during postcopy
Ping for comments. On Sat, Oct 26, 2019 at 07:19:58AM +0800, Wei Yang wrote: >We don't support multifd during postcopy, but user still could enable >both multifd and postcopy. This leads to migration failure. > >Patch 1 does proper cleanup, otherwise we may have data corruption. >Patch 2 does the main job. > >BTW, current multifd synchronization method needs a cleanup. Will send another >patch set. > >Wei Yang (2): > migration/multifd: clean pages after filling packet > migration/multifd: not use multifd during postcopy > > migration/ram.c | 15 ++- > 1 file changed, 10 insertions(+), 5 deletions(-) > >-- >2.17.1 -- Wei Yang Help you, Help me
Re: [PATCH 1/2] tests/tcg/multiarch: fix code style in function main of test-mmap.c
On Mon, Nov 11, 2019 at 10:25:43AM +, Alex Benn??e wrote:
>
>Wei Yang writes:
>
>> This file uses quite a different code style and changing just one line
>> would leads to some awkward appearance.
>>
>> This is a preparation for the following replacement of
>> sysconf(_SC_PAGESIZE).
>>
>> BTW, to depress ERROR message from checkpatch.pl, this patch replaces
>> strtoul with qemu_strtoul.
>
>
>NACK I'm afraid.
>
>The tests/tcg directory all build against glibc only to make them easier
>to cross-compile for the various targets. If you run check-tcg and have
>a non-native cross compiler setup you'll notice this fails to build:
>
>BUILD aarch64-linux-user guest-tests with aarch64-linux-gnu-gcc
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c: In function
> ???main???:
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c:467:9: warning:
> implicit declaration of function ???qemu_strtoul???; did you mean
> ???strtoul [-Wimplicit-function-declaration]
> qemu_strtoul(argv[1], NULL, 0, );
> ^~~~
> strtoul
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c:469:20: error:
> ???qemu_real_host_page_size??? undeclared (first use in this function)
> pagesize = qemu_real_host_page_size;
> ^~~~
> /home/alex/lsrc/qemu.git/tests/tcg/multiarch/test-mmap.c:469:20: note: each
> undeclared identifier is reported only once for each function it appears in
> make[2]: *** [../Makefile.target:103: test-mmap] Error 1
> make[1]: *** [/home/alex/lsrc/qemu.git/tests/tcg/Makefile.qemu:33:
> cross-build-guest-tests] Error 2
> make: *** [/home/alex/lsrc/qemu.git/tests/Makefile.include:1094:
> build-tcg-tests-aarch64-linux-user] Error 2
> make: *** Waiting for unfinished jobs
>
This output is from "make test" ?
>>
>> Signed-off-by: Wei Yang
>> ---
>> tests/tcg/multiarch/test-mmap.c | 67 ++---
>> 1 file changed, 36 insertions(+), 31 deletions(-)
>>
>> diff --git a/tests/tcg/multiarch/test-mmap.c
>> b/tests/tcg/multiarch/test-mmap.c
>> index 11d0e777b1..9ea49e2307 100644
>> --- a/tests/tcg/multiarch/test-mmap.c
>> +++ b/tests/tcg/multiarch/test-mmap.c
>> @@ -456,49 +456,54 @@ void check_invalid_mmaps(void)
>>
>> int main(int argc, char **argv)
>> {
>> -char tempname[] = "/tmp/.cmmapXX";
>> -unsigned int i;
>> -
>> -/* Trust the first argument, otherwise probe the system for our
>> - pagesize. */
>> -if (argc > 1)
>> -pagesize = strtoul(argv[1], NULL, 0);
>> -else
>> -pagesize = sysconf(_SC_PAGESIZE);
>> +char tempname[] = "/tmp/.cmmapXX";
>> +unsigned int i;
>> +
>> +/*
>> + * Trust the first argument, otherwise probe the system for our
>> + * pagesize.
>> + */
>> +if (argc > 1) {
>> +qemu_strtoul(argv[1], NULL, 0, );
>> +} else {
>> +pagesize = sysconf(_SC_PAGESIZE);
>> +}
>>
>> -/* Assume pagesize is a power of two. */
>> -pagemask = pagesize - 1;
>> -dummybuf = malloc (pagesize);
>> -printf ("pagesize=%u pagemask=%x\n", pagesize, pagemask);
>> +/* Assume pagesize is a power of two. */
>> +pagemask = pagesize - 1;
>> +dummybuf = malloc(pagesize);
>> +printf("pagesize=%u pagemask=%x\n", pagesize, pagemask);
>>
>> -test_fd = mkstemp(tempname);
>> -unlink(tempname);
>> +test_fd = mkstemp(tempname);
>> +unlink(tempname);
>>
>> -/* Fill the file with int's counting from zero and up. */
>> +/* Fill the file with int's counting from zero and up. */
>> for (i = 0; i < (pagesize * 4) / sizeof i; i++) {
>> checked_write(test_fd, , sizeof i);
>> }
>>
>> -/* Append a few extra writes to make the file end at non
>> - page boundary. */
>> +/*
>> + * Append a few extra writes to make the file end at non
>> + * page boundary.
>> + */
>> checked_write(test_fd, , sizeof i); i++;
>> checked_write(test_fd, , sizeof i); i++;
>> checked_write(test_fd, , sizeof i); i++;
>>
>> -test_fsize = lseek(test_fd, 0, SEEK_CUR);
>> +test_fsize = lseek(test_fd, 0, SEEK_CUR);
>>
>> -/* Run the tests. */
>> -check_aligned_anonymous_unfixed_mmaps();
>> -check_aligned_anonymous_unfixe
Re: [PATCH 2/2] core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
On Mon, Nov 11, 2019 at 11:06:41AM +0100, Stefano Garzarella wrote:
>Why "core:" in the commit title?
>
>Perhaps to indicate that the patch concerns different subsystems,
>I'd use "qemu: ", but I'm not sure :-)
>
I didn't find a better one. Maybe "qemu" is better :-)
>On Tue, Oct 15, 2019 at 11:13:50AM +0800, Wei Yang wrote:
>> Signed-off-by: Wei Yang
>> Suggested-by: "Dr. David Alan Gilbert"
>> CC: Richard Henderson
>> ---
>> block/file-posix.c | 2 +-
>> net/l2tpv3.c| 2 +-
>> tests/tcg/multiarch/test-mmap.c | 2 +-
>> 3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 5d1995a07c..853ed42134 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2562,7 +2562,7 @@ static void check_cache_dropped(BlockDriverState *bs,
>> Error **errp)
>> off_t end;
>>
>> /* mincore(2) page status information requires 1 byte per page */
>> -page_size = sysconf(_SC_PAGESIZE);
>> +page_size = qemu_real_host_page_size;
>> vec = g_malloc(DIV_ROUND_UP(window_size, page_size));
>>
>> end = raw_getlength(bs);
>> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
>> index 55fea17c0f..5f843240de 100644
>> --- a/net/l2tpv3.c
>> +++ b/net/l2tpv3.c
>> @@ -41,7 +41,7 @@
>> * chosen to be sufficient to accommodate one packet with some headers
>> */
>>
>> -#define BUFFER_ALIGN sysconf(_SC_PAGESIZE)
>> +#define BUFFER_ALIGN qemu_real_host_page_size
>> #define BUFFER_SIZE 2048
>> #define IOVSIZE 2
>> #define MAX_L2TPV3_MSGCNT 64
>> diff --git a/tests/tcg/multiarch/test-mmap.c
>> b/tests/tcg/multiarch/test-mmap.c
>> index 9ea49e2307..370842e5c2 100644
>> --- a/tests/tcg/multiarch/test-mmap.c
>> +++ b/tests/tcg/multiarch/test-mmap.c
>> @@ -466,7 +466,7 @@ int main(int argc, char **argv)
>> if (argc > 1) {
>> qemu_strtoul(argv[1], NULL, 0, );
>> } else {
>> -pagesize = sysconf(_SC_PAGESIZE);
>> +pagesize = qemu_real_host_page_size;
>> }
>>
>> /* Assume pagesize is a power of two. */
>
>The patch LGTM:
>Reviewed-by: Stefano Garzarella
>
>Thanks,
>Stefano
>
--
Wei Yang
Help you, Help me
Re: [PATCH 0/2] replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
On Tue, Oct 15, 2019 at 11:13:48AM +0800, Wei Yang wrote: >This is a following up patch to cleanup page size, suggested by >"Dr. David Alan Gilbert" . > >Patch 2 does the job, while during the cleanup I found test-mmap.c has quite a >lot code style problem. To make the code looks good, patch 1 is introduced to >make checkpatch.pl happy a little. Does this patch set look good? > >Wei Yang (2): > tests/tcg/multiarch: fix code style in function main of test-mmap.c > core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size > > block/file-posix.c | 2 +- > net/l2tpv3.c| 2 +- > tests/tcg/multiarch/test-mmap.c | 67 ++--- > 3 files changed, 38 insertions(+), 33 deletions(-) > >-- >2.17.1 > -- Wei Yang Help you, Help me
Re: [Patch v2 5/6] migration/postcopy: enable random order target page arrival
On Thu, Nov 07, 2019 at 02:30:25PM +, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> After using number of target page received to track one host page, we >> could have the capability to handle random order target page arrival in >> one host page. >> >> This is a preparation for enabling compress during postcopy. >> >> Signed-off-by: Wei Yang > >Yep, that's better > >Reviewed-by: Dr. David Alan Gilbert > Thanks for your suggestion :-) -- Wei Yang Help you, Help me
[Patch v2 6/6] migration/postcopy: enable compress during postcopy
postcopy requires to place a whole host page, while migration thread
migrate memory in target page size. This makes postcopy need to collect
all target pages in one host page before placing via userfaultfd.
To enable compress during postcopy, there are two problems to solve:
1. Random order for target page arrival
2. Target pages in one host page arrives without interrupt by target
page from other host page
The first one is handled by previous cleanup patch.
This patch handles the second one by:
1. Flush compress thread for each host page
2. Wait for decompress thread for before placing host page
Signed-off-by: Wei Yang
Reviewed-by: Dr. David Alan Gilbert
---
migration/migration.c | 11 ---
migration/ram.c | 28 +++-
2 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 354ad072fa..3c926a3ae3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1005,17 +1005,6 @@ static bool migrate_caps_check(bool *cap_list,
#endif
if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
-if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
-/* The decompression threads asynchronously write into RAM
- * rather than use the atomic copies needed to avoid
- * userfaulting. It should be possible to fix the decompression
- * threads for compatibility in future.
- */
-error_setg(errp, "Postcopy is not currently compatible "
- "with compression");
-return false;
-}
-
/* This check is reasonably expensive, so only when it's being
* set the first time, also it's only the destination that needs
* special support.
diff --git a/migration/ram.c b/migration/ram.c
index 666ad69284..0d53786311 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3449,6 +3449,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
rs->target_page_count += pages;
+/*
+ * During postcopy, it is necessary to make sure one whole host
+ * page is sent in one chunk.
+ */
+if (migrate_postcopy_ram()) {
+flush_compressed_data(rs);
+}
+
/*
* we want to check in the 1st loop, just in case it was the 1st
* time and we had to sync the dirty bitmap.
@@ -4026,6 +4034,7 @@ static int ram_load_postcopy(QEMUFile *f)
void *place_source = NULL;
RAMBlock *block = NULL;
uint8_t ch;
+int len;
addr = qemu_get_be64(f);
@@ -4043,7 +4052,8 @@ static int ram_load_postcopy(QEMUFile *f)
trace_ram_load_postcopy_loop((uint64_t)addr, flags);
place_needed = false;
-if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE)) {
+if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
+ RAM_SAVE_FLAG_COMPRESS_PAGE)) {
block = ram_block_from_stream(f, flags);
host = host_from_ram_block_offset(block, addr);
@@ -4126,6 +4136,17 @@ static int ram_load_postcopy(QEMUFile *f)
TARGET_PAGE_SIZE);
}
break;
+case RAM_SAVE_FLAG_COMPRESS_PAGE:
+all_zero = false;
+len = qemu_get_be32(f);
+if (len < 0 || len > compressBound(TARGET_PAGE_SIZE)) {
+error_report("Invalid compressed data length: %d", len);
+ret = -EINVAL;
+break;
+}
+decompress_data_with_multi_threads(f, page_buffer, len);
+break;
+
case RAM_SAVE_FLAG_EOS:
/* normal exit */
multifd_recv_sync_main();
@@ -4137,6 +4158,11 @@ static int ram_load_postcopy(QEMUFile *f)
break;
}
+/* Got the whole host page, wait for decompress before placing. */
+if (place_needed) {
+ret |= wait_for_decompress_done();
+}
+
/* Detect for any possible file errors */
if (!ret && qemu_file_get_error(f)) {
ret = qemu_file_get_error(f);
--
2.17.1
[Patch v2 5/6] migration/postcopy: enable random order target page arrival
After using number of target page received to track one host page, we
could have the capability to handle random order target page arrival in
one host page.
This is a preparation for enabling compress during postcopy.
Signed-off-by: Wei Yang
---
v2:
* use uintptr_t to calculate place_dest
* check target pages belongs to the same host page
---
migration/ram.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index b5759793a9..666ad69284 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4015,7 +4015,7 @@ static int ram_load_postcopy(QEMUFile *f)
MigrationIncomingState *mis = migration_incoming_get_current();
/* Temporary page that is later 'placed' */
void *postcopy_host_page = mis->postcopy_tmp_page;
-void *last_host = NULL;
+void *this_host = NULL;
bool all_zero = false;
int target_pages = 0;
@@ -4062,24 +4062,26 @@ static int ram_load_postcopy(QEMUFile *f)
* that's moved into place later.
* The migration protocol uses, possibly smaller, target-pages
* however the source ensures it always sends all the components
- * of a host page in order.
+ * of a host page in one chunk.
*/
page_buffer = postcopy_host_page +
((uintptr_t)host & (block->page_size - 1));
/* If all TP are zero then we can optimise the place */
if (target_pages == 1) {
all_zero = true;
+this_host = (void *)QEMU_ALIGN_DOWN((uintptr_t)host,
+block->page_size);
} else {
/* not the 1st TP within the HP */
-if (host != (last_host + TARGET_PAGE_SIZE)) {
-error_report("Non-sequential target page %p/%p",
- host, last_host);
+if (QEMU_ALIGN_DOWN((uintptr_t)host, block->page_size) !=
+(uintptr_t)this_host) {
+error_report("Non-same host page %p/%p",
+ host, this_host);
ret = -EINVAL;
break;
}
}
-
/*
* If it's the last part of a host page then we place the host
* page
@@ -4090,7 +4092,6 @@ static int ram_load_postcopy(QEMUFile *f)
}
place_source = postcopy_host_page;
}
-last_host = host;
switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
case RAM_SAVE_FLAG_ZERO:
@@ -4143,7 +4144,8 @@ static int ram_load_postcopy(QEMUFile *f)
if (!ret && place_needed) {
/* This gets called at the last target page in the host page */
-void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
+void *place_dest = (void *)QEMU_ALIGN_DOWN((uintptr_t)host,
+ block->page_size);
if (all_zero) {
ret = postcopy_place_page_zero(mis, place_dest,
--
2.17.1
[Patch v2 2/6] migration/postcopy: wait for decompress thread in precopy
Compress is not supported with postcopy, it is safe to wait for
decompress thread just in precopy.
This is a preparation for later patch.
Signed-off-by: Wei Yang
Reviewed-by: Dr. David Alan Gilbert
---
migration/ram.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 7938a643d9..f59e3fe197 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4375,6 +4375,7 @@ static int ram_load_precopy(QEMUFile *f)
}
}
+ret |= wait_for_decompress_done();
return ret;
}
@@ -4406,8 +4407,6 @@ static int ram_load(QEMUFile *f, void *opaque, int
version_id)
} else {
ret = ram_load_precopy(f);
}
-
-ret |= wait_for_decompress_done();
}
trace_ram_load_complete(ret, seq_iter);
--
2.17.1
[Patch v2 4/6] migration/postcopy: set all_zero to true on the first target page
For the first target page, all_zero is set to true for this round check.
After target_pages introduced, we could leverage this variable instead
of checking the address offset.
Signed-off-by: Wei Yang
Reviewed-by: Dr. David Alan Gilbert
---
migration/ram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 5c05376d8d..b5759793a9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4067,7 +4067,7 @@ static int ram_load_postcopy(QEMUFile *f)
page_buffer = postcopy_host_page +
((uintptr_t)host & (block->page_size - 1));
/* If all TP are zero then we can optimise the place */
-if (!((uintptr_t)host & (block->page_size - 1))) {
+if (target_pages == 1) {
all_zero = true;
} else {
/* not the 1st TP within the HP */
--
2.17.1
[Patch v2 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size
In this case, page_buffer content would not be used.
Skip this to save some time.
Signed-off-by: Wei Yang
Reviewed-by: Dr. David Alan Gilbert
---
migration/ram.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 99a98b2da4..7938a643d9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4091,7 +4091,13 @@ static int ram_load_postcopy(QEMUFile *f)
switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
case RAM_SAVE_FLAG_ZERO:
ch = qemu_get_byte(f);
-memset(page_buffer, ch, TARGET_PAGE_SIZE);
+/*
+ * Can skip to set page_buffer when
+ * this is a zero page and (block->page_size == TARGET_PAGE_SIZE).
+ */
+if (ch || !matches_target_page_size) {
+memset(page_buffer, ch, TARGET_PAGE_SIZE);
+}
if (ch) {
all_zero = false;
}
--
2.17.1
[Patch v2 0/6] migration/postcopy: enable compress during postcopy
This patch set tries enable compress during postcopy. postcopy requires to place a whole host page, while migration thread migrate memory in target page size. This makes postcopy need to collect all target pages in one host page before placing via userfaultfd. To enable compress during postcopy, there are two problems to solve: 1. Random order for target page arrival 2. Target pages in one host page arrives without interrupt by target page from other host page The first one is handled by counting the number of target pages arrived instead of the last target page arrived. The second one is handled by: 1. Flush compress thread for each host page 2. Wait for decompress thread for before placing host page With the combination of these two changes, compress is enabled during postcopy. --- v2: * use uintptr_t to calculate place_dest * check target pages belongs to the same host page Wei Yang (6): migration/postcopy: reduce memset when it is zero page and matches_target_page_size migration/postcopy: wait for decompress thread in precopy migration/postcopy: count target page number to decide the place_needed migration/postcopy: set all_zero to true on the first target page migration/postcopy: enable random order target page arrival migration/postcopy: enable compress during postcopy migration/migration.c | 11 --- migration/ram.c | 67 +-- 2 files changed, 52 insertions(+), 26 deletions(-) -- 2.17.1
[Patch v2 3/6] migration/postcopy: count target page number to decide the place_needed
In postcopy, it requires to place whole host page instead of target
page.
Currently, it relies on the page offset to decide whether this is the
last target page. We also can count the target page number during the
iteration. When the number of target page equals
(host page size / target page size), this means it is the last target
page in the host page.
This is a preparation for non-ordered target page transmission.
Signed-off-by: Wei Yang
Reviewed-by: Dr. David Alan Gilbert
---
migration/ram.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index f59e3fe197..5c05376d8d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4017,6 +4017,7 @@ static int ram_load_postcopy(QEMUFile *f)
void *postcopy_host_page = mis->postcopy_tmp_page;
void *last_host = NULL;
bool all_zero = false;
+int target_pages = 0;
while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
ram_addr_t addr;
@@ -4051,6 +4052,7 @@ static int ram_load_postcopy(QEMUFile *f)
ret = -EINVAL;
break;
}
+target_pages++;
matches_target_page_size = block->page_size == TARGET_PAGE_SIZE;
/*
* Postcopy requires that we place whole host pages atomically;
@@ -4082,8 +4084,10 @@ static int ram_load_postcopy(QEMUFile *f)
* If it's the last part of a host page then we place the host
* page
*/
-place_needed = (((uintptr_t)host + TARGET_PAGE_SIZE) &
- (block->page_size - 1)) == 0;
+if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) {
+place_needed = true;
+target_pages = 0;
+}
place_source = postcopy_host_page;
}
last_host = host;
--
2.17.1
Re: [PATCH 1/2] migration/compress: compress QEMUFile is not writable
On Thu, Nov 07, 2019 at 11:59:10AM +, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> We open a file with empty_ops for compress QEMUFile, which means this is >> not writable. > >That explanation sounds reasonable; but I'm confused by the history of >this; the code was added by Liang Li in : > > b3be289 qemu-file: Fix qemu_put_compression_data flaw > > ( https://www.mail-archive.com/qemu-devel@nongnu.org/msg368974.html ) > >with almost exactly the opposite argument; can we figure out why? > Hmm... sounds interesting. Toke a look into the change log, which says Current qemu_put_compression_data can only work with no writable QEMUFile, and can't work with the writable QEMUFile. But it does not provide any measure to prevent users from using it with a writable QEMUFile. We should fix this flaw to make it works with writable QEMUFile. While I don't see a chance to use writable QEMUFile. Do I miss something? >Dave > -- Wei Yang Help you, Help me
Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
On Thu, Nov 07, 2019 at 09:15:44AM +, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> On Wed, Nov 06, 2019 at 08:11:44PM +, Dr. David Alan Gilbert wrote: >> >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> >> This patch set tries enable compress during postcopy. >> >> >> >> postcopy requires to place a whole host page, while migration thread >> >> migrate >> >> memory in target page size. This makes postcopy need to collect all target >> >> pages in one host page before placing via userfaultfd. >> >> >> >> To enable compress during postcopy, there are two problems to solve: >> >> >> >> 1. Random order for target page arrival >> >> 2. Target pages in one host page arrives without interrupt by target >> >>page from other host page >> >> >> >> The first one is handled by counting the number of target pages arrived >> >> instead of the last target page arrived. >> >> >> >> The second one is handled by: >> >> >> >> 1. Flush compress thread for each host page >> >> 2. Wait for decompress thread for before placing host page >> >> >> >> With the combination of these two changes, compress is enabled during >> >> postcopy. >> > >> >What have you tested this with? 2MB huge pages I guess? >> > >> >> I tried with this qemu option: >> >>-object >> memory-backend-file,id=mem1,mem-path=/dev/hugepages/guest2,size=4G \ >>-device pc-dimm,id=dimm1,memdev=mem1 >> >> /dev/hugepages/guest2 is a file under hugetlbfs >> >>hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M) > >OK, yes that should be fine. >I suspect on Power/ARM where they have normal memory with 16/64k pages, >the cost of the flush will mean compression is more expensive in >postcopy mode; but still makes it possible. > Not get your point clearly about more expensive. You mean more expensive on ARM/Power? If the solution looks good to you, I would prepare v2. >Dave > >> >Dave >> > >> >> Wei Yang (6): >> >> migration/postcopy: reduce memset when it is zero page and >> >> matches_target_page_size >> >> migration/postcopy: wait for decompress thread in precopy >> >> migration/postcopy: count target page number to decide the >> >> place_needed >> >> migration/postcopy: set all_zero to true on the first target page >> >> migration/postcopy: enable random order target page arrival >> >> migration/postcopy: enable compress during postcopy >> >> >> >> migration/migration.c | 11 >> >> migration/ram.c | 65 ++- >> >> 2 files changed, 45 insertions(+), 31 deletions(-) >> >> >> >> -- >> >> 2.17.1 >> >> >> >-- >> >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK >> >> -- >> Wei Yang >> Help you, Help me >-- >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Wei Yang Help you, Help me
Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
On Wed, Nov 06, 2019 at 08:11:44PM +, Dr. David Alan Gilbert wrote: >* Wei Yang (richardw.y...@linux.intel.com) wrote: >> This patch set tries enable compress during postcopy. >> >> postcopy requires to place a whole host page, while migration thread migrate >> memory in target page size. This makes postcopy need to collect all target >> pages in one host page before placing via userfaultfd. >> >> To enable compress during postcopy, there are two problems to solve: >> >> 1. Random order for target page arrival >> 2. Target pages in one host page arrives without interrupt by target >>page from other host page >> >> The first one is handled by counting the number of target pages arrived >> instead of the last target page arrived. >> >> The second one is handled by: >> >> 1. Flush compress thread for each host page >> 2. Wait for decompress thread for before placing host page >> >> With the combination of these two changes, compress is enabled during >> postcopy. > >What have you tested this with? 2MB huge pages I guess? > I tried with this qemu option: -object memory-backend-file,id=mem1,mem-path=/dev/hugepages/guest2,size=4G \ -device pc-dimm,id=dimm1,memdev=mem1 /dev/hugepages/guest2 is a file under hugetlbfs hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime,pagesize=2M) >Dave > >> Wei Yang (6): >> migration/postcopy: reduce memset when it is zero page and >> matches_target_page_size >> migration/postcopy: wait for decompress thread in precopy >> migration/postcopy: count target page number to decide the >> place_needed >> migration/postcopy: set all_zero to true on the first target page >> migration/postcopy: enable random order target page arrival >> migration/postcopy: enable compress during postcopy >> >> migration/migration.c | 11 >> migration/ram.c | 65 ++- >> 2 files changed, 45 insertions(+), 31 deletions(-) >> >> -- >> 2.17.1 >> >-- >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Wei Yang Help you, Help me
Re: [PATCH 5/6] migration/postcopy: enable random order target page arrival
On Wed, Nov 06, 2019 at 08:08:28PM +, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> After using number of target page received to track one host page, we
>> could have the capability to handle random order target page arrival in
>> one host page.
>>
>> This is a preparation for enabling compress during postcopy.
>>
>> Signed-off-by: Wei Yang
>> ---
>> migration/ram.c | 16 +++-
>> 1 file changed, 3 insertions(+), 13 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b5759793a9..da0596411c 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4015,7 +4015,6 @@ static int ram_load_postcopy(QEMUFile *f)
>> MigrationIncomingState *mis = migration_incoming_get_current();
>> /* Temporary page that is later 'placed' */
>> void *postcopy_host_page = mis->postcopy_tmp_page;
>> -void *last_host = NULL;
>> bool all_zero = false;
>> int target_pages = 0;
>>
>> @@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
>> * that's moved into place later.
>> * The migration protocol uses, possibly smaller, target-pages
>> * however the source ensures it always sends all the components
>> - * of a host page in order.
>> + * of a host page in one chunk.
>> */
>> page_buffer = postcopy_host_page +
>>((uintptr_t)host & (block->page_size - 1));
>> /* If all TP are zero then we can optimise the place */
>> if (target_pages == 1) {
>> all_zero = true;
>> -} else {
>> -/* not the 1st TP within the HP */
>> -if (host != (last_host + TARGET_PAGE_SIZE)) {
>> -error_report("Non-sequential target page %p/%p",
>> - host, last_host);
>> -ret = -EINVAL;
>> -break;
>> -}
>
>I think this is losing more protection than needed.
>I think you can still protect against a page from a different host-page
>arriving until we've placed the current host-page.
>So something like:
>
>if (((uintptr_t)host & ~(block->page_size - 1)) !=
>last_host)
>
OK, looks reasonable.
>and then set last_host to the start of the host page.
>
I think it is not necessary to update the last_host on each target page. We
can just set it at the first target page.
>Then you'll check if that flush is really working.
>
>Dave
>
>> }
>>
>> -
>> /*
>> * If it's the last part of a host page then we place the host
>> * page
>> @@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
>> }
>> place_source = postcopy_host_page;
>> }
>> -last_host = host;
>>
>> switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
>> case RAM_SAVE_FLAG_ZERO:
>> @@ -4143,7 +4132,8 @@ static int ram_load_postcopy(QEMUFile *f)
>>
>> if (!ret && place_needed) {
>> /* This gets called at the last target page in the host page */
>> -void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
>> +void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
>> + block->page_size);
>>
>> if (all_zero) {
>> ret = postcopy_place_page_zero(mis, place_dest,
>> --
>> 2.17.1
>>
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
[PATCH] target/i386: return directly from hyperv_init_vcpu() if hyperv not enabled
If hyperv is not enabled, related features are not set or enabled.
No extra work to do, return directly.
---
First time touch hyperv, hope my understanding is correct.
Signed-off-by: Wei Yang
---
target/i386/kvm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index bfd09bd441..6532904f0c 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1365,6 +1365,9 @@ static int hyperv_init_vcpu(X86CPU *cpu)
Error *local_err = NULL;
int ret;
+if (!hyperv_enabled(cpu))
+return 0;
+
if (cpu->hyperv_passthrough && hv_passthrough_mig_blocker == NULL) {
error_setg(_passthrough_mig_blocker,
"'hv-passthrough' CPU flag prevents migration, use explicit"
--
2.17.1
Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned
On Tue, Oct 29, 2019 at 07:04:19AM +, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> On Fri, Jul 19, 2019 at 07:06:51PM +0100, Dr. David Alan Gilbert wrote:
>> >* Paolo Bonzini (pbonz...@redhat.com) wrote:
>> >> On 19/07/19 19:54, Dr. David Alan Gilbert wrote:
>> >> >> -if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> >> >> -error_report("ram_block_discard_range: Unaligned end
>> >> >> address: %p",
>> >> >> - host_endaddr);
>> >> >> +if (length & (rb->page_size - 1)) {
>> >> >> +error_report("ram_block_discard_range: Unaligned length:
>> >> >> %lx",
>> >> >> + length);
>> >> > Yes, I *think* this is safe, we'll need to watch out for any warnings;
>> >>
>> >> Do you mean compiler or QEMU warning?
>> >
>> >No, I mean lots of these error reports being printed out in some common
>> >case.
>> >
>>
>> Hi, Dave
>>
>> Haven't see you for a period of time :-)
>
>I'm here (although been busy with virtiofs) - but this patch is exec.c
>so I think it needs to be picked by Paolo or rth.
>
Thanks Dave
Paolo
Do you feel comfortable with this?
>Dave
>
>> >Dave
>> >
>> > The patch is safe since there's an
>> >>
>> >> if ((uintptr_t)host_startaddr & (rb->page_size - 1)) {
>> >> error_report("ram_block_discard_range: Unaligned start address:
>> >> %p",
>> >> host_startaddr);
>> >> goto err;
>> >> }
>> >>
>> >> just before this context.
>> >>
>> >> Paolo
>> >--
>> >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>>
>> --
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
Re: [Qemu-devel] [PATCH] migration: check length directly to make sure the range is aligned
On Fri, Jul 19, 2019 at 07:06:51PM +0100, Dr. David Alan Gilbert wrote:
>* Paolo Bonzini (pbonz...@redhat.com) wrote:
>> On 19/07/19 19:54, Dr. David Alan Gilbert wrote:
>> >> -if ((uintptr_t)host_endaddr & (rb->page_size - 1)) {
>> >> -error_report("ram_block_discard_range: Unaligned end
>> >> address: %p",
>> >> - host_endaddr);
>> >> +if (length & (rb->page_size - 1)) {
>> >> +error_report("ram_block_discard_range: Unaligned length:
>> >> %lx",
>> >> + length);
>> > Yes, I *think* this is safe, we'll need to watch out for any warnings;
>>
>> Do you mean compiler or QEMU warning?
>
>No, I mean lots of these error reports being printed out in some common
>case.
>
Hi, Dave
Haven't see you for a period of time :-)
>Dave
>
> The patch is safe since there's an
>>
>> if ((uintptr_t)host_startaddr & (rb->page_size - 1)) {
>> error_report("ram_block_discard_range: Unaligned start address: %p",
>> host_startaddr);
>> goto err;
>> }
>>
>> just before this context.
>>
>> Paolo
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
[PATCH v2 4/6] migration/multifd: used must not be 0 for a pending job
After thread synchronization request is handled in another case, this
means when we only get pending_job when there is used pages.
Signed-off-by: Wei Yang
---
migration/ram.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 62072b7a35..12c270e86d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1132,12 +1132,11 @@ static void *multifd_send_thread(void *opaque)
break;
}
-if (used) {
-ret = qio_channel_writev_all(p->c, p->pages->iov,
- used, _err);
-if (ret != 0) {
-break;
-}
+assert(used);
+ret = qio_channel_writev_all(p->c, p->pages->iov,
+ used, _err);
+if (ret != 0) {
+break;
}
qemu_mutex_lock(>mutex);
--
2.17.1
[PATCH v2 1/6] migration/multifd: move Params update and pages cleanup into multifd_send_fill_packet()
Fill data and update/cleanup related field in one place. Also make the
code a little clean.
Signed-off-by: Wei Yang
---
migration/ram.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 5876054195..35f147388b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -789,15 +789,16 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
g_free(pages);
}
-static void multifd_send_fill_packet(MultiFDSendParams *p)
+static void multifd_send_fill_packet(MultiFDSendParams *p, uint32_t used)
{
MultiFDPacket_t *packet = p->packet;
+uint32_t next_packet_size = used * qemu_target_page_size();
int i;
packet->flags = cpu_to_be32(p->flags);
packet->pages_alloc = cpu_to_be32(p->pages->allocated);
packet->pages_used = cpu_to_be32(p->pages->used);
-packet->next_packet_size = cpu_to_be32(p->next_packet_size);
+packet->next_packet_size = cpu_to_be32(next_packet_size);
packet->packet_num = cpu_to_be64(p->packet_num);
if (p->pages->block) {
@@ -807,6 +808,13 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
for (i = 0; i < p->pages->used; i++) {
packet->offset[i] = cpu_to_be64(p->pages->offset[i]);
}
+
+p->next_packet_size = next_packet_size;
+p->flags = 0;
+p->num_packets++;
+p->num_pages += used;
+p->pages->used = 0;
+p->pages->block = NULL;
}
static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
@@ -1109,13 +1117,7 @@ static void *multifd_send_thread(void *opaque)
uint64_t packet_num = p->packet_num;
flags = p->flags;
-p->next_packet_size = used * qemu_target_page_size();
-multifd_send_fill_packet(p);
-p->flags = 0;
-p->num_packets++;
-p->num_pages += used;
-p->pages->used = 0;
-p->pages->block = NULL;
+multifd_send_fill_packet(p, used);
qemu_mutex_unlock(>mutex);
trace_multifd_send(p->id, packet_num, used, flags,
--
2.17.1
[PATCH v2 0/6] migration/multifd: a new mechanism for send thread sync
Current send thread could work while the sync mechanism has some problem: * has spuriously wakeup * number of channels_ready will *overflow* the number of real channels The reason is: * if MULTIFD_FLAG_SYNC is set in the middle of send thread running, there is one more spurious wakeup * if MULTIFD_FLAG_SYNC is set when send thread is not running, there is one more channels_ready be triggered To solve this situation, one new mechanism is introduced to synchronize send threads. The idea is simple, a new field *sync* is introduced to indicate a synchronization is required. --- v2: rebase on latest code Wei Yang (6): migration/multifd: move Params update and pages cleanup into multifd_send_fill_packet() migration/multifd: notify channels_ready when send thread starts migration/multifd: use sync field to synchronize send threads migration/multifd: used must not be 0 for a pending job migration/multifd: use boolean for pending_job is enough migration/multifd: there is no spurious wakeup now migration/ram.c | 74 +++-- 1 file changed, 47 insertions(+), 27 deletions(-) -- 2.17.1
[PATCH v2 6/6] migration/multifd: there is no spurious wakeup now
The spurious wakeup is gone.
Signed-off-by: Wei Yang
---
migration/ram.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index fccdbfabc5..73ace40b1b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1168,8 +1168,8 @@ static void *multifd_send_thread(void *opaque)
qemu_mutex_unlock(>mutex);
break;
} else {
-qemu_mutex_unlock(>mutex);
-/* sometimes there are spurious wakeups */
+/* no other case should trigger me */
+g_assert_not_reached();
}
}
--
2.17.1
[PATCH v2 5/6] migration/multifd: use boolean for pending_job is enough
After synchronization request is handled in another case, there only
could be one pending_job for one send thread at most.
This is fine to use boolean to represent this behavior.
Signed-off-by: Wei Yang
---
migration/ram.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 12c270e86d..fccdbfabc5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -646,7 +646,7 @@ typedef struct {
/* should this thread finish */
bool quit;
/* thread has work to do */
-int pending_job;
+bool pending_job;
/* array of pages to sent */
MultiFDPages_t *pages;
/* packet allocated len */
@@ -933,7 +933,7 @@ static int multifd_send_pages(RAMState *rs)
return -1;
}
if (!p->pending_job) {
-p->pending_job++;
+p->pending_job = true;
next_channel = (i + 1) % migrate_multifd_channels();
break;
}
@@ -1140,7 +1140,7 @@ static void *multifd_send_thread(void *opaque)
}
qemu_mutex_lock(>mutex);
-p->pending_job--;
+p->pending_job = false;
qemu_mutex_unlock(>mutex);
qemu_sem_post(_send_state->channels_ready);
@@ -1238,8 +1238,7 @@ int multifd_save_setup(void)
qemu_mutex_init(>mutex);
qemu_sem_init(>sem, 0);
qemu_sem_init(>sem_sync, 0);
-p->quit = p->sync = false;
-p->pending_job = 0;
+p->quit = p->sync = p->pending_job = false;
p->id = i;
p->pages = multifd_pages_init(page_count);
p->packet_len = sizeof(MultiFDPacket_t)
--
2.17.1
[PATCH v2 2/6] migration/multifd: notify channels_ready when send thread starts
multifd_send_state->channels_ready is initialized to 0. It is proper to
let main thread know we are ready when thread start running.
Current implementation works since ram_save_setup() calls
multifd_send_sync_main() which wake up send thread and posts
channels_ready. This behavior will introduce some unpredictable
situation and disturb the semaphore value.
This is a preparation patch to use another mechanism to do send thread
synchronization to avoid post channels_ready in this case. So this patch
posts channels_ready when send threads start running.
Signed-off-by: Wei Yang
---
migration/ram.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/ram.c b/migration/ram.c
index 35f147388b..25d477796e 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1107,6 +1107,8 @@ static void *multifd_send_thread(void *opaque)
}
/* initial packet */
p->num_packets = 1;
+/* let main thread know we are ready */
+qemu_sem_post(_send_state->channels_ready);
while (true) {
qemu_sem_wait(>sem);
--
2.17.1
[PATCH v2 3/6] migration/multifd: use sync field to synchronize send threads
Add a field in MultiFDSendParams to indicate there is a request to
synchronize send threads.
By doing so, send_thread will just post sem_sync on synchronization
request and channels_ready will not *overflow*.
Signed-off-by: Wei Yang
---
migration/ram.c | 30 --
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 25d477796e..62072b7a35 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -641,6 +641,8 @@ typedef struct {
QemuMutex mutex;
/* is this channel thread running */
bool running;
+/* should sync this channel */
+bool sync;
/* should this thread finish */
bool quit;
/* thread has work to do */
@@ -1074,8 +1076,7 @@ static void multifd_send_sync_main(RAMState *rs)
}
p->packet_num = multifd_send_state->packet_num++;
-p->flags |= MULTIFD_FLAG_SYNC;
-p->pending_job++;
+p->sync = true;
qemu_file_update_transfer(rs->f, p->packet_len);
ram_counters.multifd_bytes += p->packet_len;
ram_counters.transferred += p->packet_len;
@@ -1143,10 +1144,27 @@ static void *multifd_send_thread(void *opaque)
p->pending_job--;
qemu_mutex_unlock(>mutex);
-if (flags & MULTIFD_FLAG_SYNC) {
-qemu_sem_post(>sem_sync);
-}
qemu_sem_post(_send_state->channels_ready);
+} else if (p->sync) {
+uint64_t packet_num = p->packet_num;
+uint32_t flags = p->flags;
+assert(!p->pages->used);
+
+p->flags |= MULTIFD_FLAG_SYNC;
+multifd_send_fill_packet(p, 0);
+p->sync = false;
+qemu_mutex_unlock(>mutex);
+
+trace_multifd_send(p->id, packet_num, 0, flags | MULTIFD_FLAG_SYNC,
+ p->next_packet_size);
+
+ret = qio_channel_write_all(p->c, (void *)p->packet,
+p->packet_len, _err);
+if (ret != 0) {
+break;
+}
+
+qemu_sem_post(>sem_sync);
} else if (p->quit) {
qemu_mutex_unlock(>mutex);
break;
@@ -1221,7 +1239,7 @@ int multifd_save_setup(void)
qemu_mutex_init(>mutex);
qemu_sem_init(>sem, 0);
qemu_sem_init(>sem_sync, 0);
-p->quit = false;
+p->quit = p->sync = false;
p->pending_job = 0;
p->id = i;
p->pages = multifd_pages_init(page_count);
--
2.17.1
[PATCH 1/2] migration/multifd: clean pages after filling packet
This is a preparation for the next patch: not use multifd during postcopy. Without enabling postcopy, everything looks good. While after enabling postcopy, migration may fail even not use multifd during postcopy. The reason is the pages is not properly cleared and *old* target page will continue to be transferred. After clean pages, migration succeeds. Signed-off-by: Wei Yang --- migration/ram.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/migration/ram.c b/migration/ram.c index 80dd2d55f9..7087bb73ed 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -929,10 +929,10 @@ static int multifd_send_pages(RAMState *rs) } qemu_mutex_unlock(>mutex); } -p->pages->used = 0; +assert(!p->pages->used); +assert(!p->pages->block); p->packet_num = multifd_send_state->packet_num++; -p->pages->block = NULL; multifd_send_state->pages = p->pages; p->pages = pages; transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + p->packet_len; @@ -1114,6 +1114,8 @@ static void *multifd_send_thread(void *opaque) p->flags = 0; p->num_packets++; p->num_pages += used; +p->pages->used = 0; +p->pages->block = NULL; qemu_mutex_unlock(>mutex); trace_multifd_send(p->id, packet_num, used, flags, -- 2.17.1
[PATCH 0/2] not use multifd during postcopy
We don't support multifd during postcopy, but user still could enable both multifd and postcopy. This leads to migration failure. Patch 1 does proper cleanup, otherwise we may have data corruption. Patch 2 does the main job. BTW, current multifd synchronization method needs a cleanup. Will send another patch set. Wei Yang (2): migration/multifd: clean pages after filling packet migration/multifd: not use multifd during postcopy migration/ram.c | 15 ++- 1 file changed, 10 insertions(+), 5 deletions(-) -- 2.17.1
[PATCH 2/2] migration/multifd: not use multifd during postcopy
We don't support multifd during postcopy, but user still could enable
both multifd and postcopy. This leads to migration failure.
Skip multifd during postcopy.
Signed-off-by: Wei Yang
---
migration/ram.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 7087bb73ed..5876054195 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2547,10 +2547,13 @@ static int ram_save_target_page(RAMState *rs,
PageSearchStatus *pss,
}
/*
- * do not use multifd for compression as the first page in the new
- * block should be posted out before sending the compressed page
+ * Do not use multifd for:
+ * 1. Compression as the first page in the new block should be posted out
+ *before sending the compressed page
+ * 2. In postcopy as one whole host page should be placed
*/
-if (!save_page_use_compression(rs) && migrate_use_multifd()) {
+if (!save_page_use_compression(rs) && migrate_use_multifd()
+&& !migration_in_postcopy()) {
return ram_save_multifd_page(rs, block, offset);
}
--
2.17.1
Re: [PATCH 0/6] migration/postcopy: enable compress during postcopy
On Fri, Oct 18, 2019 at 09:50:05AM -0700, no-re...@patchew.org wrote: >Patchew URL: >https://patchew.org/QEMU/20191018004850.9888-1-richardw.y...@linux.intel.com/ > > > >Hi, > >This series failed the docker-mingw@fedora build test. Please find the testing >commands and >their output below. If you have Docker installed, you can probably reproduce it >locally. > >=== TEST SCRIPT BEGIN === >#! /bin/bash >export ARCH=x86_64 >make docker-image-fedora V=1 NETWORK=1 >time make docker-test-mingw@fedora J=14 NETWORK=1 >=== TEST SCRIPT END === > > CC aarch64-softmmu/hw/timer/allwinner-a10-pit.o >In file included from /tmp/qemu-test/src/migration/ram.c:29: >/tmp/qemu-test/src/migration/ram.c: In function 'ram_load_postcopy': >/tmp/qemu-test/src/migration/ram.c:4177:56: error: cast from pointer to >integer of different size [-Werror=pointer-to-int-cast] > void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host, >^ Sounds should use uintptr_t. Would change it in next version. >/tmp/qemu-test/src/include/qemu/osdep.h:268:33: note: in definition of macro >'QEMU_ALIGN_DOWN' > #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m)) > ^ >cc1: all warnings being treated as errors >make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1 >make[1]: *** Waiting for unfinished jobs > CC x86_64-softmmu/target/i386/arch_dump.o > CC aarch64-softmmu/hw/usb/tusb6010.o >--- > CC aarch64-softmmu/hw/arm/xlnx-zynqmp.o >In file included from /tmp/qemu-test/src/migration/ram.c:29: >/tmp/qemu-test/src/migration/ram.c: In function 'ram_load_postcopy': >/tmp/qemu-test/src/migration/ram.c:4177:56: error: cast from pointer to >integer of different size [-Werror=pointer-to-int-cast] > void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host, >^ >/tmp/qemu-test/src/include/qemu/osdep.h:268:33: note: in definition of macro >'QEMU_ALIGN_DOWN' > #define QEMU_ALIGN_DOWN(n, m) ((n) / (m) * (m)) > ^ >cc1: all warnings being treated as errors >make[1]: *** [/tmp/qemu-test/src/rules.mak:69: migration/ram.o] Error 1 >make[1]: *** Waiting for unfinished jobs >make: *** [Makefile:482: aarch64-softmmu/all] Error 2 >make: *** Waiting for unfinished jobs >make: *** [Makefile:482: x86_64-softmmu/all] Error 2 >Traceback (most recent call last): > File "./tests/docker/docker.py", line 662, in >sys.exit(main()) >--- >raise CalledProcessError(retcode, cmd) >subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', >'--label', 'com.qemu.instance.uuid=90570434880344249cff701baa188163', '-u', >'1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', >'-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', >'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', >'/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', >'/var/tmp/patchew-tester-tmp-dh8p6f27/src/docker-src.2019-10-18-12.47.19.4164:/var/tmp/qemu:z,ro', > 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit >status 2. >filter=--filter=label=com.qemu.instance.uuid=90570434880344249cff701baa188163 >make[1]: *** [docker-run] Error 1 >make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-dh8p6f27/src' >make: *** [docker-run-test-mingw@fedora] Error 2 > >real2m45.691s >user0m8.390s > > >The full log is available at >http://patchew.org/logs/20191018004850.9888-1-richardw.y...@linux.intel.com/testing.docker-mingw@fedora/?type=message. >--- >Email generated automatically by Patchew [https://patchew.org/]. >Please send your feedback to patchew-de...@redhat.com -- Wei Yang Help you, Help me
[PATCH 5/6] migration/postcopy: enable random order target page arrival
After using number of target page received to track one host page, we
could have the capability to handle random order target page arrival in
one host page.
This is a preparation for enabling compress during postcopy.
Signed-off-by: Wei Yang
---
migration/ram.c | 16 +++-
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index b5759793a9..da0596411c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4015,7 +4015,6 @@ static int ram_load_postcopy(QEMUFile *f)
MigrationIncomingState *mis = migration_incoming_get_current();
/* Temporary page that is later 'placed' */
void *postcopy_host_page = mis->postcopy_tmp_page;
-void *last_host = NULL;
bool all_zero = false;
int target_pages = 0;
@@ -4062,24 +4061,15 @@ static int ram_load_postcopy(QEMUFile *f)
* that's moved into place later.
* The migration protocol uses, possibly smaller, target-pages
* however the source ensures it always sends all the components
- * of a host page in order.
+ * of a host page in one chunk.
*/
page_buffer = postcopy_host_page +
((uintptr_t)host & (block->page_size - 1));
/* If all TP are zero then we can optimise the place */
if (target_pages == 1) {
all_zero = true;
-} else {
-/* not the 1st TP within the HP */
-if (host != (last_host + TARGET_PAGE_SIZE)) {
-error_report("Non-sequential target page %p/%p",
- host, last_host);
-ret = -EINVAL;
-break;
-}
}
-
/*
* If it's the last part of a host page then we place the host
* page
@@ -4090,7 +4080,6 @@ static int ram_load_postcopy(QEMUFile *f)
}
place_source = postcopy_host_page;
}
-last_host = host;
switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
case RAM_SAVE_FLAG_ZERO:
@@ -4143,7 +4132,8 @@ static int ram_load_postcopy(QEMUFile *f)
if (!ret && place_needed) {
/* This gets called at the last target page in the host page */
-void *place_dest = host + TARGET_PAGE_SIZE - block->page_size;
+void *place_dest = (void *)QEMU_ALIGN_DOWN((unsigned long)host,
+ block->page_size);
if (all_zero) {
ret = postcopy_place_page_zero(mis, place_dest,
--
2.17.1
[PATCH 6/6] migration/postcopy: enable compress during postcopy
postcopy requires to place a whole host page, while migration thread
migrate memory in target page size. This makes postcopy need to collect
all target pages in one host page before placing via userfaultfd.
To enable compress during postcopy, there are two problems to solve:
1. Random order for target page arrival
2. Target pages in one host page arrives without interrupt by target
page from other host page
The first one is handled by previous cleanup patch.
This patch handles the second one by:
1. Flush compress thread for each host page
2. Wait for decompress thread for before placing host page
Signed-off-by: Wei Yang
---
migration/migration.c | 11 ---
migration/ram.c | 28 +++-
2 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 3febd0f8f3..72e53e2249 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1000,17 +1000,6 @@ static bool migrate_caps_check(bool *cap_list,
#endif
if (cap_list[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
-if (cap_list[MIGRATION_CAPABILITY_COMPRESS]) {
-/* The decompression threads asynchronously write into RAM
- * rather than use the atomic copies needed to avoid
- * userfaulting. It should be possible to fix the decompression
- * threads for compatibility in future.
- */
-error_setg(errp, "Postcopy is not currently compatible "
- "with compression");
-return false;
-}
-
/* This check is reasonably expensive, so only when it's being
* set the first time, also it's only the destination that needs
* special support.
diff --git a/migration/ram.c b/migration/ram.c
index da0596411c..1403978d75 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3449,6 +3449,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
rs->target_page_count += pages;
+/*
+ * During postcopy, it is necessary to make sure one whole host
+ * page is sent in one chunk.
+ */
+if (migrate_postcopy_ram()) {
+flush_compressed_data(rs);
+}
+
/*
* we want to check in the 1st loop, just in case it was the 1st
* time and we had to sync the dirty bitmap.
@@ -4025,6 +4033,7 @@ static int ram_load_postcopy(QEMUFile *f)
void *place_source = NULL;
RAMBlock *block = NULL;
uint8_t ch;
+int len;
addr = qemu_get_be64(f);
@@ -4042,7 +4051,8 @@ static int ram_load_postcopy(QEMUFile *f)
trace_ram_load_postcopy_loop((uint64_t)addr, flags);
place_needed = false;
-if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE)) {
+if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
+ RAM_SAVE_FLAG_COMPRESS_PAGE)) {
block = ram_block_from_stream(f, flags);
host = host_from_ram_block_offset(block, addr);
@@ -4114,6 +4124,17 @@ static int ram_load_postcopy(QEMUFile *f)
TARGET_PAGE_SIZE);
}
break;
+case RAM_SAVE_FLAG_COMPRESS_PAGE:
+all_zero = false;
+len = qemu_get_be32(f);
+if (len < 0 || len > compressBound(TARGET_PAGE_SIZE)) {
+error_report("Invalid compressed data length: %d", len);
+ret = -EINVAL;
+break;
+}
+decompress_data_with_multi_threads(f, page_buffer, len);
+break;
+
case RAM_SAVE_FLAG_EOS:
/* normal exit */
multifd_recv_sync_main();
@@ -4125,6 +4146,11 @@ static int ram_load_postcopy(QEMUFile *f)
break;
}
+/* Got the whole host page, wait for decompress before placing. */
+if (place_needed) {
+ret |= wait_for_decompress_done();
+}
+
/* Detect for any possible file errors */
if (!ret && qemu_file_get_error(f)) {
ret = qemu_file_get_error(f);
--
2.17.1
[PATCH 3/6] migration/postcopy: count target page number to decide the place_needed
In postcopy, it requires to place whole host page instead of target
page.
Currently, it relies on the page offset to decide whether this is the
last target page. We also can count the target page number during the
iteration. When the number of target page equals
(host page size / target page size), this means it is the last target
page in the host page.
This is a preparation for non-ordered target page transmission.
Signed-off-by: Wei Yang
---
migration/ram.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index f59e3fe197..5c05376d8d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4017,6 +4017,7 @@ static int ram_load_postcopy(QEMUFile *f)
void *postcopy_host_page = mis->postcopy_tmp_page;
void *last_host = NULL;
bool all_zero = false;
+int target_pages = 0;
while (!ret && !(flags & RAM_SAVE_FLAG_EOS)) {
ram_addr_t addr;
@@ -4051,6 +4052,7 @@ static int ram_load_postcopy(QEMUFile *f)
ret = -EINVAL;
break;
}
+target_pages++;
matches_target_page_size = block->page_size == TARGET_PAGE_SIZE;
/*
* Postcopy requires that we place whole host pages atomically;
@@ -4082,8 +4084,10 @@ static int ram_load_postcopy(QEMUFile *f)
* If it's the last part of a host page then we place the host
* page
*/
-place_needed = (((uintptr_t)host + TARGET_PAGE_SIZE) &
- (block->page_size - 1)) == 0;
+if (target_pages == (block->page_size / TARGET_PAGE_SIZE)) {
+place_needed = true;
+target_pages = 0;
+}
place_source = postcopy_host_page;
}
last_host = host;
--
2.17.1
[PATCH 0/6] migration/postcopy: enable compress during postcopy
This patch set tries enable compress during postcopy. postcopy requires to place a whole host page, while migration thread migrate memory in target page size. This makes postcopy need to collect all target pages in one host page before placing via userfaultfd. To enable compress during postcopy, there are two problems to solve: 1. Random order for target page arrival 2. Target pages in one host page arrives without interrupt by target page from other host page The first one is handled by counting the number of target pages arrived instead of the last target page arrived. The second one is handled by: 1. Flush compress thread for each host page 2. Wait for decompress thread for before placing host page With the combination of these two changes, compress is enabled during postcopy. Wei Yang (6): migration/postcopy: reduce memset when it is zero page and matches_target_page_size migration/postcopy: wait for decompress thread in precopy migration/postcopy: count target page number to decide the place_needed migration/postcopy: set all_zero to true on the first target page migration/postcopy: enable random order target page arrival migration/postcopy: enable compress during postcopy migration/migration.c | 11 migration/ram.c | 65 ++- 2 files changed, 45 insertions(+), 31 deletions(-) -- 2.17.1
[PATCH 1/6] migration/postcopy: reduce memset when it is zero page and matches_target_page_size
In this case, page_buffer content would not be used.
Skip this to save some time.
Signed-off-by: Wei Yang
---
migration/ram.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 99a98b2da4..7938a643d9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4091,7 +4091,13 @@ static int ram_load_postcopy(QEMUFile *f)
switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
case RAM_SAVE_FLAG_ZERO:
ch = qemu_get_byte(f);
-memset(page_buffer, ch, TARGET_PAGE_SIZE);
+/*
+ * Can skip to set page_buffer when
+ * this is a zero page and (block->page_size == TARGET_PAGE_SIZE).
+ */
+if (ch || !matches_target_page_size) {
+memset(page_buffer, ch, TARGET_PAGE_SIZE);
+}
if (ch) {
all_zero = false;
}
--
2.17.1
[PATCH 2/6] migration/postcopy: wait for decompress thread in precopy
Compress is not supported with postcopy, it is safe to wait for
decompress thread just in precopy.
This is a preparation for later patch.
Signed-off-by: Wei Yang
---
migration/ram.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 7938a643d9..f59e3fe197 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4375,6 +4375,7 @@ static int ram_load_precopy(QEMUFile *f)
}
}
+ret |= wait_for_decompress_done();
return ret;
}
@@ -4406,8 +4407,6 @@ static int ram_load(QEMUFile *f, void *opaque, int
version_id)
} else {
ret = ram_load_precopy(f);
}
-
-ret |= wait_for_decompress_done();
}
trace_ram_load_complete(ret, seq_iter);
--
2.17.1
[PATCH 4/6] migration/postcopy: set all_zero to true on the first target page
For the first target page, all_zero is set to true for this round check.
After target_pages introduced, we could leverage this variable instead
of checking the address offset.
Signed-off-by: Wei Yang
---
migration/ram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 5c05376d8d..b5759793a9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4067,7 +4067,7 @@ static int ram_load_postcopy(QEMUFile *f)
page_buffer = postcopy_host_page +
((uintptr_t)host & (block->page_size - 1));
/* If all TP are zero then we can optimise the place */
-if (!((uintptr_t)host & (block->page_size - 1))) {
+if (target_pages == 1) {
all_zero = true;
} else {
/* not the 1st TP within the HP */
--
2.17.1
[Patch v2] checkpatch: sugguest to use qemu_real_host_page_size instead of getpagesize() or sysconf(_SC_PAGESIZE)
Signed-off-by: Wei Yang
CC: Richard Henderson
CC: Stefan Hajnoczi
---
v2: add "\b" for better match, suggested by Richard Henderson
---
scripts/checkpatch.pl | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index aa9a354a0e..ab68a16fd2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2915,6 +2915,12 @@ sub process {
if ($line =~ /\bbzero\(/) {
ERROR("use memset() instead of bzero()\n" . $herecurr);
}
+ if ($line =~ /\bgetpagesize\(\)/) {
+ ERROR("use qemu_real_host_page_size instead of
getpagesize()\n" . $herecurr);
+ }
+ if ($line =~ /\bsysconf\(_SC_PAGESIZE\)/) {
+ ERROR("use qemu_real_host_page_size instead of
sysconf(_SC_PAGESIZE)\n" . $herecurr);
+ }
my $non_exit_glib_asserts = qr{g_assert_cmpstr|
g_assert_cmpint|
g_assert_cmpuint|
--
2.17.1
Re: [PATCH] checkpatch: sugguest to use qemu_real_host_page_size instead of getpagesize() or sysconf(_SC_PAGESIZE)
On Wed, Oct 16, 2019 at 07:48:50PM +0100, Stefan Hajnoczi wrote:
>On Wed, Oct 16, 2019 at 09:24:32AM +0800, Wei Yang wrote:
>> Signed-off-by: Wei Yang
>> CC: David Gibson
>> ---
>> scripts/checkpatch.pl | 6 ++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index aa9a354a0e..4b360ed310 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2915,6 +2915,12 @@ sub process {
>> if ($line =~ /\bbzero\(/) {
>> ERROR("use memset() instead of bzero()\n" . $herecurr);
>> }
>> +if ($line =~ /getpagesize\(\)/) {
>> +ERROR("use qemu_real_host_page_size instead of
>> getpagesize()\n" . $herecurr);
>> +}
>> +if ($line =~ /sysconf\(_SC_PAGESIZE\)/) {
>> +ERROR("use qemu_real_host_page_size instead of
>> sysconf(_SC_PAGESIZE)\n" . $herecurr);
>> +}
>> my $non_exit_glib_asserts = qr{g_assert_cmpstr|
>> g_assert_cmpint|
>> g_assert_cmpuint|
>
>Just wanted to say thank you for extending checkpatch.pl! We don't do
>it enough but it's the best way to extend QEMU coding style because it's
>automated :).
>
You are welcome. Glad to do something.
>Stefan
--
Wei Yang
Help you, Help me
Re: [PATCH] checkpatch: sugguest to use qemu_real_host_page_size instead of getpagesize() or sysconf(_SC_PAGESIZE)
On Wed, Oct 16, 2019 at 08:43:32AM -0700, Richard Henderson wrote:
>On 10/15/19 6:24 PM, Wei Yang wrote:
>> if ($line =~ /\bbzero\(/) {
>> ERROR("use memset() instead of bzero()\n" . $herecurr);
>> }
>> +if ($line =~ /getpagesize\(\)/) {
>> +ERROR("use qemu_real_host_page_size instead of
>> getpagesize()\n" . $herecurr);
>> +}
>> +if ($line =~ /sysconf\(_SC_PAGESIZE\)/) {
>
>Use \b to match beginning of symbol like bzero did?
>
You are right, thanks for the suggestion.
>
>r~
--
Wei Yang
Help you, Help me
Re: [PATCH] checkpatch: sugguest to use qemu_real_host_page_size instead of getpagesize() or sysconf(_SC_PAGESIZE)
On Wed, Oct 16, 2019 at 03:25:53AM -0700, no-re...@patchew.org wrote:
>Patchew URL:
>https://patchew.org/QEMU/20191016012432.22731-1-richardw.y...@linux.intel.com/
>
>
>
>Hi,
>
>This series seems to have some coding style problems. See output below for
>more information:
>
>Subject: [PATCH] checkpatch: sugguest to use qemu_real_host_page_size instead
>of getpagesize() or sysconf(_SC_PAGESIZE)
>Type: series
>Message-id: 20191016012432.22731-1-richardw.y...@linux.intel.com
>
>=== TEST SCRIPT BEGIN ===
>#!/bin/bash
>git rev-parse base > /dev/null || exit 0
>git config --local diff.renamelimit 0
>git config --local diff.renames True
>git config --local diff.algorithm histogram
>./scripts/checkpatch.pl --mailback base..
>=== TEST SCRIPT END ===
>
>Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>Switched to a new branch 'test'
>6c3e035 checkpatch: sugguest to use qemu_real_host_page_size instead of
>getpagesize() or sysconf(_SC_PAGESIZE)
>
>=== OUTPUT BEGIN ===
>ERROR: line over 90 characters
>#20: FILE: scripts/checkpatch.pl:2919:
>+ ERROR("use qemu_real_host_page_size instead of
>getpagesize()\n" . $herecurr);
>
Since this is an error message and I see other similar code keep it in the
same line, I didn't split it into two lines.
If necessary, I would split it.
--
Wei Yang
Help you, Help me
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Sun, Oct 13, 2019 at 08:28:41PM +1100, David Gibson wrote: >On Sun, Oct 13, 2019 at 10:11:45AM +0800, Wei Yang wrote: >> There are three page size in qemu: >> >> real host page size >> host page size >> target page size >> >> All of them have dedicate variable to represent. For the last two, we >> use the same form in the whole qemu project, while for the first one we >> use two forms: qemu_real_host_page_size and getpagesize(). >> >> qemu_real_host_page_size is defined to be a replacement of >> getpagesize(), so let it serve the role. >> >> [Note] Not fully tested for some arch or device. >> >> Signed-off-by: Wei Yang > >Reviewed-by: David Gibson > >Although the chances of someone messing this up again are almost 100%. > Hi, David I found put a check in checkpatch.pl may be a good way to prevent it. Just draft a patch, hope you would like it. >-- >David Gibson | I'll have my music baroque, and my code >david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! >http://www.ozlabs.org/~dgibson -- Wei Yang Help you, Help me
[PATCH] checkpatch: sugguest to use qemu_real_host_page_size instead of getpagesize() or sysconf(_SC_PAGESIZE)
Signed-off-by: Wei Yang
CC: David Gibson
---
scripts/checkpatch.pl | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index aa9a354a0e..4b360ed310 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2915,6 +2915,12 @@ sub process {
if ($line =~ /\bbzero\(/) {
ERROR("use memset() instead of bzero()\n" . $herecurr);
}
+ if ($line =~ /getpagesize\(\)/) {
+ ERROR("use qemu_real_host_page_size instead of
getpagesize()\n" . $herecurr);
+ }
+ if ($line =~ /sysconf\(_SC_PAGESIZE\)/) {
+ ERROR("use qemu_real_host_page_size instead of
sysconf(_SC_PAGESIZE)\n" . $herecurr);
+ }
my $non_exit_glib_asserts = qr{g_assert_cmpstr|
g_assert_cmpint|
g_assert_cmpuint|
--
2.17.1
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Tue, Oct 15, 2019 at 02:45:15PM +0300, Yuval Shaia wrote:
>On Sun, Oct 13, 2019 at 10:11:45AM +0800, Wei Yang wrote:
>> There are three page size in qemu:
>>
>> real host page size
>> host page size
>> target page size
>>
>> All of them have dedicate variable to represent. For the last two, we
>> use the same form in the whole qemu project, while for the first one we
>> use two forms: qemu_real_host_page_size and getpagesize().
>>
>> qemu_real_host_page_size is defined to be a replacement of
>> getpagesize(), so let it serve the role.
>>
>> [Note] Not fully tested for some arch or device.
>>
>> Signed-off-by: Wei Yang
>> ---
>> accel/kvm/kvm-all.c| 6 +++---
>> backends/hostmem.c | 2 +-
>> block.c| 4 ++--
>> block/file-posix.c | 9 +
>> block/io.c | 2 +-
>> block/parallels.c | 2 +-
>> block/qcow2-cache.c| 2 +-
>> contrib/vhost-user-gpu/vugbm.c | 2 +-
>> exec.c | 6 +++---
>> hw/intc/s390_flic_kvm.c| 2 +-
>> hw/ppc/mac_newworld.c | 2 +-
>> hw/ppc/spapr_pci.c | 2 +-
>> hw/rdma/vmw/pvrdma_main.c | 2 +-
>
>for pvrdma stuff:
>
>Reviewed-by: Yuval Shaia
>Tested-by: Yuval Shaia
Thanks
>
>> hw/vfio/spapr.c| 7 ---
>> include/exec/ram_addr.h| 2 +-
>> include/qemu/osdep.h | 4 ++--
>> migration/migration.c | 2 +-
>> migration/postcopy-ram.c | 4 ++--
>> monitor/misc.c | 2 +-
>> target/ppc/kvm.c | 2 +-
>> tests/vhost-user-bridge.c | 8
>> util/mmap-alloc.c | 10 +-
>> util/oslib-posix.c | 4 ++--
>> util/oslib-win32.c | 2 +-
>> util/vfio-helpers.c| 12 ++--
>> 25 files changed, 52 insertions(+), 50 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index d2d96d73e8..140b0bd8f6 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -52,7 +52,7 @@
>> /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>> * need to use the real host PAGE_SIZE, as that's what KVM will use.
>> */
>> -#define PAGE_SIZE getpagesize()
>> +#define PAGE_SIZE qemu_real_host_page_size
>>
>> //#define DEBUG_KVM
>>
>> @@ -507,7 +507,7 @@ static int
>> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>> {
>> ram_addr_t start = section->offset_within_region +
>> memory_region_get_ram_addr(section->mr);
>> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
>> +ram_addr_t pages = int128_get64(section->size) /
>> qemu_real_host_page_size;
>>
>> cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
>> return 0;
>> @@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
>> * even with KVM. TARGET_PAGE_SIZE is assumed to be the minimum
>> * page size for the system though.
>> */
>> -assert(TARGET_PAGE_SIZE <= getpagesize());
>> +assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>>
>> s->sigmask_len = 8;
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6d333dc23c..e773bdfa6e 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend
>> *memdev)
>> #else
>> size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>> {
>> -return getpagesize();
>> +return qemu_real_host_page_size;
>> }
>> #endif
>>
>> diff --git a/block.c b/block.c
>> index 5944124845..98f47e2902 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpagesize());
>> +return MAX(4096, qemu_real_host_page_size);
>> }
>>
>> return bs->bl.opt_mem_alignment;
>> @@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpag
[PATCH 1/2] tests/tcg/multiarch: fix code style in function main of test-mmap.c
This file uses quite a different code style and changing just one line
would leads to some awkward appearance.
This is a preparation for the following replacement of
sysconf(_SC_PAGESIZE).
BTW, to depress ERROR message from checkpatch.pl, this patch replaces
strtoul with qemu_strtoul.
Signed-off-by: Wei Yang
---
tests/tcg/multiarch/test-mmap.c | 67 ++---
1 file changed, 36 insertions(+), 31 deletions(-)
diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 11d0e777b1..9ea49e2307 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -456,49 +456,54 @@ void check_invalid_mmaps(void)
int main(int argc, char **argv)
{
- char tempname[] = "/tmp/.cmmapXX";
- unsigned int i;
-
- /* Trust the first argument, otherwise probe the system for our
- pagesize. */
- if (argc > 1)
- pagesize = strtoul(argv[1], NULL, 0);
- else
- pagesize = sysconf(_SC_PAGESIZE);
+char tempname[] = "/tmp/.cmmapXX";
+unsigned int i;
+
+/*
+ * Trust the first argument, otherwise probe the system for our
+ * pagesize.
+ */
+if (argc > 1) {
+qemu_strtoul(argv[1], NULL, 0, );
+} else {
+pagesize = sysconf(_SC_PAGESIZE);
+}
- /* Assume pagesize is a power of two. */
- pagemask = pagesize - 1;
- dummybuf = malloc (pagesize);
- printf ("pagesize=%u pagemask=%x\n", pagesize, pagemask);
+/* Assume pagesize is a power of two. */
+pagemask = pagesize - 1;
+dummybuf = malloc(pagesize);
+printf("pagesize=%u pagemask=%x\n", pagesize, pagemask);
- test_fd = mkstemp(tempname);
- unlink(tempname);
+test_fd = mkstemp(tempname);
+unlink(tempname);
- /* Fill the file with int's counting from zero and up. */
+/* Fill the file with int's counting from zero and up. */
for (i = 0; i < (pagesize * 4) / sizeof i; i++) {
checked_write(test_fd, , sizeof i);
}
- /* Append a few extra writes to make the file end at non
- page boundary. */
+/*
+ * Append a few extra writes to make the file end at non
+ * page boundary.
+ */
checked_write(test_fd, , sizeof i); i++;
checked_write(test_fd, , sizeof i); i++;
checked_write(test_fd, , sizeof i); i++;
- test_fsize = lseek(test_fd, 0, SEEK_CUR);
+test_fsize = lseek(test_fd, 0, SEEK_CUR);
- /* Run the tests. */
- check_aligned_anonymous_unfixed_mmaps();
- check_aligned_anonymous_unfixed_colliding_mmaps();
- check_aligned_anonymous_fixed_mmaps();
- check_file_unfixed_mmaps();
- check_file_fixed_mmaps();
- check_file_fixed_eof_mmaps();
- check_file_unfixed_eof_mmaps();
- check_invalid_mmaps();
+/* Run the tests. */
+check_aligned_anonymous_unfixed_mmaps();
+check_aligned_anonymous_unfixed_colliding_mmaps();
+check_aligned_anonymous_fixed_mmaps();
+check_file_unfixed_mmaps();
+check_file_fixed_mmaps();
+check_file_fixed_eof_mmaps();
+check_file_unfixed_eof_mmaps();
+check_invalid_mmaps();
- /* Fails at the moment. */
- /* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
+/* Fails at the moment. */
+/* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
- return EXIT_SUCCESS;
+return EXIT_SUCCESS;
}
--
2.17.1
[PATCH 2/2] core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
Signed-off-by: Wei Yang
Suggested-by: "Dr. David Alan Gilbert"
CC: Richard Henderson
---
block/file-posix.c | 2 +-
net/l2tpv3.c| 2 +-
tests/tcg/multiarch/test-mmap.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 5d1995a07c..853ed42134 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2562,7 +2562,7 @@ static void check_cache_dropped(BlockDriverState *bs,
Error **errp)
off_t end;
/* mincore(2) page status information requires 1 byte per page */
-page_size = sysconf(_SC_PAGESIZE);
+page_size = qemu_real_host_page_size;
vec = g_malloc(DIV_ROUND_UP(window_size, page_size));
end = raw_getlength(bs);
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 55fea17c0f..5f843240de 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -41,7 +41,7 @@
* chosen to be sufficient to accommodate one packet with some headers
*/
-#define BUFFER_ALIGN sysconf(_SC_PAGESIZE)
+#define BUFFER_ALIGN qemu_real_host_page_size
#define BUFFER_SIZE 2048
#define IOVSIZE 2
#define MAX_L2TPV3_MSGCNT 64
diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 9ea49e2307..370842e5c2 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -466,7 +466,7 @@ int main(int argc, char **argv)
if (argc > 1) {
qemu_strtoul(argv[1], NULL, 0, );
} else {
-pagesize = sysconf(_SC_PAGESIZE);
+pagesize = qemu_real_host_page_size;
}
/* Assume pagesize is a power of two. */
--
2.17.1
[PATCH 0/2] replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size
This is a following up patch to cleanup page size, suggested by "Dr. David Alan Gilbert" . Patch 2 does the job, while during the cleanup I found test-mmap.c has quite a lot code style problem. To make the code looks good, patch 1 is introduced to make checkpatch.pl happy a little. Wei Yang (2): tests/tcg/multiarch: fix code style in function main of test-mmap.c core: replace sysconf(_SC_PAGESIZE) with qemu_real_host_page_size block/file-posix.c | 2 +- net/l2tpv3.c| 2 +- tests/tcg/multiarch/test-mmap.c | 67 ++--- 3 files changed, 38 insertions(+), 33 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr
On Mon, Oct 14, 2019 at 12:05:47PM -0300, Eduardo Habkost wrote: >On Sat, Oct 12, 2019 at 05:02:09PM +0800, Wei Yang wrote: >> On Sat, Sep 14, 2019 at 03:40:41PM -0400, Michael S. Tsirkin wrote: >> >On Fri, Sep 13, 2019 at 11:47:46PM +, Wei Yang wrote: >> >> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote: >> >> >When we iterate the memory-device list to get the available range, it is >> >> >not >> >> >necessary to iterate the whole list. >> >> > >> >> >1) no more overlap for hinted range if tmp exceed it >> >> > >> >> >v2: >> >> > * remove #2 as suggested by Igor and David >> >> > * add some comment to inform address assignment stay the same as >> >> > before >> >> > this change >> >> > >> >> >Wei Yang (2): >> >> > memory-device: not necessary to use goto for the last check >> >> > memory-device: break the loop if tmp exceed the hinted range >> >> > >> >> > hw/mem/memory-device.c | 3 ++- >> >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> >> > >> >> >> >> Would someone take this patch set? >> > >> >yes looks good to me too. >> >Eduardo? >> > >> >> Hmm... I don't see this any where. May I ask the status? > >Sorry, I hadn't seen Michael's message. Queued on machine-next. >Thanks! > Thanks~ have a nice day. >-- >Eduardo -- Wei Yang Help you, Help me
Re: [PATCH 0/2] cleanup on page size
Hi, All, I got one page size related question, hope to get some hint. There is one comment in page_size_init(). /* NOTE: we can always suppose that qemu_host_page_size >= TARGET_PAGE_SIZE */ The final result is true, since we compare qemu_host_page_size with TARGET_PAGE_SIZE and if not qemu_host_page_size will be assigned to TARGET_PAGE_SIZE. Generally, there is no problem, but one corner case for migration. In function ram_save_host_page(), it tries to save a whole host page. Or to be specific, it tries to save a whole REAL host page. The potential problem is when qemu_real_host_page_size < TARGET_PAGE_SIZE, this whole host page would be a wrong range. So I am wondering why we have the assumption written in page_size_init()? I tried to dig out who and why we have this comment, but found the first commit is commit 54936004fddc52c321cb3f9a9a51140e782bed5d Author: bellard Date: Tue May 13 00:25:15 2003 + mmap emulation git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@158 c046a42c-6fe2-441c-8c8c-71466251a162 There is no reason logged. On Sun, Oct 13, 2019 at 10:11:43AM +0800, Wei Yang wrote: >Patch 1 simplify the definition of xxx_PAGE_ALIGN. >Patch 2 replaces getpagesize() with qemu_real_host_page_size. This one touch a >volume of code. If some point is not correct, I'd appreciate your >notification. > >Wei Yang (2): > cpu: use ROUND_UP() to define xxx_PAGE_ALIGN > core: replace getpagesize() with qemu_real_host_page_size > > accel/kvm/kvm-all.c| 6 +++--- > backends/hostmem.c | 2 +- > block.c| 4 ++-- > block/file-posix.c | 9 + > block/io.c | 2 +- > block/parallels.c | 2 +- > block/qcow2-cache.c| 2 +- > contrib/vhost-user-gpu/vugbm.c | 2 +- > exec.c | 6 +++--- > hw/intc/s390_flic_kvm.c| 2 +- > hw/ppc/mac_newworld.c | 2 +- > hw/ppc/spapr_pci.c | 2 +- > hw/rdma/vmw/pvrdma_main.c | 2 +- > hw/vfio/spapr.c| 7 --- > include/exec/cpu-all.h | 7 +++ > include/exec/ram_addr.h| 2 +- > include/qemu/osdep.h | 4 ++-- > migration/migration.c | 2 +- > migration/postcopy-ram.c | 4 ++-- > monitor/misc.c | 2 +- > target/ppc/kvm.c | 2 +- > tests/vhost-user-bridge.c | 8 > util/mmap-alloc.c | 10 +- > util/oslib-posix.c | 4 ++-- > util/oslib-win32.c | 2 +- > util/vfio-helpers.c | 12 ++-- > 26 files changed, 55 insertions(+), 54 deletions(-) > >-- >2.17.1 -- Wei Yang Help you, Help me
Re: [PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
On Mon, Oct 14, 2019 at 10:15:02AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> There are three page size in qemu:
>>
>> real host page size
>> host page size
>> target page size
>>
>> All of them have dedicate variable to represent. For the last two, we
>> use the same form in the whole qemu project, while for the first one we
>> use two forms: qemu_real_host_page_size and getpagesize().
>>
>> qemu_real_host_page_size is defined to be a replacement of
>> getpagesize(), so let it serve the role.
>
>We also use sysconf(_SC_PAGESIZE) in a few places.
You mean need to replace them too?
>
>Dave
>
>>
>> [Note] Not fully tested for some arch or device.
>>
>> Signed-off-by: Wei Yang
>> ---
>> accel/kvm/kvm-all.c| 6 +++---
>> backends/hostmem.c | 2 +-
>> block.c| 4 ++--
>> block/file-posix.c | 9 +
>> block/io.c | 2 +-
>> block/parallels.c | 2 +-
>> block/qcow2-cache.c| 2 +-
>> contrib/vhost-user-gpu/vugbm.c | 2 +-
>> exec.c | 6 +++---
>> hw/intc/s390_flic_kvm.c| 2 +-
>> hw/ppc/mac_newworld.c | 2 +-
>> hw/ppc/spapr_pci.c | 2 +-
>> hw/rdma/vmw/pvrdma_main.c | 2 +-
>> hw/vfio/spapr.c| 7 ---
>> include/exec/ram_addr.h| 2 +-
>> include/qemu/osdep.h | 4 ++--
>> migration/migration.c | 2 +-
>> migration/postcopy-ram.c | 4 ++--
>> monitor/misc.c | 2 +-
>> target/ppc/kvm.c | 2 +-
>> tests/vhost-user-bridge.c | 8
>> util/mmap-alloc.c | 10 +-
>> util/oslib-posix.c | 4 ++--
>> util/oslib-win32.c | 2 +-
>> util/vfio-helpers.c| 12 ++--
>> 25 files changed, 52 insertions(+), 50 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index d2d96d73e8..140b0bd8f6 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -52,7 +52,7 @@
>> /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>> * need to use the real host PAGE_SIZE, as that's what KVM will use.
>> */
>> -#define PAGE_SIZE getpagesize()
>> +#define PAGE_SIZE qemu_real_host_page_size
>>
>> //#define DEBUG_KVM
>>
>> @@ -507,7 +507,7 @@ static int
>> kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
>> {
>> ram_addr_t start = section->offset_within_region +
>> memory_region_get_ram_addr(section->mr);
>> -ram_addr_t pages = int128_get64(section->size) / getpagesize();
>> +ram_addr_t pages = int128_get64(section->size) /
>> qemu_real_host_page_size;
>>
>> cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
>> return 0;
>> @@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
>> * even with KVM. TARGET_PAGE_SIZE is assumed to be the minimum
>> * page size for the system though.
>> */
>> -assert(TARGET_PAGE_SIZE <= getpagesize());
>> +assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
>>
>> s->sigmask_len = 8;
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6d333dc23c..e773bdfa6e 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend
>> *memdev)
>> #else
>> size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
>> {
>> -return getpagesize();
>> +return qemu_real_host_page_size;
>> }
>> #endif
>>
>> diff --git a/block.c b/block.c
>> index 5944124845..98f47e2902 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return MAX(4096, getpagesize());
>> +return MAX(4096, qemu_real_host_page_size);
>> }
>>
>> return bs->bl.opt_mem_alignment;
>> @@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
>> {
>> if (!bs || !bs->drv) {
>> /* page size or 4k (hdd sector size) should be on the safe side */
>> -return
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 07:38:04PM -0700, Richard Henderson wrote: >On 10/13/19 6:01 PM, Wei Yang wrote: >>> No, please. >>> >>> (1) The compiler does not know that qemu_*host_page_size is a power of 2, >>> and >>> will generate a real division at runtime. The same is true for >>> TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. >>> >> >> Confused >> >> The definition of ROUND_UP is: >> >> #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) > >Ah, my bad, I did confuse this with QEMU_ALIGN_UP. > >Hmm. > > lea -1(n, size), t > neg size > and size, t > >vs > > mov mask, t > not t > add n, t > and mask, t > >which is what I proposed here > >>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > >I'm ok with your version. > >Reviewed-by: Richard Henderson > Thanks for your clarification. Have a nice day > >r~ > -- Wei Yang Help you, Help me
Re: [PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
On Sun, Oct 13, 2019 at 11:56:35AM -0400, Richard Henderson wrote: >On 10/12/19 10:11 PM, Wei Yang wrote: >> Use ROUND_UP() to define, which is a little bit easy to read. >> >> Signed-off-by: Wei Yang >> --- >> include/exec/cpu-all.h | 7 +++ >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h >> index ad9ab85eb3..255bb186ac 100644 >> --- a/include/exec/cpu-all.h >> +++ b/include/exec/cpu-all.h >> @@ -220,7 +220,7 @@ extern int target_page_bits; >> >> #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) >> #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) >> -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & >> TARGET_PAGE_MASK) >> +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) >> >> /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even >> * when intptr_t is 32-bit and we are aligning a long long. >> @@ -228,9 +228,8 @@ extern int target_page_bits; >> extern uintptr_t qemu_host_page_size; >> extern intptr_t qemu_host_page_mask; >> >> -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & >> qemu_host_page_mask) >> -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) >> & \ >> -qemu_real_host_page_mask) >> +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) >> +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), >> qemu_real_host_page_size) > > >No, please. > >(1) The compiler does not know that qemu_*host_page_size is a power of 2, and >will generate a real division at runtime. The same is true for >TARGET_PAGE_SIZE when TARGET_PAGE_BITS_VARY. > Confused The definition of ROUND_UP is: #define ROUND_UP(n, d) (((n) + (d) - 1) & -(0 ? (n) : (d))) Why it will do division? This will be expanded to the same form as the original code, if my understanding is correct. Would you mind telling me more? >(2) The first hunk conflicts with an in-flight patch of mine: > >https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04526.html > > >r~ -- Wei Yang Help you, Help me
[PATCH 1/2] cpu: use ROUND_UP() to define xxx_PAGE_ALIGN
Use ROUND_UP() to define, which is a little bit easy to read. Signed-off-by: Wei Yang --- include/exec/cpu-all.h | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h index ad9ab85eb3..255bb186ac 100644 --- a/include/exec/cpu-all.h +++ b/include/exec/cpu-all.h @@ -220,7 +220,7 @@ extern int target_page_bits; #define TARGET_PAGE_SIZE (1 << TARGET_PAGE_BITS) #define TARGET_PAGE_MASK ~(TARGET_PAGE_SIZE - 1) -#define TARGET_PAGE_ALIGN(addr) (((addr) + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK) +#define TARGET_PAGE_ALIGN(addr) ROUND_UP((addr), TARGET_PAGE_SIZE) /* Using intptr_t ensures that qemu_*_page_mask is sign-extended even * when intptr_t is 32-bit and we are aligning a long long. @@ -228,9 +228,8 @@ extern int target_page_bits; extern uintptr_t qemu_host_page_size; extern intptr_t qemu_host_page_mask; -#define HOST_PAGE_ALIGN(addr) (((addr) + qemu_host_page_size - 1) & qemu_host_page_mask) -#define REAL_HOST_PAGE_ALIGN(addr) (((addr) + qemu_real_host_page_size - 1) & \ -qemu_real_host_page_mask) +#define HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_host_page_size) +#define REAL_HOST_PAGE_ALIGN(addr) ROUND_UP((addr), qemu_real_host_page_size) /* same as PROT_xxx */ #define PAGE_READ 0x0001 -- 2.17.1
[PATCH 2/2] core: replace getpagesize() with qemu_real_host_page_size
There are three page size in qemu:
real host page size
host page size
target page size
All of them have dedicate variable to represent. For the last two, we
use the same form in the whole qemu project, while for the first one we
use two forms: qemu_real_host_page_size and getpagesize().
qemu_real_host_page_size is defined to be a replacement of
getpagesize(), so let it serve the role.
[Note] Not fully tested for some arch or device.
Signed-off-by: Wei Yang
---
accel/kvm/kvm-all.c| 6 +++---
backends/hostmem.c | 2 +-
block.c| 4 ++--
block/file-posix.c | 9 +
block/io.c | 2 +-
block/parallels.c | 2 +-
block/qcow2-cache.c| 2 +-
contrib/vhost-user-gpu/vugbm.c | 2 +-
exec.c | 6 +++---
hw/intc/s390_flic_kvm.c| 2 +-
hw/ppc/mac_newworld.c | 2 +-
hw/ppc/spapr_pci.c | 2 +-
hw/rdma/vmw/pvrdma_main.c | 2 +-
hw/vfio/spapr.c| 7 ---
include/exec/ram_addr.h| 2 +-
include/qemu/osdep.h | 4 ++--
migration/migration.c | 2 +-
migration/postcopy-ram.c | 4 ++--
monitor/misc.c | 2 +-
target/ppc/kvm.c | 2 +-
tests/vhost-user-bridge.c | 8
util/mmap-alloc.c | 10 +-
util/oslib-posix.c | 4 ++--
util/oslib-win32.c | 2 +-
util/vfio-helpers.c| 12 ++--
25 files changed, 52 insertions(+), 50 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d2d96d73e8..140b0bd8f6 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -52,7 +52,7 @@
/* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
* need to use the real host PAGE_SIZE, as that's what KVM will use.
*/
-#define PAGE_SIZE getpagesize()
+#define PAGE_SIZE qemu_real_host_page_size
//#define DEBUG_KVM
@@ -507,7 +507,7 @@ static int
kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
{
ram_addr_t start = section->offset_within_region +
memory_region_get_ram_addr(section->mr);
-ram_addr_t pages = int128_get64(section->size) / getpagesize();
+ram_addr_t pages = int128_get64(section->size) / qemu_real_host_page_size;
cpu_physical_memory_set_dirty_lebitmap(bitmap, start, pages);
return 0;
@@ -1841,7 +1841,7 @@ static int kvm_init(MachineState *ms)
* even with KVM. TARGET_PAGE_SIZE is assumed to be the minimum
* page size for the system though.
*/
-assert(TARGET_PAGE_SIZE <= getpagesize());
+assert(TARGET_PAGE_SIZE <= qemu_real_host_page_size);
s->sigmask_len = 8;
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 6d333dc23c..e773bdfa6e 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -304,7 +304,7 @@ size_t host_memory_backend_pagesize(HostMemoryBackend
*memdev)
#else
size_t host_memory_backend_pagesize(HostMemoryBackend *memdev)
{
-return getpagesize();
+return qemu_real_host_page_size;
}
#endif
diff --git a/block.c b/block.c
index 5944124845..98f47e2902 100644
--- a/block.c
+++ b/block.c
@@ -106,7 +106,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
{
if (!bs || !bs->drv) {
/* page size or 4k (hdd sector size) should be on the safe side */
-return MAX(4096, getpagesize());
+return MAX(4096, qemu_real_host_page_size);
}
return bs->bl.opt_mem_alignment;
@@ -116,7 +116,7 @@ size_t bdrv_min_mem_align(BlockDriverState *bs)
{
if (!bs || !bs->drv) {
/* page size or 4k (hdd sector size) should be on the safe side */
-return MAX(4096, getpagesize());
+return MAX(4096, qemu_real_host_page_size);
}
return bs->bl.min_mem_alignment;
diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2d..f60ac3f93f 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -322,7 +322,7 @@ static void raw_probe_alignment(BlockDriverState *bs, int
fd, Error **errp)
{
BDRVRawState *s = bs->opaque;
char *buf;
-size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
+size_t max_align = MAX(MAX_BLOCKSIZE, qemu_real_host_page_size);
size_t alignments[] = {1, 512, 1024, 2048, 4096};
/* For SCSI generic devices the alignment is not really used.
@@ -1131,13 +1131,14 @@ static void raw_refresh_limits(BlockDriverState *bs,
Error **errp)
ret = sg_get_max_segments(s->fd);
if (ret > 0) {
-bs->bl.max_transfer = MIN(bs->bl.max_transfer, ret *
getpagesize());
+bs->bl.max_transfer = MIN(bs->bl.max_transfer,
+ ret * qemu_real_host_page_size);
}
}
raw_probe_alignment(bs, s->fd, errp);
bs->bl.min_mem_alignment = s->buf_align;
-bs->bl.opt_mem_alignment = MAX(s
[PATCH 0/2] cleanup on page size
Patch 1 simplify the definition of xxx_PAGE_ALIGN. Patch 2 replaces getpagesize() with qemu_real_host_page_size. This one touch a volume of code. If some point is not correct, I'd appreciate your notification. Wei Yang (2): cpu: use ROUND_UP() to define xxx_PAGE_ALIGN core: replace getpagesize() with qemu_real_host_page_size accel/kvm/kvm-all.c| 6 +++--- backends/hostmem.c | 2 +- block.c| 4 ++-- block/file-posix.c | 9 + block/io.c | 2 +- block/parallels.c | 2 +- block/qcow2-cache.c| 2 +- contrib/vhost-user-gpu/vugbm.c | 2 +- exec.c | 6 +++--- hw/intc/s390_flic_kvm.c| 2 +- hw/ppc/mac_newworld.c | 2 +- hw/ppc/spapr_pci.c | 2 +- hw/rdma/vmw/pvrdma_main.c | 2 +- hw/vfio/spapr.c| 7 --- include/exec/cpu-all.h | 7 +++ include/exec/ram_addr.h| 2 +- include/qemu/osdep.h | 4 ++-- migration/migration.c | 2 +- migration/postcopy-ram.c | 4 ++-- monitor/misc.c | 2 +- target/ppc/kvm.c | 2 +- tests/vhost-user-bridge.c | 8 util/mmap-alloc.c | 10 +- util/oslib-posix.c | 4 ++-- util/oslib-win32.c | 2 +- util/vfio-helpers.c| 12 ++-- 26 files changed, 55 insertions(+), 54 deletions(-) -- 2.17.1
Re: [Qemu-devel] [PATCH v2 0/2] refine memory_device_get_free_addr
On Sat, Sep 14, 2019 at 03:40:41PM -0400, Michael S. Tsirkin wrote: >On Fri, Sep 13, 2019 at 11:47:46PM +0000, Wei Yang wrote: >> On Tue, Jul 30, 2019 at 08:37:38AM +0800, Wei Yang wrote: >> >When we iterate the memory-device list to get the available range, it is not >> >necessary to iterate the whole list. >> > >> >1) no more overlap for hinted range if tmp exceed it >> > >> >v2: >> > * remove #2 as suggested by Igor and David >> > * add some comment to inform address assignment stay the same as before >> > this change >> > >> >Wei Yang (2): >> > memory-device: not necessary to use goto for the last check >> > memory-device: break the loop if tmp exceed the hinted range >> > >> > hw/mem/memory-device.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> >> Would someone take this patch set? > >yes looks good to me too. >Eduardo? > Hmm... I don't see this any where. May I ask the status? >> >-- >> >2.17.1 >> > >> >> -- >> Wei Yang >> Help you, Help me -- Wei Yang Help you, Help me
[PATCH 2/2] migration/compress: disable compress if failed to setup
In current logic, if compress_threads_save_setup() returns -1 the whole
migration would fail, while we could handle it gracefully by disable
compress.
Signed-off-by: Wei Yang
---
migration/migration.c | 9 +
migration/migration.h | 1 +
migration/ram.c | 15 ---
3 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 5f7e4d15e9..02b95f4223 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2093,6 +2093,15 @@ bool migrate_use_compression(void)
return s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS];
}
+void migrate_disable_compression(void)
+{
+MigrationState *s;
+
+s = migrate_get_current();
+
+s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS] = false;
+}
+
int migrate_compress_level(void)
{
MigrationState *s;
diff --git a/migration/migration.h b/migration/migration.h
index 4f2fe193dc..51368d3a6e 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -309,6 +309,7 @@ bool migrate_use_return_path(void);
uint64_t ram_get_total_transferred_pages(void);
bool migrate_use_compression(void);
+void migrate_disable_compression(void);
int migrate_compress_level(void);
int migrate_compress_threads(void);
int migrate_compress_wait_thread(void);
diff --git a/migration/ram.c b/migration/ram.c
index 96c9b16402..39279161a8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -533,12 +533,12 @@ static void compress_threads_save_cleanup(void)
comp_param = NULL;
}
-static int compress_threads_save_setup(void)
+static void compress_threads_save_setup(void)
{
int i, thread_count;
if (!migrate_use_compression()) {
-return 0;
+return;
}
thread_count = migrate_compress_threads();
compress_threads = g_new0(QemuThread, thread_count);
@@ -569,11 +569,14 @@ static int compress_threads_save_setup(void)
do_data_compress, comp_param + i,
QEMU_THREAD_JOINABLE);
}
-return 0;
+return;
exit:
compress_threads_save_cleanup();
-return -1;
+migrate_disable_compression();
+error_report("%s: failed to setup compress threads, compress disabled",
+ __func__);
+return;
}
/* Multiple fd's */
@@ -3338,9 +3341,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
RAMState **rsp = opaque;
RAMBlock *block;
-if (compress_threads_save_setup()) {
-return -1;
-}
+compress_threads_save_setup();
/* migration has already setup the bitmap, reuse it. */
if (!migration_in_colo_state()) {
--
2.17.1
[PATCH 0/2] migration/compress: refine the compress case
Two patches related to compress: 1. simplify the check since compress QEMUFile is not writable 2. handle compress setup failure gracefully Wei Yang (2): migration/compress: compress QEMUFile is not writable migration/compress: disable compress if failed to setup migration/migration.c | 9 + migration/migration.h | 1 + migration/qemu-file.c | 16 +++- migration/ram.c | 15 --- 4 files changed, 21 insertions(+), 20 deletions(-) -- 2.17.1
[PATCH 1/2] migration/compress: compress QEMUFile is not writable
We open a file with empty_ops for compress QEMUFile, which means this is
not writable.
Signed-off-by: Wei Yang
---
migration/qemu-file.c | 16 +++-
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 26fb25ddc1..f3d99a96ec 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -744,11 +744,8 @@ static int qemu_compress_data(z_stream *stream, uint8_t
*dest, size_t dest_len,
/* Compress size bytes of data start at p and store the compressed
* data to the buffer of f.
*
- * When f is not writable, return -1 if f has no space to save the
- * compressed data.
- * When f is wirtable and it has no space to save the compressed data,
- * do fflush first, if f still has no space to save the compressed
- * data, return -1.
+ * Since the file is dummy file with empty_ops, return -1 if f has no space to
+ * save the compressed data.
*/
ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
const uint8_t *p, size_t size)
@@ -756,14 +753,7 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream
*stream,
ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
if (blen < compressBound(size)) {
-if (!qemu_file_is_writable(f)) {
-return -1;
-}
-qemu_fflush(f);
-blen = IO_BUF_SIZE - sizeof(int32_t);
-if (blen < compressBound(size)) {
-return -1;
-}
+return -1;
}
blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
--
2.17.1
Re: [PATCH 3/4] migration/multifd: initialize packet->magic/version once at setup stage
On Fri, Oct 11, 2019 at 12:20:48PM +0200, Juan Quintela wrote: >Wei Yang wrote: >> MultiFDPacket_t's magic and version field never changes during >> migration, so move these two fields in setup stage. >> >> Signed-off-by: Wei Yang > >Reviewed-by: Juan Quintela > >It don't really matter, and is faster your way O:-) You are right. And I am wondering one more thing. Why we need to carry magic/version for each packet? Would it be better to just carry and check magic/version for the initial packet only? -- Wei Yang Help you, Help me
Re: [PULL 5/5] multifd: Use number of channels as listen backlog
On Fri, Oct 11, 2019 at 12:40:03PM +0200, Juan Quintela wrote:
>Wei Yang wrote:
>> On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>>>Reviewed-by: Daniel P. Berrang??
>>>Signed-off-by: Juan Quintela
>>>---
>>> migration/socket.c | 7 ++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/migration/socket.c b/migration/socket.c
>>>index e63f5e1612..97c9efde59 100644
>>>--- a/migration/socket.c
>>>+++ b/migration/socket.c
>>>@@ -178,10 +178,15 @@ static void
>>>socket_start_incoming_migration(SocketAddress *saddr,
>>> {
>>> QIONetListener *listener = qio_net_listener_new();
>>> size_t i;
>>>+int num = 1;
>>>
>>> qio_net_listener_set_name(listener, "migration-socket-listener");
>>>
>>>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>>>+if (migrate_use_multifd()) {
>>>+num = migrate_multifd_channels();
>>>+}
>>>+
>>>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
>>> object_unref(OBJECT(listener));
>>> return;
>>> }
>>
>> My confusion is this function is called at the beginning of the program,
>> which
>> means we didn't set multifd on or change the multifd channel parameter.
>>
>> They are the default value at this point.
>>
>> Am I right?
>
>Hi
>
>good catch!
>
>You are right. The fix worked for me because I always use on the
>command line:
>
>--global migration.multifd-channels=10
>
>or whatever number I want to avoid typing. I can only see two
>solutions:
>- increase the number always
You mean change default value? Then which one should we choose?
>- require "defer" when using multifd to be able to setup parameters.
>
>Any other good ideas?
Would you mind explaining more about "defer"? How this works?
>
>Thanks, Juan.
>
>PD. I was having problem reproducing this issue because I use the
>command line for the parameter.
--
Wei Yang
Help you, Help me
[PATCH 2/4] migration/multifd: use pages->allocated instead of the static max
multifd_send_fill_packet() prepares meta data for following pages to
transfer. It would be more proper to fill pages->allocated instead of
static max value, especially we want to support flexible packet size.
Signed-off-by: Wei Yang
---
migration/ram.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index cf30171f44..6a3bef0434 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -791,13 +791,12 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
static void multifd_send_fill_packet(MultiFDSendParams *p)
{
MultiFDPacket_t *packet = p->packet;
-uint32_t page_max = MULTIFD_PACKET_SIZE / qemu_target_page_size();
int i;
packet->magic = cpu_to_be32(MULTIFD_MAGIC);
packet->version = cpu_to_be32(MULTIFD_VERSION);
packet->flags = cpu_to_be32(p->flags);
-packet->pages_alloc = cpu_to_be32(page_max);
+packet->pages_alloc = cpu_to_be32(p->pages->allocated);
packet->pages_used = cpu_to_be32(p->pages->used);
packet->next_packet_size = cpu_to_be32(p->next_packet_size);
packet->packet_num = cpu_to_be64(p->packet_num);
--
2.17.1
[PATCH 0/4] migration/multifd: trivial cleanup for multifd
Here are four trivial cleanups related to multifd. Fix a typo, use a proper variable and setup never changed variables only once. Wei Yang (4): migration/multifd: fix a typo in comment of multifd_recv_unfill_packet() migration/multifd: use pages->allocated instead of the static max migration/multifd: initialize packet->magic/version once at setup stage migration/multifd: pages->used would be cleared when attach to multifd_send_state migration/ram.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) -- 2.17.1
[PATCH 4/4] migration/multifd: pages->used would be cleared when attach to multifd_send_state
When we found an available channel in multifd_send_pages(), its pages->used is cleared and then attached to multifd_send_state. It is not necessary to do this twice. Signed-off-by: Wei Yang --- migration/ram.c | 1 - 1 file changed, 1 deletion(-) diff --git a/migration/ram.c b/migration/ram.c index 71d845b851..051de77d5a 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1129,7 +1129,6 @@ static void *multifd_send_thread(void *opaque) p->flags = 0; p->num_packets++; p->num_pages += used; -p->pages->used = 0; qemu_mutex_unlock(>mutex); trace_multifd_send(p->id, packet_num, used, flags, -- 2.17.1
[PATCH 1/4] migration/multifd: fix a typo in comment of multifd_recv_unfill_packet()
Signed-off-by: Wei Yang
---
migration/ram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/migration/ram.c b/migration/ram.c
index 22423f08cd..cf30171f44 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -838,7 +838,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p,
Error **errp)
packet->pages_alloc = be32_to_cpu(packet->pages_alloc);
/*
- * If we recevied a packet that is 100 times bigger than expected
+ * If we received a packet that is 100 times bigger than expected
* just stop migration. It is a magic number.
*/
if (packet->pages_alloc > pages_max * 100) {
--
2.17.1
[PATCH 3/4] migration/multifd: initialize packet->magic/version once at setup stage
MultiFDPacket_t's magic and version field never changes during
migration, so move these two fields in setup stage.
Signed-off-by: Wei Yang
---
migration/ram.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 6a3bef0434..71d845b851 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -793,8 +793,6 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
MultiFDPacket_t *packet = p->packet;
int i;
-packet->magic = cpu_to_be32(MULTIFD_MAGIC);
-packet->version = cpu_to_be32(MULTIFD_VERSION);
packet->flags = cpu_to_be32(p->flags);
packet->pages_alloc = cpu_to_be32(p->pages->allocated);
packet->pages_used = cpu_to_be32(p->pages->used);
@@ -1240,6 +1238,8 @@ int multifd_save_setup(void)
p->packet_len = sizeof(MultiFDPacket_t)
+ sizeof(ram_addr_t) * page_count;
p->packet = g_malloc0(p->packet_len);
+p->packet->magic = cpu_to_be32(MULTIFD_MAGIC);
+p->packet->version = cpu_to_be32(MULTIFD_VERSION);
p->name = g_strdup_printf("multifdsend_%d", i);
socket_send_channel_create(multifd_new_send_channel_async, p);
}
--
2.17.1
Re: [Qemu-devel] [PULL 5/5] multifd: Use number of channels as listen backlog
On Wed, Sep 04, 2019 at 08:29:15AM +0200, Juan Quintela wrote:
>Reviewed-by: Daniel P. Berrangé
>Signed-off-by: Juan Quintela
>---
> migration/socket.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
>diff --git a/migration/socket.c b/migration/socket.c
>index e63f5e1612..97c9efde59 100644
>--- a/migration/socket.c
>+++ b/migration/socket.c
>@@ -178,10 +178,15 @@ static void
>socket_start_incoming_migration(SocketAddress *saddr,
> {
> QIONetListener *listener = qio_net_listener_new();
> size_t i;
>+int num = 1;
>
> qio_net_listener_set_name(listener, "migration-socket-listener");
>
>-if (qio_net_listener_open_sync(listener, saddr, 1, errp) < 0) {
>+if (migrate_use_multifd()) {
>+num = migrate_multifd_channels();
>+}
>+
>+if (qio_net_listener_open_sync(listener, saddr, num, errp) < 0) {
> object_unref(OBJECT(listener));
> return;
> }
My confusion is this function is called at the beginning of the program, which
means we didn't set multifd on or change the multifd channel parameter.
They are the default value at this point.
Am I right?
>--
>2.21.0
>
--
Wei Yang
Help you, Help me
Re: [PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check
On Wed, Oct 09, 2019 at 11:17:16AM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> On Tue, Oct 08, 2019 at 08:15:51PM +0100, Dr. David Alan Gilbert wrote:
>> >* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> >> After previous cleanup, postcopy thread is running only when
>> >> PostcopyState is LISTENNING or RUNNING. This means it is not necessary
>> >> to spare a variable have_listen_thread to represent the state.
>> >>
>> >> Replace the check on have_listen_thread with PostcopyState and remove
>> >> the variable.
>> >>
>> >> Signed-off-by: Wei Yang
>> >> ---
>> >> migration/migration.h | 1 -
>> >> migration/ram.c | 2 +-
>> >> migration/ram.h | 1 +
>> >> migration/savevm.c| 4 +---
>> >> 4 files changed, 3 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/migration/migration.h b/migration/migration.h
>> >> index 4f2fe193dc..a4d639663d 100644
>> >> --- a/migration/migration.h
>> >> +++ b/migration/migration.h
>> >> @@ -63,7 +63,6 @@ struct MigrationIncomingState {
>> >> /* Set this when we want the fault thread to quit */
>> >> bool fault_thread_quit;
>> >>
>> >> -bool have_listen_thread;
>> >> QemuThread listen_thread;
>> >> QemuSemaphore listen_thread_sem;
>> >>
>> >> diff --git a/migration/ram.c b/migration/ram.c
>> >> index 769d3f6454..dfc50d57d5 100644
>> >> --- a/migration/ram.c
>> >> +++ b/migration/ram.c
>> >> @@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
>> >> return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
>> >> }
>> >>
>> >> -static bool postcopy_is_running(void)
>> >> +bool postcopy_is_running(void)
>> >> {
>> >> PostcopyState ps = postcopy_state_get();
>> >> return ps >= POSTCOPY_INCOMING_LISTENING && ps <
>> >> POSTCOPY_INCOMING_END;
>> >> diff --git a/migration/ram.h b/migration/ram.h
>> >> index bd0eee79b6..44fe4753ad 100644
>> >> --- a/migration/ram.h
>> >> +++ b/migration/ram.h
>> >> @@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState
>> >> *ms);
>> >> /* For incoming postcopy discard */
>> >> int ram_discard_range(const char *block_name, uint64_t start, size_t
>> >> length);
>> >> int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>> >> +bool postcopy_is_running(void);
>> >>
>> >> void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>> >>
>> >> diff --git a/migration/savevm.c b/migration/savevm.c
>> >> index dcad8897a3..2a0e0b94df 100644
>> >> --- a/migration/savevm.c
>> >> +++ b/migration/savevm.c
>> >> @@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void
>> >> *opaque)
>> >> qemu_loadvm_state_cleanup();
>> >>
>> >> rcu_unregister_thread();
>> >> -mis->have_listen_thread = false;
>> >> postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
>> >
>> >That now needs a big comment saying it must be the last thing in the
>> >thread, because now it's got meaning that it's here.
>> >
>> >>
>> >> return NULL;
>> >> @@ -1880,7 +1879,6 @@ static int
>> >> loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>> >> return -1;
>> >> }
>> >>
>> >> -mis->have_listen_thread = true;
>> >> /* Start up the listening thread and wait for it to signal ready */
>> >> qemu_sem_init(>listen_thread_sem, 0);
>> >> qemu_thread_create(>listen_thread, "postcopy/listen",
>> >> @@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
>> >>
>> >> trace_qemu_loadvm_state_post_main(ret);
>> >>
>> >> -if (mis->have_listen_thread) {
>> >> +if (postcopy_is_running()) {
>> >> /* Listen thread still going, can't clean up yet */
>> >> return ret;
>> >> }
>> >
>> >Can you explain to me why this is afe in the case of a failure in
>> >loadvm_postcopy_handle_listen between the start where it sets
>> >the state to LISTENING, and the point where it currently sets
>> >hasve_listen_thread ? Wouldn't this cause qemu_loadvm_state
>> >not to cleanup?
>> >
>>
>> I have to say you are right. listen_thread may not started when
>> PostcopyState
>> is already set to LISTENING.
>>
>> The ugly fix may be set PostcopyState back to original one. Not sure whether
>> you would like this.
>
>I think the 'have_listen_thread' might be the simplest solution though;
>it's very simple!
>
Yep, you are right.
>Dave
>
>> >Dave
>> >
>> >> --
>> >> 2.17.1
>> >>
>> >--
>> >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>>
>> --
>> Wei Yang
>> Help you, Help me
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
[PATCH v2 1/2] migration/postcopy: rename postcopy_ram_enable_notify to postcopy_ram_incoming_setup
Function postcopy_ram_incoming_setup and postcopy_ram_incoming_cleanup
is a pair. Rename to make it clear for audience.
Signed-off-by: Wei Yang
Reviewed-by: Dr. David Alan Gilbert
---
migration/postcopy-ram.c | 4 ++--
migration/postcopy-ram.h | 2 +-
migration/savevm.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 1f63e65ed7..b24c4a10c2 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1094,7 +1094,7 @@ retry:
return NULL;
}
-int postcopy_ram_enable_notify(MigrationIncomingState *mis)
+int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
{
/* Open the fd for the kernel to give us userfaults */
mis->userfault_fd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
@@ -1321,7 +1321,7 @@ int postcopy_request_shared_page(struct PostCopyFD *pcfd,
RAMBlock *rb,
return -1;
}
-int postcopy_ram_enable_notify(MigrationIncomingState *mis)
+int postcopy_ram_incoming_setup(MigrationIncomingState *mis)
{
assert(0);
return -1;
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index 9c8bd2bae0..d2668cc820 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -20,7 +20,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState
*mis);
* Make all of RAM sensitive to accesses to areas that haven't yet been written
* and wire up anything necessary to deal with it.
*/
-int postcopy_ram_enable_notify(MigrationIncomingState *mis);
+int postcopy_ram_incoming_setup(MigrationIncomingState *mis);
/*
* Initialise postcopy-ram, setting the RAM to a state where we can go into
diff --git a/migration/savevm.c b/migration/savevm.c
index bb9462a54d..78c2965ca4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1865,7 +1865,7 @@ static int
loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
* shouldn't be doing anything yet so don't actually expect requests
*/
if (migrate_postcopy_ram()) {
-if (postcopy_ram_enable_notify(mis)) {
+if (postcopy_ram_incoming_setup(mis)) {
postcopy_ram_incoming_cleanup(mis);
return -1;
}
--
2.17.1
[PATCH v2 2/2] migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING
Currently, we set PostcopyState blindly to RUNNING, even we found the
previous state is not LISTENING. This will lead to a corner case.
First let's look at the code flow:
qemu_loadvm_state_main()
ret = loadvm_process_command()
loadvm_postcopy_handle_run()
return -1;
if (ret < 0) {
if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING)
...
}
>From above snippet, the corner case is loadvm_postcopy_handle_run()
always sets state to RUNNING. And then it checks the previous state. If
the previous state is not LISTENING, it will return -1. But at this
moment, PostcopyState is already been set to RUNNING.
Then ret is checked in qemu_loadvm_state_main(), when it is -1
PostcopyState is checked. Current logic would pause postcopy and retry
if PostcopyState is RUNNING. This is not what we expect, because
postcopy is not active yet.
This patch makes sure state is set to RUNNING only previous state is
LISTENING by checking the state first.
Signed-off-by: Wei Yang
Suggested by: Peter Xu
---
migration/savevm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 78c2965ca4..b9f30a7090 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1934,7 +1934,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
/* After all discards we can start running and asking for pages */
static int loadvm_postcopy_handle_run(MigrationIncomingState *mis)
{
-PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
+PostcopyState ps = postcopy_state_get();
trace_loadvm_postcopy_handle_run();
if (ps != POSTCOPY_INCOMING_LISTENING) {
@@ -1942,6 +1942,7 @@ static int
loadvm_postcopy_handle_run(MigrationIncomingState *mis)
return -1;
}
+postcopy_state_set(POSTCOPY_INCOMING_RUNNING);
mis->bh = qemu_bh_new(loadvm_postcopy_handle_run_bh, mis);
qemu_bh_schedule(mis->bh);
--
2.17.1
[PATCH v2 0/2] migration/postcopy: cleanup related to postcopy
Refine a function name and handle on corner case related to PostcopyState. v2: * remove one unnecessary patch * simplify corner case handling Wei Yang (2): migration/postcopy: rename postcopy_ram_enable_notify to postcopy_ram_incoming_setup migration/postcopy: check PostcopyState before setting to POSTCOPY_INCOMING_RUNNING migration/postcopy-ram.c | 4 ++-- migration/postcopy-ram.h | 2 +- migration/savevm.c | 5 +++-- 3 files changed, 6 insertions(+), 5 deletions(-) -- 2.17.1
Re: [PATCH 3/3] migration/postcopy: handle POSTCOPY_INCOMING_RUNNING corner case properly
On Wed, Oct 09, 2019 at 10:08:42AM +0100, Dr. David Alan Gilbert wrote: >> >> > >> >> >Maybe simply fix loadvm_postcopy_handle_run() to set the state after >> >> >the POSTCOPY_INCOMING_LISTENING check? >> >> > >> >> >> >> Set state back to ps if ps is not POSTCOPY_INCOMING_LISTENING? >> >> >> >> Sounds like another option. >> > >> >Even simpler? >> > >> > ps = postcopy_state_get(); >> > if (ps != INCOMING) > > ^^ LISTENING > oops >> >return -1; >> > postcopy_state_set(RUNNING); >> > >> >> Looks good to me. >> >> Dave, >> >> Do you feel good with it? > >Yes, I think so; it's simpler. > I will prepare v2. >Dave > >> >Thanks, >> > >> >-- >> >Peter Xu >> >> -- >> Wei Yang >> Help you, Help me >-- >Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Wei Yang Help you, Help me
Re: [PATCH 3/3] migration/postcopy: handle POSTCOPY_INCOMING_RUNNING corner case properly
On Wed, Oct 09, 2019 at 01:36:34PM +0800, Peter Xu wrote:
>On Wed, Oct 09, 2019 at 01:07:56PM +0800, Wei Yang wrote:
>> On Wed, Oct 09, 2019 at 12:12:25PM +0800, Peter Xu wrote:
>> >On Wed, Oct 09, 2019 at 09:02:04AM +0800, Wei Yang wrote:
>> >> On Tue, Oct 08, 2019 at 05:40:46PM +0100, Dr. David Alan Gilbert wrote:
>> >> >* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> >> >> Currently, we set PostcopyState blindly to RUNNING, even we found the
>> >> >> previous state is not LISTENING. This will lead to a corner case.
>> >> >>
>> >> >> First let's look at the code flow:
>> >> >>
>> >> >> qemu_loadvm_state_main()
>> >> >> ret = loadvm_process_command()
>> >> >> loadvm_postcopy_handle_run()
>> >> >> return -1;
>> >> >> if (ret < 0) {
>> >> >> if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING)
>> >> >> ...
>> >> >> }
>> >> >>
>> >> >> From above snippet, the corner case is loadvm_postcopy_handle_run()
>> >> >> always sets state to RUNNING. And then it checks the previous state. If
>> >> >> the previous state is not LISTENING, it will return -1. But at this
>> >> >> moment, PostcopyState is already been set to RUNNING.
>> >> >>
>> >> >> Then ret is checked in qemu_loadvm_state_main(), when it is -1
>> >> >> PostcopyState is checked. Current logic would pause postcopy and retry
>> >> >> if PostcopyState is RUNNING. This is not what we expect, because
>> >> >> postcopy is not active yet.
>> >> >>
>> >> >> This patch makes sure state is set to RUNNING only previous state is
>> >> >> LISTENING by introducing an old_state parameter in
>> >> >> postcopy_state_set().
>> >> >> New state only would be set when current state equals to old_state.
>> >> >>
>> >> >> Signed-off-by: Wei Yang
>> >> >
>> >> >OK, it's a shame to use a pointer there, but it works.
>> >>
>> >> You mean second parameter of postcopy_state_set()?
>> >>
>> >> I don't have a better idea. Or we introduce a new state
>> >> POSTCOPY_INCOMING_NOCHECK. Do you feel better with this?
>> >
>> >Maybe simply fix loadvm_postcopy_handle_run() to set the state after
>> >the POSTCOPY_INCOMING_LISTENING check?
>> >
>>
>> Set state back to ps if ps is not POSTCOPY_INCOMING_LISTENING?
>>
>> Sounds like another option.
>
>Even simpler?
>
> ps = postcopy_state_get();
> if (ps != INCOMING)
>return -1;
> postcopy_state_set(RUNNING);
>
Looks good to me.
Dave,
Do you feel good with it?
>Thanks,
>
>--
>Peter Xu
--
Wei Yang
Help you, Help me
Re: [PATCH 3/3] migration/postcopy: handle POSTCOPY_INCOMING_RUNNING corner case properly
On Wed, Oct 09, 2019 at 12:12:25PM +0800, Peter Xu wrote:
>On Wed, Oct 09, 2019 at 09:02:04AM +0800, Wei Yang wrote:
>> On Tue, Oct 08, 2019 at 05:40:46PM +0100, Dr. David Alan Gilbert wrote:
>> >* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> >> Currently, we set PostcopyState blindly to RUNNING, even we found the
>> >> previous state is not LISTENING. This will lead to a corner case.
>> >>
>> >> First let's look at the code flow:
>> >>
>> >> qemu_loadvm_state_main()
>> >> ret = loadvm_process_command()
>> >> loadvm_postcopy_handle_run()
>> >> return -1;
>> >> if (ret < 0) {
>> >> if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING)
>> >> ...
>> >> }
>> >>
>> >> From above snippet, the corner case is loadvm_postcopy_handle_run()
>> >> always sets state to RUNNING. And then it checks the previous state. If
>> >> the previous state is not LISTENING, it will return -1. But at this
>> >> moment, PostcopyState is already been set to RUNNING.
>> >>
>> >> Then ret is checked in qemu_loadvm_state_main(), when it is -1
>> >> PostcopyState is checked. Current logic would pause postcopy and retry
>> >> if PostcopyState is RUNNING. This is not what we expect, because
>> >> postcopy is not active yet.
>> >>
>> >> This patch makes sure state is set to RUNNING only previous state is
>> >> LISTENING by introducing an old_state parameter in postcopy_state_set().
>> >> New state only would be set when current state equals to old_state.
>> >>
>> >> Signed-off-by: Wei Yang
>> >
>> >OK, it's a shame to use a pointer there, but it works.
>>
>> You mean second parameter of postcopy_state_set()?
>>
>> I don't have a better idea. Or we introduce a new state
>> POSTCOPY_INCOMING_NOCHECK. Do you feel better with this?
>
>Maybe simply fix loadvm_postcopy_handle_run() to set the state after
>the POSTCOPY_INCOMING_LISTENING check?
>
Set state back to ps if ps is not POSTCOPY_INCOMING_LISTENING?
Sounds like another option.
--
Wei Yang
Help you, Help me
Re: [PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check
On Tue, Oct 08, 2019 at 08:15:51PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> After previous cleanup, postcopy thread is running only when
>> PostcopyState is LISTENNING or RUNNING. This means it is not necessary
>> to spare a variable have_listen_thread to represent the state.
>>
>> Replace the check on have_listen_thread with PostcopyState and remove
>> the variable.
>>
>> Signed-off-by: Wei Yang
>> ---
>> migration/migration.h | 1 -
>> migration/ram.c | 2 +-
>> migration/ram.h | 1 +
>> migration/savevm.c| 4 +---
>> 4 files changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/migration/migration.h b/migration/migration.h
>> index 4f2fe193dc..a4d639663d 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -63,7 +63,6 @@ struct MigrationIncomingState {
>> /* Set this when we want the fault thread to quit */
>> bool fault_thread_quit;
>>
>> -bool have_listen_thread;
>> QemuThread listen_thread;
>> QemuSemaphore listen_thread_sem;
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 769d3f6454..dfc50d57d5 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
>> return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
>> }
>>
>> -static bool postcopy_is_running(void)
>> +bool postcopy_is_running(void)
>> {
>> PostcopyState ps = postcopy_state_get();
>> return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
>> diff --git a/migration/ram.h b/migration/ram.h
>> index bd0eee79b6..44fe4753ad 100644
>> --- a/migration/ram.h
>> +++ b/migration/ram.h
>> @@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
>> /* For incoming postcopy discard */
>> int ram_discard_range(const char *block_name, uint64_t start, size_t
>> length);
>> int ram_postcopy_incoming_init(MigrationIncomingState *mis);
>> +bool postcopy_is_running(void);
>>
>> void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dcad8897a3..2a0e0b94df 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
>> qemu_loadvm_state_cleanup();
>>
>> rcu_unregister_thread();
>> -mis->have_listen_thread = false;
>> postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
>
>That now needs a big comment saying it must be the last thing in the
>thread, because now it's got meaning that it's here.
>
>>
>> return NULL;
>> @@ -1880,7 +1879,6 @@ static int
>> loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
>> return -1;
>> }
>>
>> -mis->have_listen_thread = true;
>> /* Start up the listening thread and wait for it to signal ready */
>> qemu_sem_init(>listen_thread_sem, 0);
>> qemu_thread_create(>listen_thread, "postcopy/listen",
>> @@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>
>> trace_qemu_loadvm_state_post_main(ret);
>>
>> -if (mis->have_listen_thread) {
>> +if (postcopy_is_running()) {
>> /* Listen thread still going, can't clean up yet */
>> return ret;
>> }
>
>Can you explain to me why this is afe in the case of a failure in
>loadvm_postcopy_handle_listen between the start where it sets
>the state to LISTENING, and the point where it currently sets
>hasve_listen_thread ? Wouldn't this cause qemu_loadvm_state
>not to cleanup?
>
I have to say you are right. listen_thread may not started when PostcopyState
is already set to LISTENING.
The ugly fix may be set PostcopyState back to original one. Not sure whether
you would like this.
>Dave
>
>> --
>> 2.17.1
>>
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
Re: [PATCH 1/4] migration/ram: only possible bit set in invalid_flags is RAM_SAVE_FLAG_COMPRESS_PAGE
On Tue, Oct 08, 2019 at 06:38:25PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> The only possible bit set in invalid_flags is
>> RAM_SAVE_FLAG_COMPRESS_PAGE at the beginning of function
>> ram_load_precopy(), which means it is not necessary to do
>> another check for RAM_SAVE_FLAG_COMPRESS_PAGE bit.
>>
>> Signed-off-by: Wei Yang
>> ---
>> migration/ram.c | 5 +
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 31051935c8..769d3f6454 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4263,10 +4263,7 @@ static int ram_load_precopy(QEMUFile *f)
>> addr &= TARGET_PAGE_MASK;
>>
>> if (flags & invalid_flags) {
>> -if (flags & invalid_flags & RAM_SAVE_FLAG_COMPRESS_PAGE) {
>> -error_report("Received an unexpected compressed page");
>> -}
>> -
>> +error_report("Received an unexpected compressed page");
>> ret = -EINVAL;
>
>I'd rather keep this one; I think Juan's idea is that we might make
>other flags illegal here and then it's easy to add to invalid_flags at
>the top.
>
Reasonable.
>Dave
>> break;
>> }
>> --
>> 2.17.1
>>
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
Re: [PATCH 2/2] migration/postcopy: map large zero page in postcopy_ram_incoming_setup()
On Tue, Oct 08, 2019 at 06:24:23PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> postcopy_ram_incoming_setup() and postcopy_ram_incoming_cleanup() are
>> counterpart. It is reasonable to map/unmap large zero page in these two
>> functions respectively.
>>
>> Signed-off-by: Wei Yang
>
>Yes, OK.
>
>> ---
>> migration/postcopy-ram.c | 34 +-
>> 1 file changed, 17 insertions(+), 17 deletions(-)
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index e554f93eec..813cfa5c42 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -1142,6 +1142,22 @@ int
>> postcopy_ram_incoming_setup(MigrationIncomingState *mis)
>> return -1;
>> }
>>
>> +/*
>> + * Map large zero page when kernel can't use UFFDIO_ZEROPAGE for
>> hugepages
>> + */
>> +mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size,
>> + PROT_READ | PROT_WRITE,
>> + MAP_PRIVATE | MAP_ANONYMOUS,
>> + -1, 0);
>> +if (mis->postcopy_tmp_zero_page == MAP_FAILED) {
>> +int e = errno;
>> +mis->postcopy_tmp_zero_page = NULL;
>> +error_report("%s: Failed to map large zero page %s",
>> + __func__, strerror(e));
>> +return -e;
>
>Note this starts returning -errno where the rest of this function
>returns 0 or -1; returning -errno is a good thing I think and it might
>be good to change the other returns.
>
This is reasonable, while I am thinking how caller would handle this.
For example, the return value would be finally returned to
qemu_loadvm_state_main(). If we handle each errno respectively in this
function, the code would be difficult to read and maintain.
Would it be good to classify return value to different category? Like
POSTCOPY_FATAL
POSTCOPY_RETRY
POSTCOPY_DISABLE
>
>Reviewed-by: Dr. David Alan Gilbert
>
>> +}
>> +memset(mis->postcopy_tmp_zero_page, '\0', mis->largest_page_size);
>> +
>> /*
>> * Ballooning can mark pages as absent while we're postcopying
>> * that would cause false userfaults.
>> @@ -1248,23 +1264,7 @@ int postcopy_place_page_zero(MigrationIncomingState
>> *mis, void *host,
>> qemu_ram_block_host_offset(rb,
>>
>> host));
>> } else {
>> -/* The kernel can't use UFFDIO_ZEROPAGE for hugepages */
>> -if (!mis->postcopy_tmp_zero_page) {
>> -mis->postcopy_tmp_zero_page = mmap(NULL, mis->largest_page_size,
>> - PROT_READ | PROT_WRITE,
>> - MAP_PRIVATE | MAP_ANONYMOUS,
>> - -1, 0);
>> -if (mis->postcopy_tmp_zero_page == MAP_FAILED) {
>> -int e = errno;
>> -mis->postcopy_tmp_zero_page = NULL;
>> -error_report("%s: %s mapping large zero page",
>> - __func__, strerror(e));
>> -return -e;
>> - }
>> -memset(mis->postcopy_tmp_zero_page, '\0',
>> mis->largest_page_size);
>> -}
>> -return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page,
>> - rb);
>> +return postcopy_place_page(mis, host, mis->postcopy_tmp_zero_page,
>> rb);
>> }
>> }
>>
>> --
>> 2.17.1
>>
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
Re: [PATCH 3/3] migration/postcopy: handle POSTCOPY_INCOMING_RUNNING corner case properly
On Tue, Oct 08, 2019 at 05:40:46PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> Currently, we set PostcopyState blindly to RUNNING, even we found the
>> previous state is not LISTENING. This will lead to a corner case.
>>
>> First let's look at the code flow:
>>
>> qemu_loadvm_state_main()
>> ret = loadvm_process_command()
>> loadvm_postcopy_handle_run()
>> return -1;
>> if (ret < 0) {
>> if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING)
>> ...
>> }
>>
>> From above snippet, the corner case is loadvm_postcopy_handle_run()
>> always sets state to RUNNING. And then it checks the previous state. If
>> the previous state is not LISTENING, it will return -1. But at this
>> moment, PostcopyState is already been set to RUNNING.
>>
>> Then ret is checked in qemu_loadvm_state_main(), when it is -1
>> PostcopyState is checked. Current logic would pause postcopy and retry
>> if PostcopyState is RUNNING. This is not what we expect, because
>> postcopy is not active yet.
>>
>> This patch makes sure state is set to RUNNING only previous state is
>> LISTENING by introducing an old_state parameter in postcopy_state_set().
>> New state only would be set when current state equals to old_state.
>>
>> Signed-off-by: Wei Yang
>
>OK, it's a shame to use a pointer there, but it works.
You mean second parameter of postcopy_state_set()?
I don't have a better idea. Or we introduce a new state
POSTCOPY_INCOMING_NOCHECK. Do you feel better with this?
>Note, something else; using '-1' as the return value and checking for it
>is something we do a lot; but in this case it's an example of an error
>we could never recover from so it never makes sense to try and recover.
>We should probably look at different types of error.
>
>
>Reviewed-by: Dr. David Alan Gilbert
>
>Dave
>
>> ---
>> migration/migration.c| 2 +-
>> migration/postcopy-ram.c | 13 +
>> migration/postcopy-ram.h | 3 ++-
>> migration/savevm.c | 11 ++-
>> 4 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 34d5e66f06..369cf3826e 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -447,7 +447,7 @@ static void process_incoming_migration_co(void *opaque)
>> assert(mis->from_src_file);
>> mis->migration_incoming_co = qemu_coroutine_self();
>> mis->largest_page_size = qemu_ram_pagesize_largest();
>> -postcopy_state_set(POSTCOPY_INCOMING_NONE);
>> +postcopy_state_set(POSTCOPY_INCOMING_NONE, NULL);
>> migrate_set_state(>state, MIGRATION_STATUS_NONE,
>>MIGRATION_STATUS_ACTIVE);
>> ret = qemu_loadvm_state(mis->from_src_file);
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index b24c4a10c2..8f741d636d 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -577,7 +577,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState
>> *mis)
>> }
>> }
>>
>> -postcopy_state_set(POSTCOPY_INCOMING_END);
>> +postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
>>
>> if (mis->postcopy_tmp_page) {
>> munmap(mis->postcopy_tmp_page, mis->largest_page_size);
>> @@ -626,7 +626,7 @@ int postcopy_ram_prepare_discard(MigrationIncomingState
>> *mis)
>> return -1;
>> }
>>
>> -postcopy_state_set(POSTCOPY_INCOMING_DISCARD);
>> +postcopy_state_set(POSTCOPY_INCOMING_DISCARD, NULL);
>>
>> return 0;
>> }
>> @@ -1457,9 +1457,14 @@ PostcopyState postcopy_state_get(void)
>> }
>>
>> /* Set the state and return the old state */
>> -PostcopyState postcopy_state_set(PostcopyState new_state)
>> +PostcopyState postcopy_state_set(PostcopyState new_state,
>> + const PostcopyState *old_state)
>> {
>> -return atomic_xchg(_postcopy_state, new_state);
>> +if (!old_state) {
>> +return atomic_xchg(_postcopy_state, new_state);
>> +} else {
>> +return atomic_cmpxchg(_postcopy_state, *old_state,
>> new_state);
>> +}
>> }
>>
>> /* Register a handler for external shared memory postcopy
>> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
>> index d2668cc820..e3dde32155 100644
>> --- a/migration/postcopy-ram
Re: [PATCH 2/3] migration/postcopy: not necessary to do postcopy_ram_incoming_cleanup when state is ADVISE
On Tue, Oct 08, 2019 at 05:02:02PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.y...@linux.intel.com) wrote:
>> postcopy_ram_incoming_cleanup() does cleanup for
>> postcopy_ram_incoming_setup(), while the setup happens only after
>> migration enters LISTEN state.
>>
>> This means there is nothing to cleanup when migration is still ADVISE
>> state.
>>
>> Signed-off-by: Wei Yang
>> ---
>> migration/migration.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5f7e4d15e9..34d5e66f06 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -461,7 +461,6 @@ static void process_incoming_migration_co(void *opaque)
>> * but managed to complete within the precopy period, we can use
>> * the normal exit.
>> */
>> -postcopy_ram_incoming_cleanup(mis);
>> } else if (ret >= 0) {
>> /*
>> * Postcopy was started, cleanup should happen at the end of the
>
>I think that misses the cleanup of mlock that corresponds to the
>munlockall in postcopy_ram_supported_by_host - that's called very early
>on; I think in the advise stage.
>
Thanks you are right.
BTW, do we need to check enable_mlock when calling munlockall() in
postcopy_ram_supported_by_host() ?
>Dave
>
>> --
>> 2.17.1
>>
>--
>Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Wei Yang
Help you, Help me
[PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
ram_discard_range() unmap page for specific range. To be specific, this
clears related page table entries so that userfault would be triggered.
But this step is not necessary at the very beginning.
ram_postcopy_incoming_init() is called when destination gets ADVISE
command. ADVISE command is sent when migration thread just starts, which
implies destination is not running yet. This means no page fault
happened and memory region's page tables entries are empty.
This patch removes the discard at the beginning.
Signed-off-by: Wei Yang
---
migration/postcopy-ram.c | 46
migration/postcopy-ram.h | 7 --
migration/ram.c | 16 --
migration/ram.h | 1 -
migration/savevm.c | 4
5 files changed, 74 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 5da6de8c8b..459be8e780 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -443,32 +443,6 @@ out:
return ret;
}
-/*
- * Setup an area of RAM so that it *can* be used for postcopy later; this
- * must be done right at the start prior to pre-copy.
- * opaque should be the MIS.
- */
-static int init_range(RAMBlock *rb, void *opaque)
-{
-const char *block_name = qemu_ram_get_idstr(rb);
-void *host_addr = qemu_ram_get_host_addr(rb);
-ram_addr_t offset = qemu_ram_get_offset(rb);
-ram_addr_t length = qemu_ram_get_used_length(rb);
-trace_postcopy_init_range(block_name, host_addr, offset, length);
-
-/*
- * We need the whole of RAM to be truly empty for postcopy, so things
- * like ROMs and any data tables built during init must be zero'd
- * - we're going to get the copy from the source anyway.
- * (Precopy will just overwrite this data, so doesn't need the discard)
- */
-if (ram_discard_range(block_name, 0, length)) {
-return -1;
-}
-
-return 0;
-}
-
/*
* At the end of migration, undo the effects of init_range
* opaque should be the MIS.
@@ -506,20 +480,6 @@ static int cleanup_range(RAMBlock *rb, void *opaque)
return 0;
}
-/*
- * Initialise postcopy-ram, setting the RAM to a state where we can go into
- * postcopy later; must be called prior to any precopy.
- * called from arch_init's similarly named ram_postcopy_incoming_init
- */
-int postcopy_ram_incoming_init(MigrationIncomingState *mis)
-{
-if (foreach_not_ignored_block(init_range, NULL)) {
-return -1;
-}
-
-return 0;
-}
-
/*
* Manage a single vote to the QEMU balloon inhibitor for all postcopy usage,
* last caller wins.
@@ -1282,12 +1242,6 @@ bool
postcopy_ram_supported_by_host(MigrationIncomingState *mis)
return false;
}
-int postcopy_ram_incoming_init(MigrationIncomingState *mis)
-{
-error_report("postcopy_ram_incoming_init: No OS support");
-return -1;
-}
-
int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
{
assert(0);
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index c0ccf64a96..1c79c6e51f 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -22,13 +22,6 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState
*mis);
*/
int postcopy_ram_incoming_setup(MigrationIncomingState *mis);
-/*
- * Initialise postcopy-ram, setting the RAM to a state where we can go into
- * postcopy later; must be called prior to any precopy.
- * called from ram.c's similarly named ram_postcopy_incoming_init
- */
-int postcopy_ram_incoming_init(MigrationIncomingState *mis);
-
/*
* At the end of a migration where postcopy_ram_incoming_init was called.
*/
diff --git a/migration/ram.c b/migration/ram.c
index dfc50d57d5..9a853703d8 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4015,22 +4015,6 @@ static int ram_load_cleanup(void *opaque)
return 0;
}
-/**
- * ram_postcopy_incoming_init: allocate postcopy data structures
- *
- * Returns 0 for success and negative if there was one error
- *
- * @mis: current migration incoming state
- *
- * Allocate data structures etc needed by incoming migration with
- * postcopy-ram. postcopy-ram's similarly names
- * postcopy_ram_incoming_init does the work.
- */
-int ram_postcopy_incoming_init(MigrationIncomingState *mis)
-{
-return postcopy_ram_incoming_init(mis);
-}
-
/**
* ram_load_postcopy: load a page in postcopy case
*
diff --git a/migration/ram.h b/migration/ram.h
index 44fe4753ad..66cbff1d52 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -58,7 +58,6 @@ void ram_postcopy_migrated_memory_release(MigrationState *ms);
int ram_postcopy_send_discard_bitmap(MigrationState *ms);
/* For incoming postcopy discard */
int ram_discard_range(const char *block_name, uint64_t start, size_t length);
-int ram_postcopy_incoming_init(MigrationIncomingState *mis);
bool postcopy_is_running(void);
void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
diff --git a/migration/savevm.c b/migration/savevm.c
index 9dc191e0a0..
[PATCH 2/3] migration/postcopy: postpone setting PostcopyState to END
There are two places to call function postcopy_ram_incoming_cleanup()
postcopy_ram_listen_thread on migration success
loadvm_postcopy_handle_listen one setup failure
On success, the vm will never accept another migration. On failure,
PostcopyState is transited from LISTENING to END and would be checked in
qemu_loadvm_state_main(). If PostcopyState is RUNNING, migration would
be paused and retried.
Currently PostcopyState is set to END in function
postcopy_ram_incoming_cleanup(). With above analysis, we can take this
step out and postpone this till the end of listen thread to indicate the
listen thread is done.
This is a preparation patch for later cleanup.
Signed-off-by: Wei Yang
---
migration/postcopy-ram.c | 2 --
migration/savevm.c | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a394c7c3a6..5da6de8c8b 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -577,8 +577,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState
*mis)
}
}
-postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
-
if (mis->postcopy_tmp_page) {
munmap(mis->postcopy_tmp_page, mis->largest_page_size);
mis->postcopy_tmp_page = NULL;
diff --git a/migration/savevm.c b/migration/savevm.c
index eaa4cf58ef..dcad8897a3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1837,6 +1837,8 @@ static void *postcopy_ram_listen_thread(void *opaque)
rcu_unregister_thread();
mis->have_listen_thread = false;
+postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
+
return NULL;
}
--
2.17.1
[PATCH 3/3] migration/postcopy: replace have_listen_thread check with PostcopyState check
After previous cleanup, postcopy thread is running only when
PostcopyState is LISTENNING or RUNNING. This means it is not necessary
to spare a variable have_listen_thread to represent the state.
Replace the check on have_listen_thread with PostcopyState and remove
the variable.
Signed-off-by: Wei Yang
---
migration/migration.h | 1 -
migration/ram.c | 2 +-
migration/ram.h | 1 +
migration/savevm.c| 4 +---
4 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/migration/migration.h b/migration/migration.h
index 4f2fe193dc..a4d639663d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -63,7 +63,6 @@ struct MigrationIncomingState {
/* Set this when we want the fault thread to quit */
bool fault_thread_quit;
-bool have_listen_thread;
QemuThread listen_thread;
QemuSemaphore listen_thread_sem;
diff --git a/migration/ram.c b/migration/ram.c
index 769d3f6454..dfc50d57d5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4188,7 +4188,7 @@ static bool postcopy_is_advised(void)
return ps >= POSTCOPY_INCOMING_ADVISE && ps < POSTCOPY_INCOMING_END;
}
-static bool postcopy_is_running(void)
+bool postcopy_is_running(void)
{
PostcopyState ps = postcopy_state_get();
return ps >= POSTCOPY_INCOMING_LISTENING && ps < POSTCOPY_INCOMING_END;
diff --git a/migration/ram.h b/migration/ram.h
index bd0eee79b6..44fe4753ad 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -59,6 +59,7 @@ int ram_postcopy_send_discard_bitmap(MigrationState *ms);
/* For incoming postcopy discard */
int ram_discard_range(const char *block_name, uint64_t start, size_t length);
int ram_postcopy_incoming_init(MigrationIncomingState *mis);
+bool postcopy_is_running(void);
void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
diff --git a/migration/savevm.c b/migration/savevm.c
index dcad8897a3..2a0e0b94df 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1836,7 +1836,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
qemu_loadvm_state_cleanup();
rcu_unregister_thread();
-mis->have_listen_thread = false;
postcopy_state_set(POSTCOPY_INCOMING_END, NULL);
return NULL;
@@ -1880,7 +1879,6 @@ static int
loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
return -1;
}
-mis->have_listen_thread = true;
/* Start up the listening thread and wait for it to signal ready */
qemu_sem_init(>listen_thread_sem, 0);
qemu_thread_create(>listen_thread, "postcopy/listen",
@@ -2518,7 +2516,7 @@ int qemu_loadvm_state(QEMUFile *f)
trace_qemu_loadvm_state_post_main(ret);
-if (mis->have_listen_thread) {
+if (postcopy_is_running()) {
/* Listen thread still going, can't clean up yet */
return ret;
}
--
2.17.1
[PATCH 1/3] migration/postcopy: mis->have_listen_thread check will never be touched
If mis->have_listen_thread is true, this means current PostcopyState
must be LISTENING or RUNNING. While the check at the beginning of the
function makes sure the state transaction happens when its previous
PostcopyState is ADVISE or DISCARD.
This means we would never touch this check.
Signed-off-by: Wei Yang
---
migration/savevm.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index feb757de79..eaa4cf58ef 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1878,11 +1878,6 @@ static int
loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
return -1;
}
-if (mis->have_listen_thread) {
-error_report("CMD_POSTCOPY_RAM_LISTEN already has a listen thread");
-return -1;
-}
-
mis->have_listen_thread = true;
/* Start up the listening thread and wait for it to signal ready */
qemu_sem_init(>listen_thread_sem, 0);
--
2.17.1
[PATCH 0/3] migration/postcopy: replace have_listen_thread check with PostcopyState check
have_listen_thread is used to be a indication of whether postcopy thread is running. Since we use PostcopyState to record state and the postcopy thread only runs when postcopy_is_running(), we can leverage the PostcopyState to replace the meaning of have_listen_thread. To do so, two preparation cleanup is included. Wei Yang (3): migration/postcopy: mis->have_listen_thread check will never be touched migration/postcopy: postpone setting PostcopyState to END migration/postcopy: replace have_listen_thread check with PostcopyState check migration/migration.h| 1 - migration/postcopy-ram.c | 2 -- migration/ram.c | 2 +- migration/ram.h | 1 + migration/savevm.c | 11 +++ 5 files changed, 5 insertions(+), 12 deletions(-) -- 2.17.1
[PATCH 0/4] migration: trivial cleanup and refine
This patch set contains several cleanup and refine for migration. simplify some calculation reuse the result fix typo in comment provide error message when failed Wei Yang (4): migration/ram: only possible bit set in invalid_flags is RAM_SAVE_FLAG_COMPRESS_PAGE migration/postcopy: fix typo in mark_postcopy_blocktime_begin's comment migration: pass in_postcopy instead of check state again migration: report SaveStateEntry id and name on failure migration/migration.c| 3 +-- migration/postcopy-ram.c | 8 +--- migration/ram.c | 5 + migration/savevm.c | 2 ++ 4 files changed, 9 insertions(+), 9 deletions(-) -- 2.17.1
[PATCH 4/4] migration: report SaveStateEntry id and name on failure
This provides helpful information on which entry failed.
Signed-off-by: Wei Yang
---
migration/savevm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9f0122583d..feb757de79 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1215,6 +1215,8 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
save_section_footer(f, se);
if (ret < 0) {
+error_report("failed to save SaveStateEntry with id(name): %d(%s)",
+ se->section_id, se->idstr);
qemu_file_set_error(f, ret);
}
if (ret <= 0) {
--
2.17.1
[PATCH 3/4] migration: pass in_postcopy instead of check state again
Not necessary to do the check again.
Signed-off-by: Wei Yang
---
migration/migration.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index c8eaa85867..56031305e3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3148,8 +3148,7 @@ static MigIterateState
migration_iteration_run(MigrationState *s)
return MIG_ITERATE_SKIP;
}
/* Just another iteration step */
-qemu_savevm_state_iterate(s->to_dst_file,
-s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
+qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
} else {
trace_migration_thread_low_pending(pending_size);
migration_completion(s);
--
2.17.1
[PATCH 2/4] migration/postcopy: fix typo in mark_postcopy_blocktime_begin's comment
Signed-off-by: Wei Yang
---
migration/postcopy-ram.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index d2bdd21ae3..a394c7c3a6 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -768,9 +768,11 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr,
uint32_t ptid,
atomic_xchg(>page_fault_vcpu_time[cpu], low_time_offset);
atomic_xchg(>vcpu_addr[cpu], addr);
-/* check it here, not at the begining of the function,
- * due to, check could accur early than bitmap_set in
- * qemu_ufd_copy_ioctl */
+/*
+ * check it here, not at the beginning of the function,
+ * due to, check could occur early than bitmap_set in
+ * qemu_ufd_copy_ioctl
+ */
already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
if (already_received) {
atomic_xchg(>vcpu_addr[cpu], 0);
--
2.17.1
[PATCH 1/4] migration/ram: only possible bit set in invalid_flags is RAM_SAVE_FLAG_COMPRESS_PAGE
The only possible bit set in invalid_flags is
RAM_SAVE_FLAG_COMPRESS_PAGE at the beginning of function
ram_load_precopy(), which means it is not necessary to do
another check for RAM_SAVE_FLAG_COMPRESS_PAGE bit.
Signed-off-by: Wei Yang
---
migration/ram.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 31051935c8..769d3f6454 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4263,10 +4263,7 @@ static int ram_load_precopy(QEMUFile *f)
addr &= TARGET_PAGE_MASK;
if (flags & invalid_flags) {
-if (flags & invalid_flags & RAM_SAVE_FLAG_COMPRESS_PAGE) {
-error_report("Received an unexpected compressed page");
-}
-
+error_report("Received an unexpected compressed page");
ret = -EINVAL;
break;
}
--
2.17.1
