[Qemu-devel] [PATCH 0/3 resend v2] Migration fix

[Qemu-devel] [PATCH 0/3 resend v2] Migration fix

[Lei Li]

This small patch series is extracted from localhost migration 
series that Paolo reviewed positively, send it separately as 
his request. 

It fixs wrong initialization in RDMA hook ram_control_load_hook() 
and gets rid of the negative check for qemu_file_rate_limit() with 
bytes_transferred and return value adjusted correspondingly in 
ram_save_iterate.


Lei Li (3):
  savevm: add comments for qemu_file_get_error()
  savevm: fix wrong initialization by ram_control_load_hook
  arch_init: right return for ram_save_iterate

 arch_init.c |   15 ++++++++++-----
 savevm.c    |    9 ++++++++-
 2 files changed, 18 insertions(+), 6 deletions(-)

[Qemu-devel] [PATCH 1/3 resend v2] savevm: add comments for qemu_file_get_error()

[Lei Li]

Add comments for qemu_file_get_error(), as its return value
is not very clear.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 savevm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 03fc4d9..95a11f9 100644
--- a/savevm.c
+++ b/savevm.c
@@ -566,6 +566,13 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
     return f;
 }
 
+/*
+ * Get last error for stream f
+ *
+ * Return negative error value if there has been an error on previous
+ * operations, return 0 if no error happened.
+ *
+ */
 int qemu_file_get_error(QEMUFile *f)
 {
     return f->last_error;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 1/3 resend v2] savevm: add comments for qemu_file_get_error()

[Juan Quintela]

Lei Li <lilei@linux.vnet.ibm.com> wrote:
> Add comments for qemu_file_get_error(), as its return value
> is not very clear.
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>        

[Qemu-devel] [PATCH 2/3 resend v2] savevm: fix wrong initialization by ram_control_load_hook

[Lei Li]

It should set negative error value rather than 0 in QEMUFile
if there has been an error.

Reviewed-by: Michael R. Hines <mrhines@us.ibm.com>
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---

Change since v1:
  Initialize ret to -EINVAL rather than -1 from Paolo Bonzini.

 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 95a11f9..a0be109 100644
--- a/savevm.c
+++ b/savevm.c
@@ -649,7 +649,7 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
 
 void ram_control_load_hook(QEMUFile *f, uint64_t flags)
 {
-    int ret = 0;
+    int ret = -EINVAL;
 
     if (f->ops->hook_ram_load) {
         ret = f->ops->hook_ram_load(f, f->opaque, flags);
-- 
1.7.7.6

[Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Lei Li]

qemu_file_rate_limit() never return negative value since the refactor
by Commit 1964a39, this patch gets rid of the negative check for it,
adjust bytes_transferred and return value correspondingly in 
ram_save_iterate().

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---

Change since v1:
  Return fixes and improvement from Paolo Bonzini.
  
 arch_init.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 94d45e1..a26bc89 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -709,15 +709,20 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
      */
     ram_control_after_iterate(f, RAM_CONTROL_ROUND);
 
+    bytes_transferred += total_sent;
+
+    /*
+     * Do not count these 8 bytes into total_sent, so that we can
+     * return 0 if no page had been dirtied.
+     */
+    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
+    bytes_transferred += 8;
+
+    ret = qemu_file_get_error(f);
     if (ret < 0) {
-        bytes_transferred += total_sent;
         return ret;
     }
 
-    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
-    total_sent += 8;
-    bytes_transferred += total_sent;
-
     return total_sent;
 }
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Juan Quintela]

Lei Li <lilei@linux.vnet.ibm.com> wrote:
> qemu_file_rate_limit() never return negative value since the refactor
> by Commit 1964a39, this patch gets rid of the negative check for it,
> adjust bytes_transferred and return value correspondingly in 
> ram_save_iterate().
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>
> Change since v1:
>   Return fixes and improvement from Paolo Bonzini.
>   
>  arch_init.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 94d45e1..a26bc89 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -709,15 +709,20 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>       */
>      ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>  
> +    bytes_transferred += total_sent;

Agreed.

