| Message ID | 20240613125407.1126587-1-maobibo@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Remove duplicated zero clear with dirty_bitmap buffer | expand |
Le 13/06/2024 à 14:54, Bibo Mao a écrit : > Since dirty_bitmap pointer is allocated with function __vcalloc(), > there is __GFP_ZERO flag set in the implementation about this function > __vcalloc_noprof(). It is not necessary to clear dirty_bitmap buffer > with zero again. > > Signed-off-by: Bibo Mao <maobibo@loongson.cn> > --- > virt/kvm/kvm_main.c | 3 --- > 1 file changed, 3 deletions(-) > Hi, > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 14841acb8b95..c7d4a041dcfa 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1669,9 +1669,6 @@ static int kvm_prepare_memory_region(struct kvm *kvm, > r = kvm_alloc_dirty_bitmap(new); > if (r) > return r; > - > - if (kvm_dirty_log_manual_protect_and_init_set(kvm)) > - bitmap_set(new->dirty_bitmap, 0, new->npages); unless I miss something obvious, this does not clear anything, but set all bits to 1. 0 is not for "write 0" (i.e. clear), but for "start at offset 0". So all bits are set to 1 in this case. CJ > } > } > > > base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670
On 2024/6/14 上午3:25, Christophe JAILLET wrote: > Le 13/06/2024 à 14:54, Bibo Mao a écrit : >> Since dirty_bitmap pointer is allocated with function __vcalloc(), >> there is __GFP_ZERO flag set in the implementation about this function >> __vcalloc_noprof(). It is not necessary to clear dirty_bitmap buffer >> with zero again. >> >> Signed-off-by: Bibo Mao <maobibo@loongson.cn> >> --- >> virt/kvm/kvm_main.c | 3 --- >> 1 file changed, 3 deletions(-) >> > > Hi, > >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c >> index 14841acb8b95..c7d4a041dcfa 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -1669,9 +1669,6 @@ static int kvm_prepare_memory_region(struct kvm >> *kvm, >> r = kvm_alloc_dirty_bitmap(new); >> if (r) >> return r; >> - >> - if (kvm_dirty_log_manual_protect_and_init_set(kvm)) >> - bitmap_set(new->dirty_bitmap, 0, new->npages); > > unless I miss something obvious, this does not clear anything, but set > all bits to 1. > > 0 is not for "write 0" (i.e. clear), but for "start at offset 0". > So all bits are set to 1 in this case. you are right, I had thought it is to write 0 :( I do not know whether KVM_DIRTY_LOG_INITIALLY_SET should be enabled on LoongArch. If it is set, write protection for second MMU will start one by one in function kvm_arch_mmu_enable_log_dirty_pt_masked() when dirty log is cleared if it is set, else write protection will start in function kvm_arch_commit_memory_region() when flag of memslot is changed. I do not see the obvious benefits between these two write protect stages. Can anyone give me any hints? Regards Bibo Mao > > CJ > >> } >> } >> >> base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670
On 6/14/24 04:45, maobibo wrote: > I do not know whether KVM_DIRTY_LOG_INITIALLY_SET should be enabled on > LoongArch. If it is set, write protection for second MMU will start one > by one in function kvm_arch_mmu_enable_log_dirty_pt_masked() when dirty > log is cleared if it is set, else write protection will start in > function kvm_arch_commit_memory_region() when flag of memslot is changed. > > I do not see the obvious benefits between these two write protect > stages. Can anyone give me any hints? The advantage is that you get (a lot) fewer vmexits to set the dirty bitmap, and that write protection is not done in a single expensive step. Instead it is done at the time that userspace first clears the bits in the dirty bitmap. It provides much better performance. Paolo
On 2024/6/21 上午5:17, Paolo Bonzini wrote: > On 6/14/24 04:45, maobibo wrote: >> I do not know whether KVM_DIRTY_LOG_INITIALLY_SET should be enabled on >> LoongArch. If it is set, write protection for second MMU will start >> one by one in function kvm_arch_mmu_enable_log_dirty_pt_masked() when >> dirty log is cleared if it is set, else write protection will start in >> function kvm_arch_commit_memory_region() when flag of memslot is changed. >> >> I do not see the obvious benefits between these two write protect >> stages. Can anyone give me any hints? > > The advantage is that you get (a lot) fewer vmexits to set the dirty > bitmap, and that write protection is not done in a single expensive > step. Instead it is done at the time that userspace first clears the > bits in the dirty bitmap. It provides much better performance. Got it, thanks for the explanation . Regards Bibo Mao > > Paolo >
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 14841acb8b95..c7d4a041dcfa 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1669,9 +1669,6 @@ static int kvm_prepare_memory_region(struct kvm *kvm, r = kvm_alloc_dirty_bitmap(new); if (r) return r; - - if (kvm_dirty_log_manual_protect_and_init_set(kvm)) - bitmap_set(new->dirty_bitmap, 0, new->npages); } }
Since dirty_bitmap pointer is allocated with function __vcalloc(), there is __GFP_ZERO flag set in the implementation about this function __vcalloc_noprof(). It is not necessary to clear dirty_bitmap buffer with zero again. Signed-off-by: Bibo Mao <maobibo@loongson.cn> --- virt/kvm/kvm_main.c | 3 --- 1 file changed, 3 deletions(-) base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670