On Fri, Jan 10, 2025 at 2:26 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Nov 05, 2024, James Houghton wrote:
> > Provide flexibility to the architecture to synchronize as optimally as
>
> Similar to my nit on "locking" below, "synchronize" is somewhat misleading. There's
> no requirement for architectures to synchronize during aging. There is all but
> guaranteed to be some form of "synchronization", e.g. to prevent walking freed
> page tables, but the aging itself never synchronizes, and protecting a walk by
> disabling IRQs (as x86's shadow MMU does in some flows) only "synchronizes" in a
> loose sense of the word.
>
> > they can instead of always taking the MMU lock for writing.
> >
> > Architectures that do their own locking must select
> > CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS.
>
> This is backwards and could be misleading, and for the TDP MMU outright wrong.
> If some hypothetical architecture took _additional_ locks, then it can do so
> without needing to select CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS.
>
> What you want to say is that architectures that select
> CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS are responsible for ensuring correctness.
> And it's specifically all about correctness. Common KVM doesn't care if the
> arch does it's own locking, e.g. taking mmu_lock for read, or has some completely
> lock-free scheme for aging.
>
> > The immediate application is to allow architectures to implement the
>
> "immediate application" is pretty redundant. There's really only one reason to
> not take mmu_lock in this path.
>
> > test/clear_young MMU notifiers more cheaply.
>
> IMO, "more cheaply" is vague, and doesn't add much beyond the opening sentence
> about "synchronize as optimally as they can". I would just delete this last
> sentence.
Thanks Sean. I've reworded the changelog like this:
It is possible to correctly do aging without taking the KVM MMU lock;
this option allows such architectures to do so. Architectures that
select CONFIG_KVM_MMU_NOTIFIER_AGING_LOCKLESS are responsible for
ensuring correctness.
>
> > Suggested-by: Yu Zhao <yuzhao@xxxxxxxxxx>
> > Signed-off-by: James Houghton <jthoughton@xxxxxxxxxx>
> > Reviewed-by: David Matlack <dmatlack@xxxxxxxxxx>
> > ---
> > @@ -797,6 +805,8 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn,
> > .flush_on_ret =
> > !IS_ENABLED(CONFIG_KVM_ELIDE_TLB_FLUSH_IF_YOUNG),
> > .may_block = false,
> > + .lockless =
> > + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
>
> .lockess = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS),
>
> Random thought, maybe CONFIG_KVM_MMU_NOTIFIER_AGING_LOCKLESS or
> CONFIG_KVM_MMU_NOTIFIER_AGE_LOCKLESS instead of "YOUNG"?
CONFIG_KVM_MMU_NOTIFIER_AGING_LOCKLESS sounds good. Changed.
