Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <6bc63f82495501f9664b7d19bd8c7ba64329d37b.camel@redhat.com>
Subject: Re: [PATCH 2/3] KVM: x86/mmu: remove unnecessary "bool shared"
 argument from iterators
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Date:   Thu, 28 Sep 2023 19:55:28 +0300
In-Reply-To: <20230928162959.1514661-3-pbonzini@redhat.com>
References: <20230928162959.1514661-1-pbonzini@redhat.com>
         <20230928162959.1514661-3-pbonzini@redhat.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

У чт, 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);
>  }
>