| Message ID | 1377069536-12658-5-git-send-email-lilei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Il 21/08/2013 09:18, Lei Li ha scritto: > Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate > limiting to QEMUFile, but set the return value for qemu_file_rate_limit > to 1 in the case of qemu_file_get_error. It is wrong and should be negative > compared to the original function buffered_rate_limit and the current logic > in ram_save_iterate. As qemu_file_rate_limit is called manually to determine > if it has to exit, add the definition of the meaning of the return values > as well. When there is an error it returns "time to stop" so that ultimately we get to the migration_thread function and check qemu_file_get_error there. Why doesn't this work for you? Paolo > Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> > --- > savevm.c | 14 ++++++++++++-- > 1 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/savevm.c b/savevm.c > index 68552a7..6362275 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f) > return f->pos; > } > > +/* > + * The meaning of the return values is: > + * 0: We can continue sending > + * 1: Time to stop > + * negative: There has been an error > + */ > + > int qemu_file_rate_limit(QEMUFile *f) > { > - if (qemu_file_get_error(f)) { > - return 1; > + int ret; > + > + ret = qemu_file_get_error(f); > + if (ret) { > + return ret; > } > if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) { > return 1; >
On 08/21/2013 06:42 PM, Paolo Bonzini wrote: > Il 21/08/2013 09:18, Lei Li ha scritto: >> Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate >> limiting to QEMUFile, but set the return value for qemu_file_rate_limit >> to 1 in the case of qemu_file_get_error. It is wrong and should be negative >> compared to the original function buffered_rate_limit and the current logic >> in ram_save_iterate. As qemu_file_rate_limit is called manually to determine >> if it has to exit, add the definition of the meaning of the return values >> as well. > When there is an error it returns "time to stop" so that ultimately we > get to the migration_thread function and check qemu_file_get_error there. > > Why doesn't this work for you? Hi Paolo, Say, In ram_save_iterate(), the current logic is: ret = qemu_file_rate_limit(); while(ret == 0) { save RAM blocks until no more to send. } if (ret < 0) { return ret; } ... And in savevm layer, qemu_savevm_state_iterate() set an error if the return value of ram_save_iterate < 0. Obviously the return value of qemu_file_rate_limit() should have an negative value for there has been an error. Otherwise we need to modify the logic above. > Paolo > >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> >> --- >> savevm.c | 14 ++++++++++++-- >> 1 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/savevm.c b/savevm.c >> index 68552a7..6362275 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f) >> return f->pos; >> } >> >> +/* >> + * The meaning of the return values is: >> + * 0: We can continue sending >> + * 1: Time to stop >> + * negative: There has been an error >> + */ >> + >> int qemu_file_rate_limit(QEMUFile *f) >> { >> - if (qemu_file_get_error(f)) { >> - return 1; >> + int ret; >> + >> + ret = qemu_file_get_error(f); >> + if (ret) { >> + return ret; >> } >> if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) { >> return 1; >> >
> Say, In ram_save_iterate(), the current logic is: > > ret = qemu_file_rate_limit(); > while(ret == 0) { > save RAM blocks until no more to send. > } > if (ret < 0) { > return ret; > } > ... > > And in savevm layer, qemu_savevm_state_iterate() set an error if the return > value of ram_save_iterate < 0. But that is to report errors *not in the QEMUFile*. Errors in the QEMUFile are already reported by qemu_file_get_error(), and qemu_savevm_state_iterate() will not overwrite them. qemu_file_rate_limit() returning 1 is enough to exit the loop, which is all that is needed. > Obviously the return value of qemu_file_rate_limit() should have an negative > value for there has been an error. Otherwise we need to modify the logic > above. It is not obvious to me... what is, again, the bug that you're observing? I think it is happening only because you're modifying the migration thread's body. If you use the normal code of the migration thread, it will just work. Paolo
On 08/23/2013 01:34 PM, Paolo Bonzini wrote: >> Say, In ram_save_iterate(), the current logic is: >> >> ret = qemu_file_rate_limit(); >> while(ret == 0) { >> save RAM blocks until no more to send. >> } >> if (ret < 0) { >> return ret; >> } >> ... >> >> And in savevm layer, qemu_savevm_state_iterate() set an error if the return >> value of ram_save_iterate < 0. > But that is to report errors *not in the QEMUFile*. Errors in the > QEMUFile are already reported by qemu_file_get_error(), and > qemu_savevm_state_iterate() will not overwrite them. Sorry, a little confusion. In qemu_savevm_state_iterate(), if the return of ram_save_iterate < 0, it will set error in QEMUFile by qemu_file_set_error(f, ret), the error reported by qemu_file_get_error() in the QEMUFile is got from qemu_file_set_error() by reading the f->last_error, right? And now as qemu_file_rate_limit() never return negative value, what's the meaning for the check: if (qemu_file_rate_limit(f) < 0) in ram_save_iterate()? > > qemu_file_rate_limit() returning 1 is enough to exit the loop, > which is all that is needed. >> Obviously the return value of qemu_file_rate_limit() should have an negative >> value for there has been an error. Otherwise we need to modify the logic >> above. > It is not obvious to me... what is, again, the bug that you're observing? > I think it is happening only because you're modifying the migration thread's > body. If you use the normal code of the migration thread, it will just work. > > Paolo >
Il 23/08/2013 11:11, Lei Li ha scritto: > > In qemu_savevm_state_iterate(), if the return of ram_save_iterate < 0, > it will set error in QEMUFile by qemu_file_set_error(f, ret), the error > reported by qemu_file_get_error() in the QEMUFile is got from > qemu_file_set_error() by reading the f->last_error, right? > > And now as qemu_file_rate_limit() never return negative value, what's the > meaning for the check: if (qemu_file_rate_limit(f) < 0) in > ram_save_iterate()? I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no less-than-zero check. Are we looking at the same code? :) Paolo
On 08/23/2013 05:14 PM, Paolo Bonzini wrote: > Il 23/08/2013 11:11, Lei Li ha scritto: >> In qemu_savevm_state_iterate(), if the return of ram_save_iterate < 0, >> it will set error in QEMUFile by qemu_file_set_error(f, ret), the error >> reported by qemu_file_get_error() in the QEMUFile is got from >> qemu_file_set_error() by reading the f->last_error, right? >> >> And now as qemu_file_rate_limit() never return negative value, what's the >> meaning for the check: if (qemu_file_rate_limit(f) < 0) in >> ram_save_iterate()? > I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no > less-than-zero check. > > Are we looking at the same code? :) I think so, hehe. You might want to look a little more. After the while(..), there is a check: if (ret < 0) { bytes_transferred += total_sent; return ret; } > > Paolo >
Il 23/08/2013 11:18, Lei Li ha scritto: >>> >>> And now as qemu_file_rate_limit() never return negative value, what's >>> the >>> meaning for the check: if (qemu_file_rate_limit(f) < 0) in >>> ram_save_iterate()? >> I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no >> less-than-zero check. >> >> Are we looking at the same code? :) > > I think so, hehe. > You might want to look a little more. After the while(..), there is a > check: > > if (ret < 0) { > bytes_transferred += total_sent; > return ret; > } Aha, there you are. It's dead code, you can delete it. Paolo
On 08/23/2013 05:22 PM, Paolo Bonzini wrote: > Il 23/08/2013 11:18, Lei Li ha scritto: >>>> And now as qemu_file_rate_limit() never return negative value, what's >>>> the >>>> meaning for the check: if (qemu_file_rate_limit(f) < 0) in >>>> ram_save_iterate()? >>> I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no >>> less-than-zero check. >>> >>> Are we looking at the same code? :) >> I think so, hehe. >> You might want to look a little more. After the while(..), there is a >> check: >> >> if (ret < 0) { >> bytes_transferred += total_sent; >> return ret; >> } > Aha, there you are. It's dead code, you can delete it. Sure, I will send it along with other two patches you reviewed positively. :) > > Paolo >
diff --git a/savevm.c b/savevm.c index 68552a7..6362275 100644 --- a/savevm.c +++ b/savevm.c @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f) return f->pos; } +/* + * The meaning of the return values is: + * 0: We can continue sending + * 1: Time to stop + * negative: There has been an error + */ + int qemu_file_rate_limit(QEMUFile *f) { - if (qemu_file_get_error(f)) { - return 1; + int ret; + + ret = qemu_file_get_error(f); + if (ret) { + return ret; } if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) { return 1;
Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate limiting to QEMUFile, but set the return value for qemu_file_rate_limit to 1 in the case of qemu_file_get_error. It is wrong and should be negative compared to the original function buffered_rate_limit and the current logic in ram_save_iterate. As qemu_file_rate_limit is called manually to determine if it has to exit, add the definition of the meaning of the return values as well. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- savevm.c | 14 ++++++++++++-- 1 files changed, 12 insertions(+), 2 deletions(-)