| Message ID | 1430152117-100558-12-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, 04/27 18:28, Paolo Bonzini wrote: > The separate handling of DIRTY_MEMORY_MIGRATION, which does not > call log_start/log_stop callbacks when it changes in a region's > dirty logging mask, has caused several bugs. > > One recent example is commit 4cc856f (kvm-all: Sync dirty-bitmap from > kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02). > Another performance problem is that KVM keeps tracking dirty pages > after a failed live migration, which causes bad performance due to > disallowing huge page mapping. > > This patch removes the root cause of the problem by reporting > DIRTY_MEMORY_MIGRATION changes via log_start and log_stop. > Note that we now have to rebuild the FlatView when global dirty > logging is enabled or disabled; this ensures that log_start and > log_stop callbacks are invoked. > > This will also be used to make the setting of bitmaps conditional. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > memory.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/memory.c b/memory.c > index 1966347..174cd15 100644 > --- a/memory.c > +++ b/memory.c > @@ -537,7 +537,7 @@ static void render_memory_region(FlatView *view, > remain = clip.size; > > fr.mr = mr; > - fr.dirty_log_mask = mr->dirty_log_mask; > + fr.dirty_log_mask = memory_region_get_dirty_log_mask(mr); > fr.romd_mode = mr->romd_mode; > fr.readonly = readonly; > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr) > > uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) > { > - return mr->dirty_log_mask; > + uint8_t mask = mr->dirty_log_mask; > + if (global_dirty_log) { > + mask |= (1 << DIRTY_MEMORY_MIGRATION); This is ugly, but I don't know how to do differently. :( > + } > + return mask; > } > > bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) > @@ -1892,10 +1896,20 @@ void memory_global_dirty_log_start(void) > { > global_dirty_log = true; > MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward); > + > + /* Refresh DIRTY_LOG_MIGRATION bit. */ > + memory_region_transaction_begin(); > + memory_region_update_pending = true; > + memory_region_transaction_commit(); > } > > void memory_global_dirty_log_stop(void) > { > + /* Refresh DIRTY_LOG_MIGRATION bit. */ > + memory_region_transaction_begin(); > + memory_region_update_pending = true; > + memory_region_transaction_commit(); > + > global_dirty_log = false; Shouldn't this go before memory_region_transaction_begin? Fam
On 26/05/2015 10:40, Fam Zheng wrote: > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr) > > > > uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) > > { > > - return mr->dirty_log_mask; > > + uint8_t mask = mr->dirty_log_mask; > > + if (global_dirty_log) { > > + mask |= (1 << DIRTY_MEMORY_MIGRATION); > > This is ugly, but I don't know how to do differently. :( Why do you think it's ugly? As long as the log_start/log_stop callbacks are handled properly, I think it's okay. I find the "Refresh DIRTY_LOG_MIGRATION bit." part uglier. :) Paolo
On 26/05/2015 10:40, Fam Zheng wrote: >> > void memory_global_dirty_log_stop(void) >> > { >> > + /* Refresh DIRTY_LOG_MIGRATION bit. */ >> > + memory_region_transaction_begin(); >> > + memory_region_update_pending = true; >> > + memory_region_transaction_commit(); >> > + >> > global_dirty_log = false; > Shouldn't this go before memory_region_transaction_begin? Yes. Paolo
On Tue, 05/26 11:07, Paolo Bonzini wrote: > > > On 26/05/2015 10:40, Fam Zheng wrote: > > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr) > > > > > > uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) > > > { > > > - return mr->dirty_log_mask; > > > + uint8_t mask = mr->dirty_log_mask; > > > + if (global_dirty_log) { > > > + mask |= (1 << DIRTY_MEMORY_MIGRATION); > > > > This is ugly, but I don't know how to do differently. :( > > Why do you think it's ugly? > As long as the log_start/log_stop callbacks > are handled properly, I think it's okay. Ugly in the per object function relying on a global to propogate the new value. Fam
On 26/05/2015 11:22, Fam Zheng wrote: >>>> > > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr) >>>> > > > >>>> > > > uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) >>>> > > > { >>>> > > > - return mr->dirty_log_mask; >>>> > > > + uint8_t mask = mr->dirty_log_mask; >>>> > > > + if (global_dirty_log) { >>>> > > > + mask |= (1 << DIRTY_MEMORY_MIGRATION); >>> > > >>> > > This is ugly, but I don't know how to do differently. :( >> > >> > Why do you think it's ugly? >> > As long as the log_start/log_stop callbacks >> > are handled properly, I think it's okay. > Ugly in the per object function relying on a global to propogate the new value. Yes, but that's the point of the patch. :) It lets the listeners track updates to the local state of the MemoryRegion instead of having to track the global state. So it's somewhat ugly, but it is less ugly than tracking exactly the same global state in exec.c (core_log_global_start) and kvm-all.c (kvm_log_global_start). Does this make sense? Paolo
On Tue, 05/26 11:26, Paolo Bonzini wrote: > > > On 26/05/2015 11:22, Fam Zheng wrote: > >>>> > > > @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr) > >>>> > > > > >>>> > > > uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) > >>>> > > > { > >>>> > > > - return mr->dirty_log_mask; > >>>> > > > + uint8_t mask = mr->dirty_log_mask; > >>>> > > > + if (global_dirty_log) { > >>>> > > > + mask |= (1 << DIRTY_MEMORY_MIGRATION); > >>> > > > >>> > > This is ugly, but I don't know how to do differently. :( > >> > > >> > Why do you think it's ugly? > >> > As long as the log_start/log_stop callbacks > >> > are handled properly, I think it's okay. > > Ugly in the per object function relying on a global to propogate the new value. > > Yes, but that's the point of the patch. :) It lets the listeners track > updates to the local state of the MemoryRegion instead of having to > track the global state. So it's somewhat ugly, but it is less ugly than > tracking exactly the same global state in exec.c (core_log_global_start) > and kvm-all.c (kvm_log_global_start). Truely it's less ugly :) > > Does this make sense? Yes. Thanks. Fam
diff --git a/memory.c b/memory.c index 1966347..174cd15 100644 --- a/memory.c +++ b/memory.c @@ -537,7 +537,7 @@ static void render_memory_region(FlatView *view, remain = clip.size; fr.mr = mr; - fr.dirty_log_mask = mr->dirty_log_mask; + fr.dirty_log_mask = memory_region_get_dirty_log_mask(mr); fr.romd_mode = mr->romd_mode; fr.readonly = readonly; @@ -1329,7 +1329,11 @@ bool memory_region_is_skip_dump(MemoryRegion *mr) uint8_t memory_region_get_dirty_log_mask(MemoryRegion *mr) { - return mr->dirty_log_mask; + uint8_t mask = mr->dirty_log_mask; + if (global_dirty_log) { + mask |= (1 << DIRTY_MEMORY_MIGRATION); + } + return mask; } bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) @@ -1892,10 +1896,20 @@ void memory_global_dirty_log_start(void) { global_dirty_log = true; MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward); + + /* Refresh DIRTY_LOG_MIGRATION bit. */ + memory_region_transaction_begin(); + memory_region_update_pending = true; + memory_region_transaction_commit(); } void memory_global_dirty_log_stop(void) { + /* Refresh DIRTY_LOG_MIGRATION bit. */ + memory_region_transaction_begin(); + memory_region_update_pending = true; + memory_region_transaction_commit(); + global_dirty_log = false; MEMORY_LISTENER_CALL_GLOBAL(log_global_stop, Reverse); }
The separate handling of DIRTY_MEMORY_MIGRATION, which does not call log_start/log_stop callbacks when it changes in a region's dirty logging mask, has caused several bugs. One recent example is commit 4cc856f (kvm-all: Sync dirty-bitmap from kvm before kvm destroy the corresponding dirty_bitmap, 2015-04-02). Another performance problem is that KVM keeps tracking dirty pages after a failed live migration, which causes bad performance due to disallowing huge page mapping. This patch removes the root cause of the problem by reporting DIRTY_MEMORY_MIGRATION changes via log_start and log_stop. Note that we now have to rebuild the FlatView when global dirty logging is enabled or disabled; this ensures that log_start and log_stop callbacks are invoked. This will also be used to make the setting of bitmaps conditional. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- memory.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)