Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Fri, 29 Sep 2023 09:14:14 -0700
In-Reply-To: <6bc63f82495501f9664b7d19bd8c7ba64329d37b.camel@redhat.com>
Mime-Version: 1.0
References: <20230928162959.1514661-1-pbonzini@redhat.com> <20230928162959.1514661-3-pbonzini@redhat.com>
 <6bc63f82495501f9664b7d19bd8c7ba64329d37b.camel@redhat.com>
Message-ID: <ZRb31g0PBR588XwK@google.com>
Subject: Re: [PATCH 2/3] KVM: x86/mmu: remove unnecessary "bool shared"
 argument from iterators
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 28, 2023, Maxim Levitsky wrote:
> =D0=A3 =D1=87=D1=82, 2023-09-28 =D1=83 12:29 -0400, Paolo Bonzini =D0=BF=
=D0=B8=D1=88=D0=B5:
> > 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.
>=20
> Why not to leave the 'kvm_lockdep_assert_mmu_lock_held' but drop the shar=
ed
> 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 iterat=
ing.

It'll be a redundant assertion 99% of the time, but it's not like performan=
ce
matters all that much when running with lockdep enabled.  And I find lockde=
p
assertions to be wonderful documentation.