| Message ID | 1392726818-14606-1-git-send-email-vincent.kherbache@inria.fr |
|---|---|
| State | New |
| Headers | show |
On 02/18/2014 05:33 AM, Vincent KHERBACHE wrote: > The test (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) is wrong because > the condition is valid when enable = 0 and current dirty log memory flag is set. > As a consequence kvm_log_global_stop() does not stop the KVM dirty log > tracking: kvm_set_migration_log(0) didn't do its job. > So instead I propose to use kvm_slot_dirty_pages_log_change() which correctly > compare the memory flags (old/new). > > Signed-off-by: Vincent KHERBACHE <vincent.kherbache@inria.fr> > --- > kvm-all.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > - return err; > - } > + err = kvm_slot_dirty_pages_log_change(mem, (bool)enable); Is this a v2 post? Any reason you reposted without addressing my earlier review? https://lists.gnu.org/archive/html/qemu-devel/2014-02/msg02840.html
Il 18/02/2014 13:33, Vincent KHERBACHE ha scritto: > diff --git a/kvm-all.c b/kvm-all.c > index 2ca9143..f104f87 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -355,7 +355,7 @@ static int kvm_set_migration_log(int enable) > { > KVMState *s = kvm_state; > KVMSlot *mem; > - int i, err; > + int i, err = 0; > > s->migration_log = enable; > > @@ -365,15 +365,9 @@ static int kvm_set_migration_log(int enable) > if (!mem->memory_size) { > continue; > } > - if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) { > - continue; > - } > - err = kvm_set_user_memory_region(s, mem); > - if (err) { > - return err; > - } > + err = kvm_slot_dirty_pages_log_change(mem, (bool)enable); > } > - return 0; > + return err; > } This patch is not correct, because it introduces another one: if dirty logging was enabled on the memory region (kvm_dirty_pages_log_change), it will be disabled after migration. Can you explain better the problem? I cannot see anything wrong in the code, though it is not the clearest. Note that the effects of kvm_set_migration_log and kvm_dirty_pages_log_change should be logically ORed. Paolo
Le 18/02/2014 17:16, Paolo Bonzini a écrit : > This patch is not correct, because it introduces another one: if dirty > logging was enabled on the memory region (kvm_dirty_pages_log_change), > it will be disabled after migration. Yes you're right that was my initial goal, I didn't distinguish the two cases. > Note that the effects of > kvm_set_migration_log and kvm_dirty_pages_log_change should be logically > ORed. OK, but that's not really the case (see next comment) > Can you explain better the problem? I cannot see anything wrong in the > code, though it is not the clearest. The problem is that if dirty logging is enabled by kvm_set_migration_log(1) it will NOT be disabled by a kvm_set_migration_log(0). So, if this behavior is 'normal', kvm_log_global_stop (which call kvm_set_migration_log) can not be used independently ? Also, what I don't understand is why kvm_log_global_start/stop and kvm_log_start/stop are so different: kvm_set_migration_log VS kvm_dirty_pages_log_change. Thank you for the support. Regards,
diff --git a/kvm-all.c b/kvm-all.c index 2ca9143..f104f87 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -355,7 +355,7 @@ static int kvm_set_migration_log(int enable) { KVMState *s = kvm_state; KVMSlot *mem; - int i, err; + int i, err = 0; s->migration_log = enable; @@ -365,15 +365,9 @@ static int kvm_set_migration_log(int enable) if (!mem->memory_size) { continue; } - if (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) { - continue; - } - err = kvm_set_user_memory_region(s, mem); - if (err) { - return err; - } + err = kvm_slot_dirty_pages_log_change(mem, (bool)enable); } - return 0; + return err; } /* get kvm's dirty pages bitmap and update qemu's */
The test (!!(mem->flags & KVM_MEM_LOG_DIRTY_PAGES) == enable) is wrong because the condition is valid when enable = 0 and current dirty log memory flag is set. As a consequence kvm_log_global_stop() does not stop the KVM dirty log tracking: kvm_set_migration_log(0) didn't do its job. So instead I propose to use kvm_slot_dirty_pages_log_change() which correctly compare the memory flags (old/new). Signed-off-by: Vincent KHERBACHE <vincent.kherbache@inria.fr> --- kvm-all.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)