lkml.org 
[lkml]   [2021]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2 20/28] KVM: x86/mmu: Use atomic ops to set SPTEs in TDP MMU map
Date
On 01/04/21 18:50, Ben Gardon wrote:
>> retry:
>> if (is_shadow_present_pte(iter.old_spte)) {
>> if (is_large_pte(iter.old_spte)) {
>> if (!tdp_mmu_zap_spte_atomic(vcpu->kvm, &iter))
>> break;
>>
>> /*
>> * The iter must explicitly re-read the SPTE because
>> * the atomic cmpxchg failed.
>> */
>> iter.old_spte = READ_ONCE(*rcu_dereference(iter.sptep));
>> goto retry;
>> }
>> } else {
>> ...
>> }
>>
>> ?
> To be honest, that feels less readable to me. For me retry implies
> that we failed to make progress and need to repeat an operation, but
> the reality is that we did make progress and there are just multiple
> steps to replace the large SPTE with a child PT.

You're right, it's makes no sense---I misremembered the direction of
tdp_mmu_zap_spte_atomic's return value. I was actually thinking of this:

> Another option which could improve readability and performance would
> be to use the retry to repeat failed cmpxchgs instead of breaking out
> of the loop. Then we could avoid retrying the page fault each time a
> cmpxchg failed, which may happen a lot as vCPUs allocate intermediate
> page tables on boot. (Probably less common for leaf entries, but
> possibly useful there too.)

which would be

retry:
if (is_shadow_present_pte(iter.old_spte)) {
if (is_large_pte(iter.old_spte) &&
!tdp_mmu_zap_spte_atomic(vcpu->kvm, &iter)) {
/*
* The iter must explicitly re-read the SPTE because
* the atomic cmpxchg failed.
*/
iter.old_spte = READ_ONCE(*rcu_dereference(iter.sptep));
goto retry;
}
/* XXX move this to tdp_mmu_zap_spte_atomic? */
iter.old_spte = 0;
} else {
continue;
}
}
sp = alloc_tdp_mmu_page(vcpu, iter.gfn, iter.level);
child_pt = sp->spt;

new_spte = make_nonleaf_spte(child_pt,
!shadow_accessed_mask);

if (!tdp_mmu_set_spte_atomic(vcpu->kvm, &iter,
new_spte)) {
tdp_mmu_free_sp(sp);
/*
* The iter must explicitly re-read the SPTE because
* the atomic cmpxchg failed.
*/
iter.old_spte = READ_ONCE(*rcu_dereference(iter.sptep));
goto retry;
}
tdp_mmu_link_page(vcpu->kvm, sp, true,
huge_page_disallowed &&
req_level >= iter.level);

trace_kvm_mmu_get_page(sp, true);

which survives at least a quick smoke test of booting a 20-vCPU Windows
guest. If you agree I'll turn this into an actual patch.

\
 
 \ /
  Last update: 2021-04-01 20:10    [W:0.156 / U:1.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site