[Qemu-devel] [PATCH 0/3] migration/savevm: move non SaveStateEntry condition check out of iteration

[Qemu-devel] [PATCH 0/3] migration/savevm: move non SaveStateEntry condition check out of iteration

[Wei Yang]

qemu_savevm_state_complete_precopy() iterates SaveStateEntry and does proper
tasks for migration.

For each iteration, in_postcopy and iterable_only would be checked to see
whether it should skip. Since these two conditions are not SaveStateEntry
specific, it is proper to move the check out of iteration.

These three patches prepare the code and move the condition check out of the
iteration.


Wei Yang (3):
  migration/savevm: flush file for iterable_only case
  migration/savevm: split qemu_savevm_state_complete_precopy() into two
    parts
  migration/savevm: move non SaveStateEntry condition check out of
    iteration

 migration/savevm.c | 68 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 18 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/3] migration/savevm: flush file for iterable_only case

[Wei Yang]

It would be proper to flush file even for iterable_only case.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/savevm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index c0e557b4c2..becedcc1c6 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1292,7 +1292,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
     }
 
     if (iterable_only) {
-        return 0;
+        goto flush;
     }
 
     vmdesc = qjson_new();
@@ -1353,6 +1353,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
     }
     qjson_destroy(vmdesc);
 
+flush:
     qemu_fflush(f);
     return 0;
 }
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/3] migration/savevm: flush file for iterable_only case

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> It would be proper to flush file even for iterable_only case.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

OK, I don't think this is actually necessary; but it's safe.
We only really need the flush at the end of the file, and in the
non-iterable-only case it's not the end of the file.

Since it makes your next change simpler,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c0e557b4c2..becedcc1c6 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1292,7 +1292,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>      }
>  
>      if (iterable_only) {
> -        return 0;
> +        goto flush;
>      }
>  
>      vmdesc = qjson_new();
> @@ -1353,6 +1353,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>      }
>      qjson_destroy(vmdesc);
>  
> +flush:
>      qemu_fflush(f);
>      return 0;
>  }
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] migration/savevm: flush file for iterable_only case

[Wei Yang]

On Fri, Jul 19, 2019 at 05:47:59PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> It would be proper to flush file even for iterable_only case.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>OK, I don't think this is actually necessary; but it's safe.
>We only really need the flush at the end of the file, and in the
>non-iterable-only case it's not the end of the file.
>
>Since it makes your next change simpler,

Yep, you are so kind.

>
>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> ---
>>  migration/savevm.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index c0e557b4c2..becedcc1c6 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1292,7 +1292,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>>      }
>>  
>>      if (iterable_only) {
>> -        return 0;
>> +        goto flush;
>>      }
>>  
>>      vmdesc = qjson_new();
>> @@ -1353,6 +1353,7 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>>      }
>>      qjson_destroy(vmdesc);
>>  
>> +flush:
>>      qemu_fflush(f);
>>      return 0;
>>  }
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

[Qemu-devel] [PATCH 2/3] migration/savevm: split qemu_savevm_state_complete_precopy() into two parts

[Wei Yang]

This is a preparation patch for further cleanup.

No functional change, just wrap two major part of
qemu_savevm_state_complete_precopy() into function.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/savevm.c | 66 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index becedcc1c6..c41e13e322 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1246,23 +1246,12 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
     qemu_fflush(f);
 }
 
-int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
-                                       bool inactivate_disks)
+static
+int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
+                                                bool iterable_only)
 {
-    QJSON *vmdesc;
-    int vmdesc_len;
     SaveStateEntry *se;
     int ret;
-    bool in_postcopy = migration_in_postcopy();
-    Error *local_err = NULL;
-
-    if (precopy_notify(PRECOPY_NOTIFY_COMPLETE, &local_err)) {
-        error_report_err(local_err);
-    }
-
-    trace_savevm_state_complete_precopy();
-
-    cpu_synchronize_all_states();
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
         if (!se->ops ||
@@ -1291,9 +1280,18 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
         }
     }
 
