* Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
2019-10-07 9:10 [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning Wei Yang
@ 2020-01-13 2:22 ` Wei Yang
2020-01-31 0:15 ` Wei Yang
2020-02-13 10:17 ` Dr. David Alan Gilbert
2 siblings, 0 replies; 5+ messages in thread
From: Wei Yang @ 2020-01-13 2:22 UTC (permalink / raw)
To: Wei Yang; +Cc: qemu-devel, dgilbert, quintela
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 <richardw.yang@linux.intel.com>
>---
> 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..d2a427a3bf 100644
>--- a/migration/savevm.c
>+++ b/migration/savevm.c
>@@ -1674,10 +1674,6 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> return -1;
> }
>
>- if (ram_postcopy_incoming_init(mis)) {
>- return -1;
>- }
>-
> return 0;
> }
>
>--
>2.17.1
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
2019-10-07 9:10 [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning Wei Yang
2020-01-13 2:22 ` Wei Yang
@ 2020-01-31 0:15 ` Wei Yang
2020-02-13 10:17 ` Dr. David Alan Gilbert
2 siblings, 0 replies; 5+ messages in thread
From: Wei Yang @ 2020-01-31 0:15 UTC (permalink / raw)
To: Wei Yang; +Cc: qemu-devel, dgilbert, quintela
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 <richardw.yang@linux.intel.com>
>---
> 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..d2a427a3bf 100644
>--- a/migration/savevm.c
>+++ b/migration/savevm.c
>@@ -1674,10 +1674,6 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> return -1;
> }
>
>- if (ram_postcopy_incoming_init(mis)) {
>- return -1;
>- }
>-
> return 0;
> }
>
>--
>2.17.1
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
2019-10-07 9:10 [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning Wei Yang
2020-01-13 2:22 ` Wei Yang
2020-01-31 0:15 ` Wei Yang
@ 2020-02-13 10:17 ` Dr. David Alan Gilbert
2020-02-13 22:13 ` Wei Yang
2 siblings, 1 reply; 5+ messages in thread
From: Dr. David Alan Gilbert @ 2020-02-13 10:17 UTC (permalink / raw)
To: Wei Yang; +Cc: qemu-devel, quintela
* Wei Yang (richardw.yang@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 <richardw.yang@linux.intel.com>
> ---
> 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.
Dave
> - 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..d2a427a3bf 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1674,10 +1674,6 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis,
> return -1;
> }
>
> - if (ram_postcopy_incoming_init(mis)) {
> - return -1;
> - }
> -
> return 0;
> }
>
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] migration/postcopy: not necessary to discard all RAM at the beginning
2020-02-13 10:17 ` Dr. David Alan Gilbert
@ 2020-02-13 22:13 ` Wei Yang
0 siblings, 0 replies; 5+ messages in thread
From: Wei Yang @ 2020-02-13 22:13 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, Wei Yang, quintela
On Thu, Feb 13, 2020 at 10:17:04AM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@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 <richardw.yang@linux.intel.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread