memory: Fix incorrect calls of log_global_start/stop

[PATCH] memory: Fix incorrect calls of log_global_start/stop

Posted by Peter Xu 2 years, 4 months ago

We should only call the log_global_start/stop when the global dirty track
bitmask changes from zero<->non-zero.

No real issue reported for this yet probably because no immediate user to
enable both dirty rate measurement and migration at the same time.  However
it'll be good to be prepared for it.

Fixes: 63b41db4bc ("memory: make global_dirty_tracking a bitmask")
Cc: Hyman Huang <huangy81@chinatelecom.cn>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/memory.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7340e19ff5..81d4bf1454 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2773,6 +2773,8 @@ static VMChangeStateEntry *vmstate_change;
 
 void memory_global_dirty_log_start(unsigned int flags)
 {
+    unsigned int old_flags = global_dirty_tracking;
+
     if (vmstate_change) {
         qemu_del_vm_change_state_handler(vmstate_change);
         vmstate_change = NULL;
@@ -2781,15 +2783,14 @@ void memory_global_dirty_log_start(unsigned int flags)
     assert(flags && !(flags & (~GLOBAL_DIRTY_MASK)));
     assert(!(global_dirty_tracking & flags));
     global_dirty_tracking |= flags;
-
     trace_global_dirty_changed(global_dirty_tracking);
 
-    MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
-
-    /* Refresh DIRTY_MEMORY_MIGRATION bit.  */
-    memory_region_transaction_begin();
-    memory_region_update_pending = true;
-    memory_region_transaction_commit();
+    if (!old_flags) {
+        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
+        memory_region_transaction_begin();
+        memory_region_update_pending = true;
+        memory_region_transaction_commit();
+    }
 }
 
 static void memory_global_dirty_log_do_stop(unsigned int flags)
@@ -2800,12 +2801,12 @@ static void memory_global_dirty_log_do_stop(unsigned int flags)
 
     trace_global_dirty_changed(global_dirty_tracking);
 
-    /* Refresh DIRTY_MEMORY_MIGRATION bit.  */
-    memory_region_transaction_begin();
-    memory_region_update_pending = true;
-    memory_region_transaction_commit();
-
-    MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
+    if (!global_dirty_tracking) {
+        memory_region_transaction_begin();
+        memory_region_update_pending = true;
+        memory_region_transaction_commit();
+        MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse);
+    }
 }
 
 static void memory_vm_change_state_handler(void *opaque, bool running,
-- 
2.32.0

Re: [PATCH] memory: Fix incorrect calls of log_global_start/stop

Posted by David Hildenbrand 2 years, 4 months ago

On 30.11.21 09:00, Peter Xu wrote:
> We should only call the log_global_start/stop when the global dirty track
> bitmask changes from zero<->non-zero.
> 
> No real issue reported for this yet probably because no immediate user to
> enable both dirty rate measurement and migration at the same time.  However
> it'll be good to be prepared for it.
> 
> Fixes: 63b41db4bc ("memory: make global_dirty_tracking a bitmask")
> Cc: Hyman Huang <huangy81@chinatelecom.cn>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH] memory: Fix incorrect calls of log_global_start/stop

Posted by Peter Xu 2 years, 4 months ago

On Tue, Nov 30, 2021 at 04:03:10PM +0100, David Hildenbrand wrote:
> On 30.11.21 09:00, Peter Xu wrote:
> > We should only call the log_global_start/stop when the global dirty track
> > bitmask changes from zero<->non-zero.
> > 
> > No real issue reported for this yet probably because no immediate user to
> > enable both dirty rate measurement and migration at the same time.  However
> > it'll be good to be prepared for it.
> > 
> > Fixes: 63b41db4bc ("memory: make global_dirty_tracking a bitmask")
> > Cc: Hyman Huang <huangy81@chinatelecom.cn>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Cc: Juan Quintela <quintela@redhat.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> LGTM
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks for the quick review, David.

Just a heads-up that I think it'll be nice to have this as 6.2-rc3 material.
QEMU planning page told me rc3 has just passed (Nov 30th) but still I didn't
see it released, so not sure..

I won't call it a blocker though, so if it missed 6.2 we just copy stable, and
it'll be needed for 6.2 stable branch only.

-- 
Peter Xu

Re: [PATCH] memory: Fix incorrect calls of log_global_start/stop

Posted by Philippe Mathieu-Daudé via 2 years, 3 months ago

On 11/30/21 09:00, Peter Xu wrote:
> We should only call the log_global_start/stop when the global dirty track
> bitmask changes from zero<->non-zero.
> 
> No real issue reported for this yet probably because no immediate user to
> enable both dirty rate measurement and migration at the same time.  However
> it'll be good to be prepared for it.
> 
> Fixes: 63b41db4bc ("memory: make global_dirty_tracking a bitmask")
> Cc: Hyman Huang <huangy81@chinatelecom.cn>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  softmmu/memory.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)

Thanks, queued via memory-api (adding Cc: qemu-stable).