-    if (iterable_only) {
-        goto flush;
-    }
+    return 0;
+}
+
+static
+int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
+                                                    bool in_postcopy,
+                                                    bool inactivate_disks)
+{
+    QJSON *vmdesc;
+    int vmdesc_len;
+    SaveStateEntry *se;
+    int ret;
 
     vmdesc = qjson_new();
     json_prop_int(vmdesc, "page_size", qemu_target_page_size());
@@ -1353,6 +1351,40 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
     }
     qjson_destroy(vmdesc);
 
+    return 0;
+}
+
+int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
+                                       bool inactivate_disks)
+{
+    int ret;
+    Error *local_err = NULL;
+    bool in_postcopy = migration_in_postcopy();
+
+    if (precopy_notify(PRECOPY_NOTIFY_COMPLETE, &local_err)) {
+        error_report_err(local_err);
+    }
+
+    trace_savevm_state_complete_precopy();
+
+    cpu_synchronize_all_states();
+
+    ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy,
+                                                      iterable_only);
+    if (ret) {
+        return ret;
+    }
+
+    if (iterable_only) {
+        goto flush;
+    }
+
+    ret = qemu_savevm_state_complete_precopy_non_iterable(f, in_postcopy,
+                                                          inactivate_disks);
+    if (ret) {
+        return ret;
+    }
+
 flush:
     qemu_fflush(f);
     return 0;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 2/3] migration/savevm: split qemu_savevm_state_complete_precopy() into two parts

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> This is a preparation patch for further cleanup.
> 
> No functional change, just wrap two major part of
> qemu_savevm_state_complete_precopy() into function.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 66 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index becedcc1c6..c41e13e322 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1246,23 +1246,12 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>      qemu_fflush(f);
>  }
>  
> -int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
> -                                       bool inactivate_disks)
> +static
> +int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
> +                                                bool iterable_only)
>  {
> -    QJSON *vmdesc;
> -    int vmdesc_len;
>      SaveStateEntry *se;
>      int ret;
> -    bool in_postcopy = migration_in_postcopy();
> -    Error *local_err = NULL;
> -
> -    if (precopy_notify(PRECOPY_NOTIFY_COMPLETE, &local_err)) {
> -        error_report_err(local_err);
> -    }
> -
> -    trace_savevm_state_complete_precopy();
> -
> -    cpu_synchronize_all_states();
>  
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops ||
> @@ -1291,9 +1280,18 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>          }
>      }
>  
> -    if (iterable_only) {
> -        goto flush;
> -    }
> +    return 0;
> +}
> +
> +static
> +int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> +                                                    bool in_postcopy,
> +                                                    bool inactivate_disks)
> +{
> +    QJSON *vmdesc;
> +    int vmdesc_len;
> +    SaveStateEntry *se;
> +    int ret;
>  
>      vmdesc = qjson_new();
>      json_prop_int(vmdesc, "page_size", qemu_target_page_size());
> @@ -1353,6 +1351,40 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>      }
>      qjson_destroy(vmdesc);
>  
> +    return 0;
> +}
> +
> +int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
> +                                       bool inactivate_disks)
> +{
> +    int ret;
> +    Error *local_err = NULL;
> +    bool in_postcopy = migration_in_postcopy();
> +
> +    if (precopy_notify(PRECOPY_NOTIFY_COMPLETE, &local_err)) {
> +        error_report_err(local_err);
> +    }
> +
> +    trace_savevm_state_complete_precopy();
> +
> +    cpu_synchronize_all_states();
> +
> +    ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy,
> +                                                      iterable_only);
> +    if (ret) {
> +        return ret;
> +    }
> +
> +    if (iterable_only) {
> +        goto flush;
> +    }
> +
> +    ret = qemu_savevm_state_complete_precopy_non_iterable(f, in_postcopy,
> +                                                          inactivate_disks);
> +    if (ret) {
> +        return ret;
> +    }
> +
>  flush:
>      qemu_fflush(f);
>      return 0;
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 3/3] migration/savevm: move non SaveStateEntry condition check out of iteration

[Wei Yang]

in_postcopy and iterable_only are not SaveStateEntry specific, it would
be more proper to check them out of iteration.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/savevm.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index c41e13e322..8a2ada529e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1247,8 +1247,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
 }
 
 static
