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

[Chongyun Wu <wucy11@chinatelecom.cn>]

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