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 <[email protected]>
---
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);
}
--
2.39.1
У чт, 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 <[email protected]>
> ---
> 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.