From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BFF32C3DA5D for ; Thu, 25 Jul 2024 16:40:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=QcSVy1tvOkU8yRLWC0qxdbVVrc8ZEhqmlAiyT2QSHkE=; b=dxIy92zBEbEIBG5Bp9u5JOuaXt Q0mMOpnBQdixNiuQ3+S8Ah65ZbxAqDhAeTP4d7iumhT5CkdT2HkjVo4nlgS+5Yv0bOBdeX0piO01s jz6Xst3uE7AwJdsng9Nee2CKOQ1ZMCcoqGpV8jVPGYqClygJaWGyk0DY7TI29Av6EuAm5dE3UnBR9 T8rLoj+mcJM3lQH9BO74DSfFXEzfEwlZZ+0dtkAfOlQl/3ayUtGkilmJueM8LTdFjR+szkP5T6Yld f7Wr8KUF28vLxX9tPyEl8UTkps8JQAkaKCfTnQNrLed6dH7/VL1Y6QF4wejsGQv818l79G1MDpw1y rPjyutHQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sX1Vh-00000001ZoA-04mn; Thu, 25 Jul 2024 16:40:09 +0000 Received: from mail-pl1-x630.google.com ([2607:f8b0:4864:20::630]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sX1VH-00000001ZfJ-09iz for linux-arm-kernel@lists.infradead.org; Thu, 25 Jul 2024 16:39:44 +0000 Received: by mail-pl1-x630.google.com with SMTP id d9443c01a7336-1fc658b6b2eso10117895ad.0 for ; Thu, 25 Jul 2024 09:39:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1721925578; x=1722530378; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=QcSVy1tvOkU8yRLWC0qxdbVVrc8ZEhqmlAiyT2QSHkE=; b=qn6M3MqDCwdhlSEc3uInCk3TdytF3/5J4qRunMsQAsNDIjNc5UMu0N4mfomYVl0FxD NiyE4M35qXfGOsbLj7VSLg0qz6q2++Fbl6jLBniJUk5x0HUNReJ+R16NKQIXSzSUnbMR dCsAOlf2EyWZqbJm70GXH8Z8Qy5iPchnyB68ACXCcIdjeP/UKNoW4Lmr5kPZjM55L5yd cjkFz1I3KxMEc7Y1ZqqEu3y37mSq/jZlwxu2/7w3EZV+mEIKdSKIu6SpHOKEPWhFcKPC P93invIW+BKiEfKg75Hj5meclX3EqozYT64MMemIayA0W8QyjjU31skFdbyVnsWS1ySX D7Rg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1721925578; x=1722530378; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=QcSVy1tvOkU8yRLWC0qxdbVVrc8ZEhqmlAiyT2QSHkE=; b=hzSJIzf8PduntMS9a2y9YRmL6vudJ8iGBIuZDU4tB6bVQo5inUe52mylJONbt8kbjo OmtVy8L/bBbHzgSz7xYEmXQg9PmGoh7sKO+5/uLreGdidDEt9605z2qR/5im94+TENmf Esrc3r6RaynO8cCugeIOmL64UA5xagJ8WywZ6oYXE8iAPe7RBJWrouPv58R2+mUSZK6Z bykv/uAMQr3rwzqJ8Ad1METdtORpxl+PY4ARTlXvT86bsvkCHrS7i9V3Yw+pBrqS9JTP IE00Uu3d0tQuAjJ4UAQZrUnWBNt7eSIV7zEhpYjlp36+DBMV0tzC1N2Sch2tCFxpXuK1 YLrA== X-Forwarded-Encrypted: i=1; AJvYcCUm/40EaaUmGb+xlRmiW0HJYZBE0RuSiuVkEn286JRBgBEXo8qX9LHcB6eTdLO3wqmUCbNWnlyAocNk75sp2Azs7hsrfzOGhpJhYv0ATCrH0SH/N3Q= X-Gm-Message-State: AOJu0Yz0MfoH/Li/UoYoRhD/1f+Pz9egUSoQ7JFsPtie0vjpwW97Re/f PbE+SDCBW9C+7VwZkfefuTUsM1vES1YSRdPA7hDvONkJ/gMJVYBcA1yf4DmFUg== X-Google-Smtp-Source: AGHT+IHB3MBDNmfV4qXP0PeruhsiIcuNxKT7iPw0JA5SjJko2zFwnnPefUglyTGQ1O1zxG/RUL8Ssw== X-Received: by 2002:a17:903:1246:b0:1f8:6bae:28f with SMTP id d9443c01a7336-1fed3870ea6mr39885005ad.9.1721925577381; Thu, 25 Jul 2024 09:39:37 -0700 (PDT) Received: from google.com (61.139.125.34.bc.googleusercontent.com. [34.125.139.61]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-1fed7ee4ce6sm16408675ad.157.2024.07.25.09.39.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Jul 2024 09:39:36 -0700 (PDT) Date: Thu, 25 Jul 2024 09:39:32 -0700 From: David Matlack To: James Houghton Cc: Andrew Morton , Paolo Bonzini , Ankit Agrawal , Axel Rasmussen , Catalin Marinas , David Rientjes , James Morse , Jason Gunthorpe , Jonathan Corbet , Marc Zyngier , Oliver Upton , Raghavendra Rao Ananta , Ryan Roberts , Sean Christopherson , Shaoqin Huang , Suzuki K Poulose , Wei Xu , Will Deacon , Yu Zhao , Zenghui Yu , kvmarm@lists.linux.dev, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH v6 01/11] KVM: Add lockless memslot walk to KVM Message-ID: References: <20240724011037.3671523-1-jthoughton@google.com> <20240724011037.3671523-2-jthoughton@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240724011037.3671523-2-jthoughton@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240725_093943_109118_CB6B0B9C X-CRM114-Status: GOOD ( 33.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2024-07-24 01:10 AM, James Houghton wrote: > Provide flexibility to the architecture to synchronize as optimally as > 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. > > The immediate application is to allow architectures to implement the > test/clear_young MMU notifiers more cheaply. > > Suggested-by: Yu Zhao > Signed-off-by: James Houghton Aside from the cleanup suggestion (which should be in separate patches anyway): Reviewed-by: David Matlack > --- > include/linux/kvm_host.h | 1 + > virt/kvm/Kconfig | 3 +++ > virt/kvm/kvm_main.c | 26 +++++++++++++++++++------- > 3 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 689e8be873a7..8cd80f969cff 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -266,6 +266,7 @@ struct kvm_gfn_range { > gfn_t end; > union kvm_mmu_notifier_arg arg; > bool may_block; > + bool lockless; > }; > bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range); > bool kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range); > diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig > index b14e14cdbfb9..632334861001 100644 > --- a/virt/kvm/Kconfig > +++ b/virt/kvm/Kconfig > @@ -100,6 +100,9 @@ config KVM_GENERIC_MMU_NOTIFIER > select MMU_NOTIFIER > bool > > +config KVM_MMU_NOTIFIER_YOUNG_LOCKLESS > + bool > + > config KVM_GENERIC_MEMORY_ATTRIBUTES > depends on KVM_GENERIC_MMU_NOTIFIER > bool > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index d0788d0a72cc..33f8997a5c29 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -555,6 +555,7 @@ struct kvm_mmu_notifier_range { > on_lock_fn_t on_lock; > bool flush_on_ret; > bool may_block; > + bool lockless; > }; > > /* > @@ -609,6 +610,10 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, > IS_KVM_NULL_FN(range->handler))) > return r; > > + /* on_lock will never be called for lockless walks */ > + if (WARN_ON_ONCE(range->lockless && !IS_KVM_NULL_FN(range->on_lock))) > + return r; > + > idx = srcu_read_lock(&kvm->srcu); > > for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) { > @@ -640,15 +645,18 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, > gfn_range.start = hva_to_gfn_memslot(hva_start, slot); > gfn_range.end = hva_to_gfn_memslot(hva_end + PAGE_SIZE - 1, slot); > gfn_range.slot = slot; > + gfn_range.lockless = range->lockless; > > if (!r.found_memslot) { > r.found_memslot = true; > - KVM_MMU_LOCK(kvm); > - if (!IS_KVM_NULL_FN(range->on_lock)) > - range->on_lock(kvm); > - > - if (IS_KVM_NULL_FN(range->handler)) > - goto mmu_unlock; > + if (!range->lockless) { > + KVM_MMU_LOCK(kvm); > + if (!IS_KVM_NULL_FN(range->on_lock)) > + range->on_lock(kvm); > + > + if (IS_KVM_NULL_FN(range->handler)) > + goto mmu_unlock; > + } > } > r.ret |= range->handler(kvm, &gfn_range); > } > @@ -658,7 +666,7 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, > kvm_flush_remote_tlbs(kvm); > > mmu_unlock: > - if (r.found_memslot) > + if (r.found_memslot && !range->lockless) > KVM_MMU_UNLOCK(kvm); > > srcu_read_unlock(&kvm->srcu, idx); > @@ -679,6 +687,8 @@ static __always_inline int kvm_handle_hva_range(struct mmu_notifier *mn, > .on_lock = (void *)kvm_null_fn, > .flush_on_ret = true, > .may_block = false, > + .lockless = > + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), > }; > > return __kvm_handle_hva_range(kvm, &range).ret; > @@ -697,6 +707,8 @@ static __always_inline int kvm_handle_hva_range_no_flush(struct mmu_notifier *mn > .on_lock = (void *)kvm_null_fn, > .flush_on_ret = false, > .may_block = false, > + .lockless = > + IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), kvm_handle_hva_range{,_no_flush}() have very generic names but they're intimately tied to the "young" notifiers. Whereas __kvm_handle_hva_range() is the truly generic handler function. This is arguably a pre-existing issue, but adding CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS makes these functions even more intamtely tied to the "young" notifiers. We could rename kvm_handle_hva_range{,_no_flush}() but I think the cleanest thing to do might be to just drop them entirely and move their contents into their callers (there are only 2 callers of these 3 functions). That will create a little duplication but IMO will make the code easier to read. And then we can also rename __kvm_handle_hva_range() to kvm_handle_hva_range(). e.g. Something like this as the end result: diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 86fb2b560d98..0146c83e24bd 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -590,8 +590,8 @@ static void kvm_null_fn(void) node; \ node = interval_tree_iter_next(node, start, last)) \ -static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, - const struct kvm_mmu_notifier_range *range) +static __always_inline kvm_mn_ret_t kvm_handle_hva_range(struct kvm *kvm, + const struct kvm_mmu_notifier_range *range) { struct kvm_mmu_notifier_return r = { .ret = false, @@ -674,48 +674,6 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm, return r; } -static __always_inline int kvm_handle_hva_range(struct mmu_notifier *mn, - unsigned long start, - unsigned long end, - gfn_handler_t handler) -{ - struct kvm *kvm = mmu_notifier_to_kvm(mn); - const struct kvm_mmu_notifier_range range = { - .start = start, - .end = end, - .handler = handler, - .on_lock = (void *)kvm_null_fn, - .flush_on_ret = true, - .may_block = false, - .lockless = - IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), - }; - - return __kvm_handle_hva_range(kvm, &range).ret; -} - -static __always_inline int kvm_handle_hva_range_no_flush(struct mmu_notifier *mn, - unsigned long start, - unsigned long end, - gfn_handler_t handler, - bool fast_only) -{ - struct kvm *kvm = mmu_notifier_to_kvm(mn); - const struct kvm_mmu_notifier_range range = { - .start = start, - .end = end, - .handler = handler, - .on_lock = (void *)kvm_null_fn, - .flush_on_ret = false, - .may_block = false, - .lockless = - IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), - .arg.fast_only = fast_only, - }; - - return __kvm_handle_hva_range(kvm, &range).ret; -} - void kvm_mmu_invalidate_begin(struct kvm *kvm) { lockdep_assert_held_write(&kvm->mmu_lock); @@ -808,7 +766,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn, * that guest memory has been reclaimed. This needs to be done *after* * dropping mmu_lock, as x86's reclaim path is slooooow. */ - if (__kvm_handle_hva_range(kvm, &hva_range).found_memslot) + if (kvm_handle_hva_range(kvm, &hva_range).found_memslot) kvm_arch_guest_memory_reclaimed(kvm); return 0; @@ -854,7 +812,7 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn, }; bool wake; - __kvm_handle_hva_range(kvm, &hva_range); + kvm_handle_hva_range(kvm, &hva_range); /* Pairs with the increment in range_start(). */ spin_lock(&kvm->mn_invalidate_lock); @@ -876,6 +834,17 @@ static int kvm_mmu_notifier_clear_flush_young(struct mmu_notifier *mn, unsigned long start, unsigned long end) { + struct kvm *kvm = mmu_notifier_to_kvm(mn); + const struct kvm_mmu_notifier_range range = { + .start = start, + .end = end, + .handler = kvm_age_gfn, + .on_lock = (void *)kvm_null_fn, + .flush_on_ret = true, + .may_block = false, + .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), + }; + trace_kvm_age_hva(start, end, false); return kvm_handle_hva_range(mn, start, end, kvm_age_gfn); @@ -887,6 +856,18 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn, unsigned long end, bool fast_only) { + struct kvm *kvm = mmu_notifier_to_kvm(mn); + const struct kvm_mmu_notifier_range range = { + .start = start, + .end = end, + .handler = kvm_age_gfn, + .on_lock = (void *)kvm_null_fn, + .flush_on_ret = false, + .may_block = false, + .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), + .arg.fast_only = fast_only, + }; + trace_kvm_age_hva(start, end, fast_only); /* @@ -902,8 +883,7 @@ static int kvm_mmu_notifier_clear_young(struct mmu_notifier *mn, * cadence. If we find this inaccurate, we might come up with a * more sophisticated heuristic later. */ - return kvm_handle_hva_range_no_flush(mn, start, end, kvm_age_gfn, - fast_only); + return kvm_handle_hva_range(kvm, &range).ret; } static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn, @@ -911,6 +891,18 @@ static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn, unsigned long address, bool fast_only) { + struct kvm *kvm = mmu_notifier_to_kvm(mn); + const struct kvm_mmu_notifier_range range = { + .start = address, + .end = address + 1, + .handler = kvm_test_age_gfn, + .on_lock = (void *)kvm_null_fn, + .flush_on_ret = false, + .may_block = false, + .lockless = IS_ENABLED(CONFIG_KVM_MMU_NOTIFIER_YOUNG_LOCKLESS), + .arg.fast_only = fast_only, + }; + trace_kvm_test_age_hva(address, fast_only); return kvm_handle_hva_range_no_flush(mn, address, address + 1, > }; > > return __kvm_handle_hva_range(kvm, &range).ret; > -- > 2.46.0.rc1.232.g9752f9e123-goog >