| Message ID | 20230213003925.40158-4-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Support dirty ring | expand |
On Mon, Feb 13, 2023 at 08:39:22AM +0800, Gavin Shan wrote: > In the last stage of live migration or memory slot removal, the > backup bitmap needs to be synchronized when it has been enabled. > > Signed-off-by: Gavin Shan <gshan@redhat.com> > --- > accel/kvm/kvm-all.c | 11 +++++++++++ > include/sysemu/kvm_int.h | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 01a6a026af..b5e12de522 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -1352,6 +1352,10 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml, > */ > if (kvm_state->kvm_dirty_ring_size) { > kvm_dirty_ring_reap_locked(kvm_state, NULL); > + if (kvm_state->kvm_dirty_ring_with_bitmap) { > + kvm_slot_sync_dirty_pages(mem); > + kvm_slot_get_dirty_log(kvm_state, mem); > + } > } else { > kvm_slot_get_dirty_log(kvm_state, mem); > } IIUC after the memory atomic update changes lands QEMU, we may not need this sync at all. My understanding is that we sync dirty log here only because of non-atomic updates happening in the past and we may lose dirty bits unexpectedly. Maybe Paolo knows. But that needs some more justification and history digging, so definitely more suitable to leave it for later and separate discussion. Reviewed-by: Peter Xu <peterx@redhat.com> Thanks, > @@ -1573,6 +1577,12 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage) > mem = &kml->slots[i]; > if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { > kvm_slot_sync_dirty_pages(mem); > + > + if (s->kvm_dirty_ring_with_bitmap && last_stage && > + kvm_slot_get_dirty_log(s, mem)) { > + kvm_slot_sync_dirty_pages(mem); > + } > + > /* > * This is not needed by KVM_GET_DIRTY_LOG because the > * ioctl will unconditionally overwrite the whole region. > @@ -3701,6 +3711,7 @@ static void kvm_accel_instance_init(Object *obj) > s->kernel_irqchip_split = ON_OFF_AUTO_AUTO; > /* KVM dirty ring is by default off */ > s->kvm_dirty_ring_size = 0; > + s->kvm_dirty_ring_with_bitmap = false; > s->notify_vmexit = NOTIFY_VMEXIT_OPTION_RUN; > s->notify_window = 0; > } > diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h > index 60b520a13e..fdd5b1bde0 100644 > --- a/include/sysemu/kvm_int.h > +++ b/include/sysemu/kvm_int.h > @@ -115,6 +115,7 @@ struct KVMState > } *as; > uint64_t kvm_dirty_ring_bytes; /* Size of the per-vcpu dirty ring */ > uint32_t kvm_dirty_ring_size; /* Number of dirty GFNs per ring */ > + bool kvm_dirty_ring_with_bitmap; > struct KVMDirtyRingReaper reaper; > NotifyVmexitOption notify_vmexit; > uint32_t notify_window; > -- > 2.23.0 >
On 2/22/23 4:46 AM, Peter Xu wrote: > On Mon, Feb 13, 2023 at 08:39:22AM +0800, Gavin Shan wrote: >> In the last stage of live migration or memory slot removal, the >> backup bitmap needs to be synchronized when it has been enabled. >> >> Signed-off-by: Gavin Shan <gshan@redhat.com> >> --- >> accel/kvm/kvm-all.c | 11 +++++++++++ >> include/sysemu/kvm_int.h | 1 + >> 2 files changed, 12 insertions(+) >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index 01a6a026af..b5e12de522 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -1352,6 +1352,10 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml, >> */ >> if (kvm_state->kvm_dirty_ring_size) { >> kvm_dirty_ring_reap_locked(kvm_state, NULL); >> + if (kvm_state->kvm_dirty_ring_with_bitmap) { >> + kvm_slot_sync_dirty_pages(mem); >> + kvm_slot_get_dirty_log(kvm_state, mem); >> + } >> } else { >> kvm_slot_get_dirty_log(kvm_state, mem); >> } > > IIUC after the memory atomic update changes lands QEMU, we may not need > this sync at all. > > My understanding is that we sync dirty log here only because of non-atomic > updates happening in the past and we may lose dirty bits unexpectedly. > Maybe Paolo knows. > > But that needs some more justification and history digging, so definitely > more suitable to leave it for later and separate discussion. > > Reviewed-by: Peter Xu <peterx@redhat.com> > Peter, could you please give some hints for me to understand the atomic and non-atomic update here? Ok, I will drop this part of changes in next revision with the assumption that we have atomic update supported for ARM64. Thanks, Gavin > >> @@ -1573,6 +1577,12 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage) >> mem = &kml->slots[i]; >> if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { >> kvm_slot_sync_dirty_pages(mem); >> + >> + if (s->kvm_dirty_ring_with_bitmap && last_stage && >> + kvm_slot_get_dirty_log(s, mem)) { >> + kvm_slot_sync_dirty_pages(mem); >> + } >> + >> /* >> * This is not needed by KVM_GET_DIRTY_LOG because the >> * ioctl will unconditionally overwrite the whole region. >> @@ -3701,6 +3711,7 @@ static void kvm_accel_instance_init(Object *obj) >> s->kernel_irqchip_split = ON_OFF_AUTO_AUTO; >> /* KVM dirty ring is by default off */ >> s->kvm_dirty_ring_size = 0; >> + s->kvm_dirty_ring_with_bitmap = false; >> s->notify_vmexit = NOTIFY_VMEXIT_OPTION_RUN; >> s->notify_window = 0; >> } >> diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h >> index 60b520a13e..fdd5b1bde0 100644 >> --- a/include/sysemu/kvm_int.h >> +++ b/include/sysemu/kvm_int.h >> @@ -115,6 +115,7 @@ struct KVMState >> } *as; >> uint64_t kvm_dirty_ring_bytes; /* Size of the per-vcpu dirty ring */ >> uint32_t kvm_dirty_ring_size; /* Number of dirty GFNs per ring */ >> + bool kvm_dirty_ring_with_bitmap; >> struct KVMDirtyRingReaper reaper; >> NotifyVmexitOption notify_vmexit; >> uint32_t notify_window; >> -- >> 2.23.0 >> >
Hi, Gavin, On Wed, Feb 22, 2023 at 10:44:07AM +1100, Gavin Shan wrote: > Peter, could you please give some hints for me to understand the atomic > and non-atomic update here? Ok, I will drop this part of changes in next > revision with the assumption that we have atomic update supported for > ARM64. See commit f39b7d2b96. Please don't remove the change in this patch. The comment was just something I thought about when reading, not something I suggested to change. If to remove it we'll need to remove the whole chunk not your changes alone here. Still, please take it with a grain of salt before anyone can help to confirm because I can miss something else here. In short: before we know anything solidly, your current code is exactly correct, AFAICT. Thanks,
On 2/22/23 10:58 AM, Peter Xu wrote: > On Wed, Feb 22, 2023 at 10:44:07AM +1100, Gavin Shan wrote: >> Peter, could you please give some hints for me to understand the atomic >> and non-atomic update here? Ok, I will drop this part of changes in next >> revision with the assumption that we have atomic update supported for >> ARM64. > > See commit f39b7d2b96. Please don't remove the change in this patch. > > The comment was just something I thought about when reading, not something > I suggested to change. > > If to remove it we'll need to remove the whole chunk not your changes alone > here. Still, please take it with a grain of salt before anyone can help to > confirm because I can miss something else here. > > In short: before we know anything solidly, your current code is exactly > correct, AFAICT. > Thanks, Peter. I think it's all for later. I will keep the changes and with your r-b in next revision :) Thanks, Gavin
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 01a6a026af..b5e12de522 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -1352,6 +1352,10 @@ static void kvm_set_phys_mem(KVMMemoryListener *kml, */ if (kvm_state->kvm_dirty_ring_size) { kvm_dirty_ring_reap_locked(kvm_state, NULL); + if (kvm_state->kvm_dirty_ring_with_bitmap) { + kvm_slot_sync_dirty_pages(mem); + kvm_slot_get_dirty_log(kvm_state, mem); + } } else { kvm_slot_get_dirty_log(kvm_state, mem); } @@ -1573,6 +1577,12 @@ static void kvm_log_sync_global(MemoryListener *l, bool last_stage) mem = &kml->slots[i]; if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { kvm_slot_sync_dirty_pages(mem); + + if (s->kvm_dirty_ring_with_bitmap && last_stage && + kvm_slot_get_dirty_log(s, mem)) { + kvm_slot_sync_dirty_pages(mem); + } + /* * This is not needed by KVM_GET_DIRTY_LOG because the * ioctl will unconditionally overwrite the whole region. @@ -3701,6 +3711,7 @@ static void kvm_accel_instance_init(Object *obj) s->kernel_irqchip_split = ON_OFF_AUTO_AUTO; /* KVM dirty ring is by default off */ s->kvm_dirty_ring_size = 0; + s->kvm_dirty_ring_with_bitmap = false; s->notify_vmexit = NOTIFY_VMEXIT_OPTION_RUN; s->notify_window = 0; } diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h index 60b520a13e..fdd5b1bde0 100644 --- a/include/sysemu/kvm_int.h +++ b/include/sysemu/kvm_int.h @@ -115,6 +115,7 @@ struct KVMState } *as; uint64_t kvm_dirty_ring_bytes; /* Size of the per-vcpu dirty ring */ uint32_t kvm_dirty_ring_size; /* Number of dirty GFNs per ring */ + bool kvm_dirty_ring_with_bitmap; struct KVMDirtyRingReaper reaper; NotifyVmexitOption notify_vmexit; uint32_t notify_window;
In the last stage of live migration or memory slot removal, the backup bitmap needs to be synchronized when it has been enabled. Signed-off-by: Gavin Shan <gshan@redhat.com> --- accel/kvm/kvm-all.c | 11 +++++++++++ include/sysemu/kvm_int.h | 1 + 2 files changed, 12 insertions(+)