| Message ID | 20230928162959.1514661-3-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: small locking cleanups | expand |
У чт, 2023-09-28 у 12:29 -0400, Paolo Bonzini пише: > The "bool shared" argument is more or less unnecessary in the > for_each_*_tdp_mmu_root_yield_safe() macros. Many users check for > the lock before calling it; all of them either call small functions > that do the check, or end up calling tdp_mmu_set_spte_atomic() and > tdp_mmu_iter_set_spte(). Add a few assertions to make up for the > lost check in for_each_*_tdp_mmu_root_yield_safe(), but even this > is probably overkill and mostly for documentation reasons. Why not to leave the 'kvm_lockdep_assert_mmu_lock_held' but drop the shared argument from it? and then use lockdep_assert_held. If I am not mistaken, lockdep_assert_held should assert if the lock is held for read or write. Best regards, Maxim Levitsky > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/mmu/tdp_mmu.c | 42 +++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c > index ab0876015be7..b9abfa78808a 100644 > --- a/arch/x86/kvm/mmu/tdp_mmu.c > +++ b/arch/x86/kvm/mmu/tdp_mmu.c > @@ -155,23 +155,20 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm, > * If shared is set, this function is operating under the MMU lock in read > * mode. > */ > -#define __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _shared, _only_valid)\ > +#define __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _only_valid)\ > for (_root = tdp_mmu_next_root(_kvm, NULL, _only_valid); \ > _root; \ > _root = tdp_mmu_next_root(_kvm, _root, _only_valid)) \ > - if (kvm_lockdep_assert_mmu_lock_held(_kvm, _shared) && \ > - kvm_mmu_page_as_id(_root) != _as_id) { \ > + if (kvm_mmu_page_as_id(_root) != _as_id) { \ > } else > > -#define for_each_valid_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _shared) \ > - __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _shared, true) > +#define for_each_valid_tdp_mmu_root_yield_safe(_kvm, _root, _as_id) \ > + __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, true) > > -#define for_each_tdp_mmu_root_yield_safe(_kvm, _root, _shared) \ > +#define for_each_tdp_mmu_root_yield_safe(_kvm, _root) \ > for (_root = tdp_mmu_next_root(_kvm, NULL, false); \ > _root; \ > _root = tdp_mmu_next_root(_kvm, _root, false)) > - if (!kvm_lockdep_assert_mmu_lock_held(_kvm, _shared)) { \ > - } else > > /* > * Iterate over all TDP MMU roots. Requires that mmu_lock be held for write, > @@ -840,7 +837,8 @@ bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, gfn_t start, gfn_t end, bool flush) > { > struct kvm_mmu_page *root; > > - for_each_tdp_mmu_root_yield_safe(kvm, root, false) > + lockdep_assert_held_write(&kvm->mmu_lock); > + for_each_tdp_mmu_root_yield_safe(kvm, root) > flush = tdp_mmu_zap_leafs(kvm, root, start, end, true, flush); > > return flush; > @@ -862,7 +860,8 @@ void kvm_tdp_mmu_zap_all(struct kvm *kvm) > * is being destroyed or the userspace VMM has exited. In both cases, > * KVM_RUN is unreachable, i.e. no vCPUs will ever service the request. > */ > - for_each_tdp_mmu_root_yield_safe(kvm, root, false) > + lockdep_assert_held_write(&kvm->mmu_lock); > + for_each_tdp_mmu_root_yield_safe(kvm, root) > tdp_mmu_zap_root(kvm, root, false); > } > > @@ -876,7 +875,7 @@ void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm) > > read_lock(&kvm->mmu_lock); > > - for_each_tdp_mmu_root_yield_safe(kvm, root, true) { > + for_each_tdp_mmu_root_yield_safe(kvm, root) { > if (!root->tdp_mmu_scheduled_root_to_zap) > continue; > > @@ -899,7 +898,7 @@ void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm) > * the root must be reachable by mmu_notifiers while it's being > * zapped > */ > - kvm_tdp_mmu_put_root(kvm, root, true); > + kvm_tdp_mmu_put_root(kvm, root); > } > > read_unlock(&kvm->mmu_lock); > @@ -1133,7 +1132,9 @@ bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range, > { > struct kvm_mmu_page *root; > > - __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, false, false) > + lockdep_assert_held_write(&kvm->mmu_lock); > + > + __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, false) > flush = tdp_mmu_zap_leafs(kvm, root, range->start, range->end, > range->may_block, flush); > > @@ -1322,7 +1323,7 @@ bool kvm_tdp_mmu_wrprot_slot(struct kvm *kvm, > > lockdep_assert_held_read(&kvm->mmu_lock); > > - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, true) > + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) > spte_set |= wrprot_gfn_range(kvm, root, slot->base_gfn, > slot->base_gfn + slot->npages, min_level); > > @@ -1354,6 +1355,8 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm, > { > struct kvm_mmu_page *sp; > > + kvm_lockdep_assert_mmu_lock_held(kvm, shared); > + > /* > * Since we are allocating while under the MMU lock we have to be > * careful about GFP flags. Use GFP_NOWAIT to avoid blocking on direct > @@ -1504,11 +1507,10 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm, > int r = 0; > > kvm_lockdep_assert_mmu_lock_held(kvm, shared); > - > - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, shared) { > + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) { > r = tdp_mmu_split_huge_pages_root(kvm, root, start, end, target_level, shared); > if (r) { > - kvm_tdp_mmu_put_root(kvm, root, shared); > + kvm_tdp_mmu_put_root(kvm, root); > break; > } > } > @@ -1568,8 +1570,7 @@ bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm, > bool spte_set = false; > > lockdep_assert_held_read(&kvm->mmu_lock); > - > - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, true) > + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) > spte_set |= clear_dirty_gfn_range(kvm, root, slot->base_gfn, > slot->base_gfn + slot->npages); > > @@ -1703,8 +1704,7 @@ void kvm_tdp_mmu_zap_collapsible_sptes(struct kvm *kvm, > struct kvm_mmu_page *root; > > lockdep_assert_held_read(&kvm->mmu_lock); > - > - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, true) > + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) > zap_collapsible_spte_range(kvm, root, slot); > } >
On Thu, Sep 28, 2023, Maxim Levitsky wrote: > У чт, 2023-09-28 у 12:29 -0400, Paolo Bonzini пише: > > The "bool shared" argument is more or less unnecessary in the > > for_each_*_tdp_mmu_root_yield_safe() macros. Many users check for > > the lock before calling it; all of them either call small functions > > that do the check, or end up calling tdp_mmu_set_spte_atomic() and > > tdp_mmu_iter_set_spte(). Add a few assertions to make up for the > > lost check in for_each_*_tdp_mmu_root_yield_safe(), but even this > > is probably overkill and mostly for documentation reasons. > > Why not to leave the 'kvm_lockdep_assert_mmu_lock_held' but drop the shared > argument from it? and then use lockdep_assert_held. If I am not mistaken, > lockdep_assert_held should assert if the lock is held for read or write. +1, I don't see any downside to asserting that mmu_lock is held when iterating. It'll be a redundant assertion 99% of the time, but it's not like performance matters all that much when running with lockdep enabled. And I find lockdep assertions to be wonderful documentation.
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c index ab0876015be7..b9abfa78808a 100644 --- a/arch/x86/kvm/mmu/tdp_mmu.c +++ b/arch/x86/kvm/mmu/tdp_mmu.c @@ -155,23 +155,20 @@ static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm, * If shared is set, this function is operating under the MMU lock in read * mode. */ -#define __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _shared, _only_valid)\ +#define __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _only_valid)\ for (_root = tdp_mmu_next_root(_kvm, NULL, _only_valid); \ _root; \ _root = tdp_mmu_next_root(_kvm, _root, _only_valid)) \ - if (kvm_lockdep_assert_mmu_lock_held(_kvm, _shared) && \ - kvm_mmu_page_as_id(_root) != _as_id) { \ + if (kvm_mmu_page_as_id(_root) != _as_id) { \ } else -#define for_each_valid_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _shared) \ - __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _shared, true) +#define for_each_valid_tdp_mmu_root_yield_safe(_kvm, _root, _as_id) \ + __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, true) -#define for_each_tdp_mmu_root_yield_safe(_kvm, _root, _shared) \ +#define for_each_tdp_mmu_root_yield_safe(_kvm, _root) \ for (_root = tdp_mmu_next_root(_kvm, NULL, false); \ _root; \ _root = tdp_mmu_next_root(_kvm, _root, false)) - if (!kvm_lockdep_assert_mmu_lock_held(_kvm, _shared)) { \ - } else /* * Iterate over all TDP MMU roots. Requires that mmu_lock be held for write, @@ -840,7 +837,8 @@ bool kvm_tdp_mmu_zap_leafs(struct kvm *kvm, gfn_t start, gfn_t end, bool flush) { struct kvm_mmu_page *root; - for_each_tdp_mmu_root_yield_safe(kvm, root, false) + lockdep_assert_held_write(&kvm->mmu_lock); + for_each_tdp_mmu_root_yield_safe(kvm, root) flush = tdp_mmu_zap_leafs(kvm, root, start, end, true, flush); return flush; @@ -862,7 +860,8 @@ void kvm_tdp_mmu_zap_all(struct kvm *kvm) * is being destroyed or the userspace VMM has exited. In both cases, * KVM_RUN is unreachable, i.e. no vCPUs will ever service the request. */ - for_each_tdp_mmu_root_yield_safe(kvm, root, false) + lockdep_assert_held_write(&kvm->mmu_lock); + for_each_tdp_mmu_root_yield_safe(kvm, root) tdp_mmu_zap_root(kvm, root, false); } @@ -876,7 +875,7 @@ void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm) read_lock(&kvm->mmu_lock); - for_each_tdp_mmu_root_yield_safe(kvm, root, true) { + for_each_tdp_mmu_root_yield_safe(kvm, root) { if (!root->tdp_mmu_scheduled_root_to_zap) continue; @@ -899,7 +898,7 @@ void kvm_tdp_mmu_zap_invalidated_roots(struct kvm *kvm) * the root must be reachable by mmu_notifiers while it's being * zapped */ - kvm_tdp_mmu_put_root(kvm, root, true); + kvm_tdp_mmu_put_root(kvm, root); } read_unlock(&kvm->mmu_lock); @@ -1133,7 +1132,9 @@ bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range, { struct kvm_mmu_page *root; - __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, false, false) + lockdep_assert_held_write(&kvm->mmu_lock); + + __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id, false) flush = tdp_mmu_zap_leafs(kvm, root, range->start, range->end, range->may_block, flush); @@ -1322,7 +1323,7 @@ bool kvm_tdp_mmu_wrprot_slot(struct kvm *kvm, lockdep_assert_held_read(&kvm->mmu_lock); - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, true) + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) spte_set |= wrprot_gfn_range(kvm, root, slot->base_gfn, slot->base_gfn + slot->npages, min_level); @@ -1354,6 +1355,8 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm, { struct kvm_mmu_page *sp; + kvm_lockdep_assert_mmu_lock_held(kvm, shared); + /* * Since we are allocating while under the MMU lock we have to be * careful about GFP flags. Use GFP_NOWAIT to avoid blocking on direct @@ -1504,11 +1507,10 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm, int r = 0; kvm_lockdep_assert_mmu_lock_held(kvm, shared); - - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, shared) { + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) { r = tdp_mmu_split_huge_pages_root(kvm, root, start, end, target_level, shared); if (r) { - kvm_tdp_mmu_put_root(kvm, root, shared); + kvm_tdp_mmu_put_root(kvm, root); break; } } @@ -1568,8 +1570,7 @@ bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm, bool spte_set = false; lockdep_assert_held_read(&kvm->mmu_lock); - - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, true) + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) spte_set |= clear_dirty_gfn_range(kvm, root, slot->base_gfn, slot->base_gfn + slot->npages); @@ -1703,8 +1704,7 @@ void kvm_tdp_mmu_zap_collapsible_sptes(struct kvm *kvm, struct kvm_mmu_page *root; lockdep_assert_held_read(&kvm->mmu_lock); - - for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id, true) + for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) zap_collapsible_spte_range(kvm, root, slot); }
The "bool shared" argument is more or less unnecessary in the for_each_*_tdp_mmu_root_yield_safe() macros. Many users check for the lock before calling it; all of them either call small functions that do the check, or end up calling tdp_mmu_set_spte_atomic() and tdp_mmu_iter_set_spte(). Add a few assertions to make up for the lost check in for_each_*_tdp_mmu_root_yield_safe(), but even this is probably overkill and mostly for documentation reasons. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/mmu/tdp_mmu.c | 42 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-)