-int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
-                                                bool iterable_only)
+int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy)
 {
     SaveStateEntry *se;
     int ret;
@@ -1257,7 +1256,6 @@ int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
         if (!se->ops ||
             (in_postcopy && se->ops->has_postcopy &&
              se->ops->has_postcopy(se->opaque)) ||
-            (in_postcopy && !iterable_only) ||
             !se->ops->save_live_complete_precopy) {
             continue;
         }
@@ -1369,10 +1367,11 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
 
     cpu_synchronize_all_states();
 
-    ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy,
-                                                      iterable_only);
-    if (ret) {
-        return ret;
+    if (!in_postcopy || iterable_only) {
+        ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy);
+        if (ret) {
+            return ret;
+        }
     }
 
     if (iterable_only) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 3/3] migration/savevm: move non SaveStateEntry condition check out of iteration

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> in_postcopy and iterable_only are not SaveStateEntry specific, it would
> be more proper to check them out of iteration.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Worth it just to make that big if statement simpler!


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/savevm.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c41e13e322..8a2ada529e 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1247,8 +1247,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>  }
>  
>  static
> -int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
> -                                                bool iterable_only)
> +int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy)
>  {
>      SaveStateEntry *se;
>      int ret;
> @@ -1257,7 +1256,6 @@ int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
>          if (!se->ops ||
>              (in_postcopy && se->ops->has_postcopy &&
>               se->ops->has_postcopy(se->opaque)) ||
> -            (in_postcopy && !iterable_only) ||
>              !se->ops->save_live_complete_precopy) {
>              continue;
>          }
> @@ -1369,10 +1367,11 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>  
>      cpu_synchronize_all_states();
>  
> -    ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy,
> -                                                      iterable_only);
> -    if (ret) {
> -        return ret;
> +    if (!in_postcopy || iterable_only) {
> +        ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy);
> +        if (ret) {
> +            return ret;
> +        }
>      }
>  
>      if (iterable_only) {
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] migration/savevm: move non SaveStateEntry condition check out of iteration

[Wei Yang]

On Fri, Jul 19, 2019 at 05:59:50PM +0100, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> in_postcopy and iterable_only are not SaveStateEntry specific, it would
>> be more proper to check them out of iteration.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>Worth it just to make that big if statement simpler!
>

Yep, the original one looks a little hard to understand.

Thanks for your comment.

>
>Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
>> ---
>>  migration/savevm.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>> 
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index c41e13e322..8a2ada529e 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1247,8 +1247,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
>>  }
>>  
>>  static
>> -int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
>> -                                                bool iterable_only)
>> +int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy)
>>  {
>>      SaveStateEntry *se;
>>      int ret;
>> @@ -1257,7 +1256,6 @@ int qemu_savevm_state_complete_precopy_iterable(QEMUFile *f, bool in_postcopy,
>>          if (!se->ops ||
>>              (in_postcopy && se->ops->has_postcopy &&
>>               se->ops->has_postcopy(se->opaque)) ||
>> -            (in_postcopy && !iterable_only) ||
>>              !se->ops->save_live_complete_precopy) {
>>              continue;
>>          }
>> @@ -1369,10 +1367,11 @@ int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
>>  
>>      cpu_synchronize_all_states();
>>  
>> -    ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy,
>> -                                                      iterable_only);
>> -    if (ret) {
>> -        return ret;
>> +    if (!in_postcopy || iterable_only) {
>> +        ret = qemu_savevm_state_complete_precopy_iterable(f, in_postcopy);
>> +        if (ret) {
>> +            return ret;
>> +        }
>>      }
>>  
>>      if (iterable_only) {
>> -- 
>> 2.17.1
>> 
>--
>Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH 0/3] migration/savevm: move non SaveStateEntry condition check out of iteration

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> qemu_savevm_state_complete_precopy() iterates SaveStateEntry and does proper
> tasks for migration.
> 
> For each iteration, in_postcopy and iterable_only would be checked to see
> whether it should skip. Since these two conditions are not SaveStateEntry
> specific, it is proper to move the check out of iteration.
> 
> These three patches prepare the code and move the condition check out of the
> iteration.
> 

Queued

> 
> Wei Yang (3):
>   migration/savevm: flush file for iterable_only case
>   migration/savevm: split qemu_savevm_state_complete_precopy() into two
>     parts
>   migration/savevm: move non SaveStateEntry condition check out of
>     iteration
> 
>  migration/savevm.c | 68 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 18 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK