diff mbox

[V6,02/10] migration: pass MigrationIncomingState* into migration check functions

Message ID 1495539071-12995-3-git-send-email-a.perevalov@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alexey Perevalov May 23, 2017, 11:31 a.m. UTC
That tiny refactoring is necessary to be able to set
UFFD_FEATURE_THREAD_ID while requesting features, and then
to create downtime context in case when kernel supports it.

Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
---
 migration/migration.c    |  2 +-
 migration/postcopy-ram.c | 10 +++++-----
 migration/postcopy-ram.h |  2 +-
 migration/savevm.c       |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

Comments

Dr. David Alan Gilbert May 31, 2017, 5:54 p.m. UTC | #1
* Alexey Perevalov (a.perevalov@samsung.com) wrote:
> That tiny refactoring is necessary to be able to set
> UFFD_FEATURE_THREAD_ID while requesting features, and then
> to create downtime context in case when kernel supports it.
> 
> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> ---
>  migration/migration.c    |  2 +-
>  migration/postcopy-ram.c | 10 +++++-----
>  migration/postcopy-ram.h |  2 +-
>  migration/savevm.c       |  2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0304c01..d735976 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -789,7 +789,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>           * special support.
>           */
>          if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
> -            !postcopy_ram_supported_by_host()) {
> +            !postcopy_ram_supported_by_host(NULL)) {

Why is that NULL rather than migration_incoming_get_current()?
That's on the destination.

Dave

>              /* postcopy_ram_supported_by_host will have emitted a more
>               * detailed message
>               */
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a0489f6..4adab36 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -59,7 +59,7 @@ struct PostcopyDiscardState {
>  #include <sys/eventfd.h>
>  #include <linux/userfaultfd.h>
>  
> -static bool ufd_version_check(int ufd)
> +static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
>  {
>      struct uffdio_api api_struct;
>      uint64_t ioctl_mask;
> @@ -112,7 +112,7 @@ static int test_range_shared(const char *block_name, void *host_addr,
>   * normally fine since if the postcopy succeeds it gets turned back on at the
>   * end.
>   */
> -bool postcopy_ram_supported_by_host(void)
> +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>  {
>      long pagesize = getpagesize();
>      int ufd = -1;
> @@ -135,7 +135,7 @@ bool postcopy_ram_supported_by_host(void)
>      }
>  
>      /* Version and features check */
> -    if (!ufd_version_check(ufd)) {
> +    if (!ufd_version_check(ufd, mis)) {
>          goto out;
>      }
>  
> @@ -512,7 +512,7 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>       * Although the host check already tested the API, we need to
>       * do the check again as an ABI handshake on the new fd.
>       */
> -    if (!ufd_version_check(mis->userfault_fd)) {
> +    if (!ufd_version_check(mis->userfault_fd, mis)) {
>          return -1;
>      }
>  
> @@ -650,7 +650,7 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
>  
>  #else
>  /* No target OS support, stubs just fail */
> -bool postcopy_ram_supported_by_host(void)
> +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>  {
>      error_report("%s: No OS support", __func__);
>      return false;
> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> index 52d51e8..587a8b8 100644
> --- a/migration/postcopy-ram.h
> +++ b/migration/postcopy-ram.h
> @@ -14,7 +14,7 @@
>  #define QEMU_POSTCOPY_RAM_H
>  
>  /* Return true if the host supports everything we need to do postcopy-ram */
> -bool postcopy_ram_supported_by_host(void);
> +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
>  
>  /*
>   * Make all of RAM sensitive to accesses to areas that haven't yet been written
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f5e8194..61a6df7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1356,7 +1356,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
>          return -1;
>      }
>  
> -    if (!postcopy_ram_supported_by_host()) {
> +    if (!postcopy_ram_supported_by_host(mis)) {
>          postcopy_state_set(POSTCOPY_INCOMING_NONE);
>          return -1;
>      }
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Alexey Perevalov June 5, 2017, 5:59 a.m. UTC | #2
On 05/31/2017 08:54 PM, Dr. David Alan Gilbert wrote:
> * Alexey Perevalov (a.perevalov@samsung.com) wrote:
>> That tiny refactoring is necessary to be able to set
>> UFFD_FEATURE_THREAD_ID while requesting features, and then
>> to create downtime context in case when kernel supports it.
>>
>> Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
>> ---
>>   migration/migration.c    |  2 +-
>>   migration/postcopy-ram.c | 10 +++++-----
>>   migration/postcopy-ram.h |  2 +-
>>   migration/savevm.c       |  2 +-
>>   4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 0304c01..d735976 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -789,7 +789,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>>            * special support.
>>            */
>>           if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
>> -            !postcopy_ram_supported_by_host()) {
>> +            !postcopy_ram_supported_by_host(NULL)) {
> Why is that NULL rather than migration_incoming_get_current()?
> That's on the destination.
In current implementation, AFAIU,
migrate-set-capabilities with postcopy-ram argument should be done on 
source machine only,
and it's not required on destination. And it is unnecessary requirement 
to support userfaultfd on
source and check it there.

>
> Dave
>
>>               /* postcopy_ram_supported_by_host will have emitted a more
>>                * detailed message
>>                */
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index a0489f6..4adab36 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -59,7 +59,7 @@ struct PostcopyDiscardState {
>>   #include <sys/eventfd.h>
>>   #include <linux/userfaultfd.h>
>>   
>> -static bool ufd_version_check(int ufd)
>> +static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
>>   {
>>       struct uffdio_api api_struct;
>>       uint64_t ioctl_mask;
>> @@ -112,7 +112,7 @@ static int test_range_shared(const char *block_name, void *host_addr,
>>    * normally fine since if the postcopy succeeds it gets turned back on at the
>>    * end.
>>    */
>> -bool postcopy_ram_supported_by_host(void)
>> +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>>   {
>>       long pagesize = getpagesize();
>>       int ufd = -1;
>> @@ -135,7 +135,7 @@ bool postcopy_ram_supported_by_host(void)
>>       }
>>   
>>       /* Version and features check */
>> -    if (!ufd_version_check(ufd)) {
>> +    if (!ufd_version_check(ufd, mis)) {
>>           goto out;
>>       }
>>   
>> @@ -512,7 +512,7 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
>>        * Although the host check already tested the API, we need to
>>        * do the check again as an ABI handshake on the new fd.
>>        */
>> -    if (!ufd_version_check(mis->userfault_fd)) {
>> +    if (!ufd_version_check(mis->userfault_fd, mis)) {
>>           return -1;
>>       }
>>   
>> @@ -650,7 +650,7 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
>>   
>>   #else
>>   /* No target OS support, stubs just fail */
>> -bool postcopy_ram_supported_by_host(void)
>> +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
>>   {
>>       error_report("%s: No OS support", __func__);
>>       return false;
>> diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
>> index 52d51e8..587a8b8 100644
>> --- a/migration/postcopy-ram.h
>> +++ b/migration/postcopy-ram.h
>> @@ -14,7 +14,7 @@
>>   #define QEMU_POSTCOPY_RAM_H
>>   
>>   /* Return true if the host supports everything we need to do postcopy-ram */
>> -bool postcopy_ram_supported_by_host(void);
>> +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
>>   
>>   /*
>>    * Make all of RAM sensitive to accesses to areas that haven't yet been written
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f5e8194..61a6df7 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1356,7 +1356,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
>>           return -1;
>>       }
>>   
>> -    if (!postcopy_ram_supported_by_host()) {
>> +    if (!postcopy_ram_supported_by_host(mis)) {
>>           postcopy_state_set(POSTCOPY_INCOMING_NONE);
>>           return -1;
>>       }
>> -- 
>> 1.8.3.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
>
Dr. David Alan Gilbert June 5, 2017, 9:15 a.m. UTC | #3
* Alexey Perevalov (a.perevalov@samsung.com) wrote:
> On 05/31/2017 08:54 PM, Dr. David Alan Gilbert wrote:
> > * Alexey Perevalov (a.perevalov@samsung.com) wrote:
> > > That tiny refactoring is necessary to be able to set
> > > UFFD_FEATURE_THREAD_ID while requesting features, and then
> > > to create downtime context in case when kernel supports it.
> > > 
> > > Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com>
> > > ---
> > >   migration/migration.c    |  2 +-
> > >   migration/postcopy-ram.c | 10 +++++-----
> > >   migration/postcopy-ram.h |  2 +-
> > >   migration/savevm.c       |  2 +-
> > >   4 files changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index 0304c01..d735976 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -789,7 +789,7 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> > >            * special support.
> > >            */
> > >           if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
> > > -            !postcopy_ram_supported_by_host()) {
> > > +            !postcopy_ram_supported_by_host(NULL)) {
> > Why is that NULL rather than migration_incoming_get_current()?
> > That's on the destination.
> In current implementation, AFAIU,
> migrate-set-capabilities with postcopy-ram argument should be done on source
> machine only,
> and it's not required on destination. And it is unnecessary requirement to
> support userfaultfd on
> source and check it there.

Note the 'runstate_check(RUN_STATE_INMIGRATE)' - this check is made on
the destination.
While it's not strictly necessary for the management layer to do a
migrate_set_capability postcopy-ram   on the destination, it does it
anyway to check the destination is capable of postcopy before it starts
migration.

Dave

> 
> > 
> > Dave
> > 
> > >               /* postcopy_ram_supported_by_host will have emitted a more
> > >                * detailed message
> > >                */
> > > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > > index a0489f6..4adab36 100644
> > > --- a/migration/postcopy-ram.c
> > > +++ b/migration/postcopy-ram.c
> > > @@ -59,7 +59,7 @@ struct PostcopyDiscardState {
> > >   #include <sys/eventfd.h>
> > >   #include <linux/userfaultfd.h>
> > > -static bool ufd_version_check(int ufd)
> > > +static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
> > >   {
> > >       struct uffdio_api api_struct;
> > >       uint64_t ioctl_mask;
> > > @@ -112,7 +112,7 @@ static int test_range_shared(const char *block_name, void *host_addr,
> > >    * normally fine since if the postcopy succeeds it gets turned back on at the
> > >    * end.
> > >    */
> > > -bool postcopy_ram_supported_by_host(void)
> > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
> > >   {
> > >       long pagesize = getpagesize();
> > >       int ufd = -1;
> > > @@ -135,7 +135,7 @@ bool postcopy_ram_supported_by_host(void)
> > >       }
> > >       /* Version and features check */
> > > -    if (!ufd_version_check(ufd)) {
> > > +    if (!ufd_version_check(ufd, mis)) {
> > >           goto out;
> > >       }
> > > @@ -512,7 +512,7 @@ int postcopy_ram_enable_notify(MigrationIncomingState *mis)
> > >        * Although the host check already tested the API, we need to
> > >        * do the check again as an ABI handshake on the new fd.
> > >        */
> > > -    if (!ufd_version_check(mis->userfault_fd)) {
> > > +    if (!ufd_version_check(mis->userfault_fd, mis)) {
> > >           return -1;
> > >       }
> > > @@ -650,7 +650,7 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
> > >   #else
> > >   /* No target OS support, stubs just fail */
> > > -bool postcopy_ram_supported_by_host(void)
> > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
> > >   {
> > >       error_report("%s: No OS support", __func__);
> > >       return false;
> > > diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
> > > index 52d51e8..587a8b8 100644
> > > --- a/migration/postcopy-ram.h
> > > +++ b/migration/postcopy-ram.h
> > > @@ -14,7 +14,7 @@
> > >   #define QEMU_POSTCOPY_RAM_H
> > >   /* Return true if the host supports everything we need to do postcopy-ram */
> > > -bool postcopy_ram_supported_by_host(void);
> > > +bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
> > >   /*
> > >    * Make all of RAM sensitive to accesses to areas that haven't yet been written
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index f5e8194..61a6df7 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -1356,7 +1356,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
> > >           return -1;
> > >       }
> > > -    if (!postcopy_ram_supported_by_host()) {
> > > +    if (!postcopy_ram_supported_by_host(mis)) {
> > >           postcopy_state_set(POSTCOPY_INCOMING_NONE);
> > >           return -1;
> > >       }
> > > -- 
> > > 1.8.3.1
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> > 
> > 
> 
> -- 
> Best regards,
> Alexey Perevalov
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 0304c01..d735976 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -789,7 +789,7 @@  void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
          * special support.
          */
         if (!old_postcopy_cap && runstate_check(RUN_STATE_INMIGRATE) &&
-            !postcopy_ram_supported_by_host()) {
+            !postcopy_ram_supported_by_host(NULL)) {
             /* postcopy_ram_supported_by_host will have emitted a more
              * detailed message
              */
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a0489f6..4adab36 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -59,7 +59,7 @@  struct PostcopyDiscardState {
 #include <sys/eventfd.h>
 #include <linux/userfaultfd.h>
 
-static bool ufd_version_check(int ufd)
+static bool ufd_version_check(int ufd, MigrationIncomingState *mis)
 {
     struct uffdio_api api_struct;
     uint64_t ioctl_mask;
@@ -112,7 +112,7 @@  static int test_range_shared(const char *block_name, void *host_addr,
  * normally fine since if the postcopy succeeds it gets turned back on at the
  * end.
  */
-bool postcopy_ram_supported_by_host(void)
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
 {
     long pagesize = getpagesize();
     int ufd = -1;
@@ -135,7 +135,7 @@  bool postcopy_ram_supported_by_host(void)
     }
 
     /* Version and features check */
-    if (!ufd_version_check(ufd)) {
+    if (!ufd_version_check(ufd, mis)) {
         goto out;
     }
 
@@ -512,7 +512,7 @@  int postcopy_ram_enable_notify(MigrationIncomingState *mis)
      * Although the host check already tested the API, we need to
      * do the check again as an ABI handshake on the new fd.
      */
-    if (!ufd_version_check(mis->userfault_fd)) {
+    if (!ufd_version_check(mis->userfault_fd, mis)) {
         return -1;
     }
 
@@ -650,7 +650,7 @@  void *postcopy_get_tmp_page(MigrationIncomingState *mis)
 
 #else
 /* No target OS support, stubs just fail */
-bool postcopy_ram_supported_by_host(void)
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
 {
     error_report("%s: No OS support", __func__);
     return false;
diff --git a/migration/postcopy-ram.h b/migration/postcopy-ram.h
index 52d51e8..587a8b8 100644
--- a/migration/postcopy-ram.h
+++ b/migration/postcopy-ram.h
@@ -14,7 +14,7 @@ 
 #define QEMU_POSTCOPY_RAM_H
 
 /* Return true if the host supports everything we need to do postcopy-ram */
-bool postcopy_ram_supported_by_host(void);
+bool postcopy_ram_supported_by_host(MigrationIncomingState *mis);
 
 /*
  * Make all of RAM sensitive to accesses to areas that haven't yet been written
diff --git a/migration/savevm.c b/migration/savevm.c
index f5e8194..61a6df7 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1356,7 +1356,7 @@  static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis)
         return -1;
     }
 
-    if (!postcopy_ram_supported_by_host()) {
+    if (!postcopy_ram_supported_by_host(mis)) {
         postcopy_state_set(POSTCOPY_INCOMING_NONE);
         return -1;
     }