Re: [PATCH v1 1/5] kvm,memory: Optimize dirty page collection for dirty ring

[Chongyun Wu]

Thank you very much for your review comments.

on 3/23/2022 12:59 PM, Hyman Huang wrote:
> 在 2022/3/23 11:18, wucy11@chinatelecom.cn 写道:
> > From: Chongyun Wu <wucy11@chinatelecom.cn>
> >
> > When log_sync_global of dirty ring is called, it will collect
> > dirty pages on all cpus, including all dirty pages on memslot,
> > so when memory_region_sync_dirty_bitmap collects dirty pages
> > from KVM, this interface needs to be called once, instead of
> > traversing every dirty page. Each memslot is called once,
> > which is meaningless and time-consuming. So only need to call
> > log_sync_global once in memory_region_sync_dirty_bitmap.
> >
> > Signed-off-by: Chongyun Wu <wucy11@chinatelecom.cn>
> > ---
> >   softmmu/memory.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 8060c6d..46c3ff9 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -2184,6 +2184,12 @@ static void 
> > memory_region_sync_dirty_bitmap(MemoryRegion *mr)
> >                */
> >               listener->log_sync_global(listener);
> >               trace_memory_region_sync_dirty(mr ? mr->name : "(all)", 
> > listener->name, 1);
> > +            /*
> > +             * The log_sync_global of the dirty ring will collect the dirty
> > +             * pages of all memslots at one time, so there is no need to
> > +             * call log_sync_global once when traversing each memslot.
> > +             */
> > +            break;
> To the code logic itself, if a listener does not implement the log_sync_global 
> callback, it never go there. If not, it should not break because we never know 
> which callback is what dirty ring want.
> IMHO, maybe the modification should remove from the common logic and implemented 
> somewhere else, i havn't think it through yet.
Yes, you are right. It may be more reasonable to move the modification to 
kvm_set_phys_mem. In kvm_set_phys_mem, kvm_dirty_ring_reap_locked is called once 
for all memslots marked with dirty pages. This interface will collect dirty 
pages of all memslots, so there is a problem of repeated calls, so it is may be 
more reasonable to optimize them here. I will modify it in the next version.
> >           }
> >       }
> >   }

--
Best Regard,
Chongyun Wu