> +
> +    /*
> +     * Do not count these 8 bytes into total_sent, so that we can
> +     * return 0 if no page had been dirtied.
> +     */
> +    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
> +    bytes_transferred += 8;
> +
> +    ret = qemu_file_get_error(f);
>      if (ret < 0) {

Not sure this is the right solution.

We are sending anyways RAM_SAVE_FLAG_EOS.

And I think that the right solution is make qemu_get_rate_limit() to
return -1 in case of error (or the error, I don't care).  Looking at the
callers:


migration.c::migration_thread()

we check for error when we qemu_file_rate_limit() returns != 0.  Well.

second call:

        if (qemu_file_rate_limit(s->file)) {
            /* usleep expects microseconds */
            g_usleep((initial_time + BUFFER_DELAY - current_time)*1000);
        }


if should be:

    if (qemu_file_rate_limit(s->file) == 1)

block_migration: not correct, we don't check for the error.

arch_init.c:

   check is correct, but we need to return -ERROR in case of errors.

hw/ppc/spapr.c:
   will work correctly even if changed to -ERROR.


savevm.c: qemu_savevm_state_iterate()


        if (qemu_file_rate_limit(f)) {
            return 0;
        }


check is incorrect again, we should return an error if there is one
error.


I think that returning qemu_rate_limit() to return 0/1/negative makes sense.

Thoughts?

Thanks, Juan.

Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Paolo Bonzini]

Il 11/09/2013 11:17, Juan Quintela ha scritto:
> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>> qemu_file_rate_limit() never return negative value since the refactor
>> by Commit 1964a39, this patch gets rid of the negative check for it,
>> adjust bytes_transferred and return value correspondingly in 
>> ram_save_iterate().
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>
>> Change since v1:
>>   Return fixes and improvement from Paolo Bonzini.
>>   
>>  arch_init.c |   15 ++++++++++-----
>>  1 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 94d45e1..a26bc89 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -709,15 +709,20 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>       */
>>      ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>>  
>> +    bytes_transferred += total_sent;
> 
> Agreed.
> 
>> +
>> +    /*
>> +     * Do not count these 8 bytes into total_sent, so that we can
>> +     * return 0 if no page had been dirtied.
>> +     */
>> +    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>> +    bytes_transferred += 8;
>> +
>> +    ret = qemu_file_get_error(f);
>>      if (ret < 0) {
> 
> Not sure this is the right solution.
> 
> We are sending anyways RAM_SAVE_FLAG_EOS.

If there is an error, the qemu_put_be64 will do nothing.  It is part of
the design of QEMUFile that you can keep sending stuff to it after an
error happened.

> And I think that the right solution is make qemu_get_rate_limit() to
> return -1 in case of error (or the error, I don't care).

You might do both things, it would avoid the useless g_usleep you
pointed out below.  But Lei's patch is good, because an error could
happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS.

> savevm.c: qemu_savevm_state_iterate()
> 
>         if (qemu_file_rate_limit(f)) {
>             return 0;
>         }
> 
> check is incorrect again, we should return an error if there is one
> error.

Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so
changing qemu_savevm_state_iterate to only return 0/1 would make sense too.

Paolo


> 
> I think that returning qemu_rate_limit() to return 0/1/negative makes sense.
> 
> Thoughts?
> 
> Thanks, Juan.
> 

Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/09/2013 11:17, Juan Quintela ha scritto:
>> Lei Li <lilei@linux.vnet.ibm.com> wrote:
>>> qemu_file_rate_limit() never return negative value since the refactor
>>> by Commit 1964a39, this patch gets rid of the negative check for it,
>>> adjust bytes_transferred and return value correspondingly in 
>>> ram_save_iterate().
>>>
>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>
>>> Change since v1:
>>>   Return fixes and improvement from Paolo Bonzini.
>>>   
>>>  arch_init.c |   15 ++++++++++-----
>>>  1 files changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 94d45e1..a26bc89 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -709,15 +709,20 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>>       */
>>>      ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>>>  
>>> +    bytes_transferred += total_sent;
>> 
>> Agreed.
>> 
>>> +
>>> +    /*
>>> +     * Do not count these 8 bytes into total_sent, so that we can
>>> +     * return 0 if no page had been dirtied.
>>> +     */
>>> +    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>>> +    bytes_transferred += 8;
>>> +
>>> +    ret = qemu_file_get_error(f);
>>>      if (ret < 0) {
>> 
>> Not sure this is the right solution.
>> 
>> We are sending anyways RAM_SAVE_FLAG_EOS.
>
> If there is an error, the qemu_put_be64 will do nothing.  It is part of
> the design of QEMUFile that you can keep sending stuff to it after an
> error happened.
>
>> And I think that the right solution is make qemu_get_rate_limit() to
>> return -1 in case of error (or the error, I don't care).
>
> You might do both things, it would avoid the useless g_usleep you
> pointed out below.  But Lei's patch is good, because an error could
> happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS.


Caller checks also.  This is the reason I wanted qemu_file_* callos to
return an error.  It has some advantages and some disadvantages.  We
don't agree on which ones are bigger O:-)

>
>> savevm.c: qemu_savevm_state_iterate()
>> 
>>         if (qemu_file_rate_limit(f)) {
>>             return 0;
>>         }
>> 
>> check is incorrect again, we should return an error if there is one
>> error.
>
> Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so
> changing qemu_savevm_state_iterate to only return 0/1 would make sense too.

In this case, 0 means:
  please, call us again
when what we mean is:
  don't care about calling us again, there is an error.  Handle the error.

Notice that qemu_save_iterate() already returns errors in other code
paths, not there because it don't know, code should be:

ret = qemu_file_rate_limit(f))

if (ret == 1) {
   return 0;
} else if (ret < 0) {
   return ret;
}

If we change th ereturn value for qemu_file_rate_limit() the change that
cames with this patch is not needed, that was my point.

>
> Paolo
>
>
>> 
>> I think that returning qemu_rate_limit() to return 0/1/negative makes sense.
>> 
>> Thoughts?
>> 
>> Thanks, Juan.
>> 

Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Paolo Bonzini]

Il 11/09/2013 13:06, Juan Quintela ha scritto:
>>> And I think that the right solution is make qemu_get_rate_limit() to
>>> return -1 in case of error (or the error, I don't care).
>>
>> You might do both things, it would avoid the useless g_usleep you
>> pointed out below.  But Lei's patch is good, because an error could
>> happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS.
> 
> Caller checks also.  This is the reason I wanted qemu_file_* callers to
> return an error.  It has some advantages and some disadvantages.  We
> don't agree on which ones are bigger O:-)

I think the disadvantages are bigger.  It litters the code with error
handling, hides where things actually happen, and doesn't even simplify
QEMUFile itself.  Checking only at the toplevel is simpler, all we need
to do is ensure that we get there every now and then (and that's what
qemu_file_rate_limit does).

>>> savevm.c: qemu_savevm_state_iterate()
>>>
>>>         if (qemu_file_rate_limit(f)) {
>>>             return 0;
>>>         }
>>>
>>> check is incorrect again, we should return an error if there is one
>>> error.
>>
>> Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so
>> changing qemu_savevm_state_iterate to only return 0/1 would make sense too.
> 
> In this case, 0 means:
>   please, call us again
> when what we mean is:
>   don't care about calling us again, there is an error.  Handle the error.

Or alternatively, 0 means:

   we haven't finished the work

when what we mean is:

   we haven't finished the work (BTW, please check if there is an error)

> Notice that qemu_save_iterate() already returns errors in other code
> paths

Yes that's also unnecessary.

> If we change th ereturn value for qemu_file_rate_limit() the change that
> cames with this patch is not needed, that was my point.

This is what an earlier patch from Lei did.  I told him (or her?) to
leave qemu_file_rate_limit aside since the idea behind QEMUFile is to
only handle the error at the top.

Paolo

Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Lei Li]

On 09/11/2013 07:27 PM, Paolo Bonzini wrote:
> Il 11/09/2013 13:06, Juan Quintela ha scritto:
>>>> And I think that the right solution is make qemu_get_rate_limit() to
>>>> return -1 in case of error (or the error, I don't care).
>>> You might do both things, it would avoid the useless g_usleep you
>>> pointed out below.  But Lei's patch is good, because an error could
>>> happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS.
>> Caller checks also.  This is the reason I wanted qemu_file_* callers to
>> return an error.  It has some advantages and some disadvantages.  We
>> don't agree on which ones are bigger O:-)
> I think the disadvantages are bigger.  It litters the code with error
> handling, hides where things actually happen, and doesn't even simplify
> QEMUFile itself.  Checking only at the toplevel is simpler, all we need
> to do is ensure that we get there every now and then (and that's what
> qemu_file_rate_limit does).
>
>>>> savevm.c: qemu_savevm_state_iterate()
>>>>
>>>>          if (qemu_file_rate_limit(f)) {
>>>>              return 0;
>>>>          }
>>>>
>>>> check is incorrect again, we should return an error if there is one
>>>> error.
>>> Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so
>>> changing qemu_savevm_state_iterate to only return 0/1 would make sense too.
>> In this case, 0 means:
>>    please, call us again
>> when what we mean is:
>>    don't care about calling us again, there is an error.  Handle the error.
> Or alternatively, 0 means:
>
>     we haven't finished the work
>
> when what we mean is:
>
>     we haven't finished the work (BTW, please check if there is an error)
>
>> Notice that qemu_save_iterate() already returns errors in other code
>> paths
> Yes that's also unnecessary.
>
>> If we change th ereturn value for qemu_file_rate_limit() the change that
>> cames with this patch is not needed, that was my point.
> This is what an earlier patch from Lei did.  I told him (or her?) to

It's her. :)

> leave qemu_file_rate_limit aside since the idea behind QEMUFile is to
> only handle the error at the top.

Yes, I changed the return value for qemu_file_rate_limit() to negative for
that if there has been an error at the beginning. After the discussion with
Paolo, I think he's suggestion based on the idea that only handle error at
the top behind QEMUFile makes sense to me, so that's where this patch come
from.

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Orit Wasserman]

On 09/11/2013 02:27 PM, Paolo Bonzini wrote:
> Il 11/09/2013 13:06, Juan Quintela ha scritto:
>>>> And I think that the right solution is make qemu_get_rate_limit() to
>>>> return -1 in case of error (or the error, I don't care).
>>>
>>> You might do both things, it would avoid the useless g_usleep you
>>> pointed out below.  But Lei's patch is good, because an error could
>>> happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS.
>>
>> Caller checks also.  This is the reason I wanted qemu_file_* callers to
>> return an error.  It has some advantages and some disadvantages.  We
>> don't agree on which ones are bigger O:-)
> 
> I think the disadvantages are bigger.  It litters the code with error
> handling, hides where things actually happen, and doesn't even simplify
> QEMUFile itself.  Checking only at the toplevel is simpler, all we need
> to do is ensure that we get there every now and then (and that's what
> qemu_file_rate_limit does).
> 

I also prefer the error checking at the top level.

Orit

>>>> savevm.c: qemu_savevm_state_iterate()
>>>>
>>>>         if (qemu_file_rate_limit(f)) {
>>>>             return 0;
>>>>         }
>>>>
>>>> check is incorrect again, we should return an error if there is one
>>>> error.
>>>
>>> Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so
>>> changing qemu_savevm_state_iterate to only return 0/1 would make sense too.
>>
>> In this case, 0 means:
>>   please, call us again
>> when what we mean is:
>>   don't care about calling us again, there is an error.  Handle the error.
> 
> Or alternatively, 0 means:
> 
>    we haven't finished the work
> 
> when what we mean is:
> 
>    we haven't finished the work (BTW, please check if there is an error)
> 
>> Notice that qemu_save_iterate() already returns errors in other code
>> paths
> 
> Yes that's also unnecessary.
> 
>> If we change th ereturn value for qemu_file_rate_limit() the change that
>> cames with this patch is not needed, that was my point.
> 
> This is what an earlier patch from Lei did.  I told him (or her?) to
> leave qemu_file_rate_limit aside since the idea behind QEMUFile is to
> only handle the error at the top.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 0/3 resend v2] Migration fix

[Orit Wasserman]

On 09/04/2013 12:02 PM, Lei Li wrote:
> This small patch series is extracted from localhost migration 
> series that Paolo reviewed positively, send it separately as 
> his request. 
> 
> It fixs wrong initialization in RDMA hook ram_control_load_hook() 
> and gets rid of the negative check for qemu_file_rate_limit() with 
> bytes_transferred and return value adjusted correspondingly in 
> ram_save_iterate.
> 
> 
> Lei Li (3):
>   savevm: add comments for qemu_file_get_error()
>   savevm: fix wrong initialization by ram_control_load_hook
>   arch_init: right return for ram_save_iterate
> 
>  arch_init.c |   15 ++++++++++-----
>  savevm.c    |    9 ++++++++-
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> 

Looks good,

Reviewed-by: Orit Wasserman <owasserm@redhat.com>