diff mbox series

[5/6] migration/postcopy: enable random order target page arrival

Message ID 20191018004850.9888-6-richardw.yang@linux.intel.com
State New
Headers show
Series migration/postcopy: enable compress during postcopy | expand

Commit Message

Wei Yang Oct. 18, 2019, 12:48 a.m. UTC
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 <richardw.yang@linux.intel.com>
---
 migration/ram.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

Comments

Dr. David Alan Gilbert Nov. 6, 2019, 8:08 p.m. UTC | #1
* Wei Yang (richardw.yang@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 <richardw.yang@linux.intel.com>
> ---
>  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)

and then set last_host to the start of the host 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 / dgilbert@redhat.com / Manchester, UK
Wei Yang Nov. 7, 2019, 6 a.m. UTC | #2
On Wed, Nov 06, 2019 at 08:08:28PM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@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 <richardw.yang@linux.intel.com>
>> ---
>>  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 / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert Nov. 7, 2019, 9:14 a.m. UTC | #3
* Wei Yang (richardw.yang@linux.intel.com) wrote:
> On Wed, Nov 06, 2019 at 08:08:28PM +0000, Dr. David Alan Gilbert wrote:
> >* Wei Yang (richardw.yang@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 <richardw.yang@linux.intel.com>
> >> ---
> >>  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.

Yes, that would be fine.

Dave

> >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 / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Wei Yang
> Help you, Help me
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

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,