We needn't retrieve the memory slot again in user_mem_abort() because
the corresponding memory slot has been passed from the caller. This
would save some CPU cycles. For example, the time used to write 1GB
memory, which is backed by 2MB hugetlb pages and write-protected, is
dropped by 6.8% from 928ms to 864ms.
Signed-off-by: Gavin Shan <[email protected]>
---
arch/arm64/kvm/mmu.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index a5a8ade9fde4..4a4abcccfafb 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -846,7 +846,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
*/
smp_rmb();
- pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
+ pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
+ write_fault, &writable, NULL);
if (pfn == KVM_PFN_ERR_HWPOISON) {
kvm_send_hwpoison_signal(hva, vma_shift);
return 0;
@@ -912,7 +913,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
/* Mark the page dirty only if the fault is handled successfully */
if (writable && !ret) {
kvm_set_pfn_dirty(pfn);
- mark_page_dirty(kvm, gfn);
+ mark_page_dirty_in_slot(kvm, memslot, gfn);
}
out_unlock:
--
2.23.0
Hi Gavin,
On 2021/3/15 12:18, Gavin Shan wrote:
> We needn't retrieve the memory slot again in user_mem_abort() because
> the corresponding memory slot has been passed from the caller. This
I think you are right, though fault_ipa will be adjusted when we try to use block mapping,
the fault_supports_stage2_huge_mapping() makes sure we're not trying to map anything
not covered by the memslot, so the adjusted fault_ipa still belongs to the memslot.
> would save some CPU cycles. For example, the time used to write 1GB
> memory, which is backed by 2MB hugetlb pages and write-protected, is
> dropped by 6.8% from 928ms to 864ms.
>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> arch/arm64/kvm/mmu.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index a5a8ade9fde4..4a4abcccfafb 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -846,7 +846,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> */
> smp_rmb();
>
> - pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
> + pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
> + write_fault, &writable, NULL);
It's better to update the code comments at same time.
> if (pfn == KVM_PFN_ERR_HWPOISON) {
> kvm_send_hwpoison_signal(hva, vma_shift);
> return 0;
> @@ -912,7 +913,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> /* Mark the page dirty only if the fault is handled successfully */
> if (writable && !ret) {
> kvm_set_pfn_dirty(pfn);
> - mark_page_dirty(kvm, gfn);
> + mark_page_dirty_in_slot(kvm, memslot, gfn);
> }
>
> out_unlock:
>
Thanks,
Keqian.
Hi Keqian,
On 3/15/21 7:25 PM, Keqian Zhu wrote:
> On 2021/3/15 12:18, Gavin Shan wrote:
>> We needn't retrieve the memory slot again in user_mem_abort() because
>> the corresponding memory slot has been passed from the caller. This
> I think you are right, though fault_ipa will be adjusted when we try to use block mapping,
> the fault_supports_stage2_huge_mapping() makes sure we're not trying to map anything
> not covered by the memslot, so the adjusted fault_ipa still belongs to the memslot.
>
Yeah, it's correct. Besides, the @logging_active is determined
based on the passed memory slot. It means user_mem_abort() can't
support memory range which spans multiple memory slot.
>> would save some CPU cycles. For example, the time used to write 1GB
>> memory, which is backed by 2MB hugetlb pages and write-protected, is
>> dropped by 6.8% from 928ms to 864ms.
>>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> arch/arm64/kvm/mmu.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index a5a8ade9fde4..4a4abcccfafb 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -846,7 +846,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> */
>> smp_rmb();
>>
>> - pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
>> + pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
>> + write_fault, &writable, NULL);
> It's better to update the code comments at same time.
>
I guess you need some comments here? If so, I would add something
like below in v2:
/*
* gfn_to_pfn_prot() can be used either with unnecessary overhead
* introduced to locate the memory slot because the memory slot is
* always fixed even @gfn is adjusted for huge pages.
*/
>> if (pfn == KVM_PFN_ERR_HWPOISON) {
>> kvm_send_hwpoison_signal(hva, vma_shift);
>> return 0;
>> @@ -912,7 +913,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> /* Mark the page dirty only if the fault is handled successfully */
>> if (writable && !ret) {
>> kvm_set_pfn_dirty(pfn);
>> - mark_page_dirty(kvm, gfn);
>> + mark_page_dirty_in_slot(kvm, memslot, gfn);
>> }
>>
>> out_unlock:
>>
Thanks,
Gavin
Hi Gavin,
On 2021/3/15 17:56, Gavin Shan wrote:
> Hi Keqian,
>
> On 3/15/21 7:25 PM, Keqian Zhu wrote:
>> On 2021/3/15 12:18, Gavin Shan wrote:
>>> We needn't retrieve the memory slot again in user_mem_abort() because
>>> the corresponding memory slot has been passed from the caller. This
>> I think you are right, though fault_ipa will be adjusted when we try to use block mapping,
>> the fault_supports_stage2_huge_mapping() makes sure we're not trying to map anything
>> not covered by the memslot, so the adjusted fault_ipa still belongs to the memslot.
>>
>
> Yeah, it's correct. Besides, the @logging_active is determined
> based on the passed memory slot. It means user_mem_abort() can't
> support memory range which spans multiple memory slot.
>
>>> would save some CPU cycles. For example, the time used to write 1GB
>>> memory, which is backed by 2MB hugetlb pages and write-protected, is
>>> dropped by 6.8% from 928ms to 864ms.
>>>
>>> Signed-off-by: Gavin Shan <[email protected]>
>>> ---
>>> arch/arm64/kvm/mmu.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index a5a8ade9fde4..4a4abcccfafb 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -846,7 +846,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>> */
>>> smp_rmb();
>>> - pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
>>> + pfn = __gfn_to_pfn_memslot(memslot, gfn, false, NULL,
>>> + write_fault, &writable, NULL);
>> It's better to update the code comments at same time.
>>
>
> I guess you need some comments here? If so, I would add something
> like below in v2:
>
> /*
> * gfn_to_pfn_prot() can be used either with unnecessary overhead
> * introduced to locate the memory slot because the memory slot is
> * always fixed even @gfn is adjusted for huge pages.
> */
Looks good.
See comments above "smp_rmb();", and actually my meaning is that we should change "gfn_to_pfn_prot"
to "__gfn_to_pfn_memslot" :)
Thanks,
Keqian
>
>>> if (pfn == KVM_PFN_ERR_HWPOISON) {
>>> kvm_send_hwpoison_signal(hva, vma_shift);
>>> return 0;
>>> @@ -912,7 +913,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>> /* Mark the page dirty only if the fault is handled successfully */
>>> if (writable && !ret) {
>>> kvm_set_pfn_dirty(pfn);
>>> - mark_page_dirty(kvm, gfn);
>>> + mark_page_dirty_in_slot(kvm, memslot, gfn);
>>> }
>>> out_unlock:
>>>
>
> Thanks,
> Gavin
>
>
> .
>