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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42330C27C78 for ; Tue, 11 Jun 2024 23:05:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C74346B010D; Tue, 11 Jun 2024 19:05:28 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BFBC66B010F; Tue, 11 Jun 2024 19:05:28 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A4E896B0110; Tue, 11 Jun 2024 19:05:28 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 8519F6B010D for ; Tue, 11 Jun 2024 19:05:28 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 0A88D405D7 for ; Tue, 11 Jun 2024 23:05:28 +0000 (UTC) X-FDA: 82220141136.10.DC645BA Received: from mail-qt1-f172.google.com (mail-qt1-f172.google.com [209.85.160.172]) by imf02.hostedemail.com (Postfix) with ESMTP id 3DF5A80017 for ; Tue, 11 Jun 2024 23:05:25 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=qxvG1Ljw; spf=pass (imf02.hostedemail.com: domain of jthoughton@google.com designates 209.85.160.172 as permitted sender) smtp.mailfrom=jthoughton@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718147125; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Pz/3hS9rvEm8ISFTbesB06oUlpFYSPujM05j9EdyaiQ=; b=Z01mnuuyM4mbhY9VnSCYgyyaF483/S0LtzNd7uNerwQRPxTB9//SWMa9+Kvzsua0XEMwgO xWn6B+99WdhIm5FWBv0Nca76PmYmfAPm8Nn7whWQzntj62BGsSuo82SSktNJ5sllYGCWyT 4pApuOnfCkpy8COb0xenB3BLoFj3+wE= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=qxvG1Ljw; spf=pass (imf02.hostedemail.com: domain of jthoughton@google.com designates 209.85.160.172 as permitted sender) smtp.mailfrom=jthoughton@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718147125; a=rsa-sha256; cv=none; b=QomuWfQtpTXUJZSmLfXgjeJ9Nu7E4RbB4kKA/oACUdYaDo6wdfiJLPctLs31ONCu2nZWib bAtR5YRDuaXkEgDq8B80NewM4OdxZyfaz723/ONNDbCo+TRnzmPwR47huAo/lALjwGxSJf pgwgqr0vtm3KWgbAVh+s/0yZNvdRu7I= Received: by mail-qt1-f172.google.com with SMTP id d75a77b69052e-4405dffca81so36141cf.1 for ; Tue, 11 Jun 2024 16:05:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1718147124; x=1718751924; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=Pz/3hS9rvEm8ISFTbesB06oUlpFYSPujM05j9EdyaiQ=; b=qxvG1LjwiABVT6MmgmEOCMjcapUK/TSECkQMux2LQwwCqbdMrMh+OTdltZXyXM0UYs RYPJMKvtvbF0RxxQkbtTj4No7z4tyPjBSGWuB1JKwZiRJgbtwvHniHWgJI2w5R1vXdf5 N8uzS+yNxlrBIpc4J2gtLglzrcpFCgcPQX0HViiomoB1qrfsRSwuGfC835KxWB9rVwYn vMUBLESboc9h6mur0aP2zrf2CKgQqN1lf7ktnREUgj80o/zYrbtRK0J/bMb2D4eBDYvi hi2XDjo9mVqdPp54tPAXZ0bjpvTKq8cwiqsZqBAtgZ8SbMGp4nGFn+KsLf68KG9gXim6 7YJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718147124; x=1718751924; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Pz/3hS9rvEm8ISFTbesB06oUlpFYSPujM05j9EdyaiQ=; b=g58sjGWFyhKdPwyngAbRdfE31z+KbQrl/5wplN1iQwzD7Hzehxllfh3NOjmJkEcgRS UvDYVF7IrUWD8A/aFF9zHiKVlvjSx1kK6H9jVA5YW8Nd0MQ2ZZKSQLuxqsL1mpi9Q7QA 1WBbjN+R0kLaJTYlCYuhlvroppvNs/EZjr6T472pv5Snnh9E8xus2jW1qY0PPyb5SG6p +XKQ8Xv6BgenPfrHzlgAA4i+jVrk1foRe8zWeD/BeG7JwpK9/Qtg1ry+5RCcYD7v+wbl gyG1LzWmuS1JgG6F0l0kHHhrfmQqDthEpRPUwvJvWSWqH/75g/PDAgAep7t6pEIo7EyK hCvQ== X-Forwarded-Encrypted: i=1; AJvYcCWmev6VpNKSTCjCZJETDwzRtW8e8VfN0oJKSDnxQX7vlC1b1cyFg3vbwb1PplrMyPYTQaEwsSP6BW7wfIrKyKofInc= X-Gm-Message-State: AOJu0YyfgVHsERHCcNMgXQpT0elRuA44UGAuMtjutFTKJUUjFT6gudEG a5oI+4TkhFd6b5qwdxwOpGWNq+3hn/DOHtd536ILx/sfl6ijEinsC5rqjK1OCs3d/po30G3AyQL S659XCaT6T1HvZt2/8IaP0yeCE5uno6rsAYKp X-Google-Smtp-Source: AGHT+IE52oInjAZCB3WBMhaUU5ZrU3GZGDMx8kQzX/bSZjmjZyyxSH/FDIuluJ0bSVsR8b8JvGhmRRklaeYoppeG6VQ= X-Received: by 2002:a05:622a:4015:b0:440:331b:59f7 with SMTP id d75a77b69052e-44158bbdd7cmr1206381cf.6.1718147123716; Tue, 11 Jun 2024 16:05:23 -0700 (PDT) MIME-Version: 1.0 References: <20240611002145.2078921-1-jthoughton@google.com> <20240611002145.2078921-5-jthoughton@google.com> In-Reply-To: From: James Houghton Date: Tue, 11 Jun 2024 16:04:47 -0700 Message-ID: Subject: Re: [PATCH v5 4/9] mm: Add test_clear_young_fast_only MMU notifier To: Sean Christopherson Cc: Yu Zhao , Andrew Morton , Paolo Bonzini , Ankit Agrawal , Axel Rasmussen , Catalin Marinas , David Matlack , David Rientjes , James Morse , Jonathan Corbet , Marc Zyngier , Oliver Upton , Raghavendra Rao Ananta , Ryan Roberts , Shaoqin Huang , Suzuki K Poulose , Wei Xu , Will Deacon , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 3DF5A80017 X-Rspam-User: X-Rspamd-Server: rspam09 X-Stat-Signature: 743cfn3xgrn3pwp6ukgfwdo866go8wqu X-HE-Tag: 1718147125-763663 X-HE-Meta: U2FsdGVkX1+ca5uGJLiZON/Uw0X1NQDP1BSFcq17M4+50OTuuWvi7RU6xVoxmEs1botcrfHnkZJSgn+Xz//NrA+cFUwKsK4qZMHFb76dYzjWphpaFJsoDsMkrApDzW/SbzOr5OZSgJ/LUdkOTwl/sTvzISC+5N/WiLTI0zmxOi2SIWu0o0yK9TYbXDuOl0VaG+wZbMNNPLxW9XW08NCBo1DTUF+YehgG/w1mdgz9UXhJ2yWkWodzh3w9wV9S7w3kXSzVR3EWSlpO0v+Bpqokx3JNWGCtnVWRaaZF2nqXNfH0Qw19bwJrr55UgokId3I0NKrlQFKSZtKzhuVuP40T/KQdRlZQp9XCdDztinpb2GNz51JhBVNoLdA6cdb+eDGKtUtING6s5/4B5YS00Dkq8qbPgrFzthFtqm/5G1pUB1zkbXf51CJGtKr7coKcvnRfqGdtTgo63gtfFhCeQtRo5ATyPt0kGGRUjaMTrLq7DQ9EOlaiZ+MnhOWblLvNWvT4eQAkPyZy874sjyp5wx+eUQ+40tJGNzlOb7hFR6KwJNrgrUfnQLF/U79Dtv2qXVAkEGlcWGmh9LAUeSnZmcFmZ3yvn0mL49ii3d5k5SsmIsrCofeQSF7zherQJygaN6AS+RvdTXAUwC6ASZAW/4rnsgEq9GYDF+z6Y3T0qKC6XfqYI5xTBj5TktxC6Dzq0i4z2fbojjFFni8emVV8NFQgdm0/079574pOV/i9/jK1Js3uKqH9jOI7Ry5+RKzg8+rz/V5dJvRkDmFM5vBmpt8BBFV8mM94Xqyosf0LpuvVtAKO/JkGjP2hBsWkRKbLlmZ5zkyb26m36ssbOq0sL0mTNL43UAL79lD7Rfxe/sA/MMaSpyHHjZUwW56wdjvmP6ymtc5yP01yoajEFjY2XgtnsHx0JHwOKr8xOOC6FfOh/hKpkepzDdcB83+vXZ+Y2jUHIn7ye3+fuR4PZxje/6O p75vqn07 5Z79b7mKVdFIS9UUSvTPQ06Eali5bxB2/ODsLX3qob5t6tVJdI3c/AZ6jU+ZB+M95mF8HMLPpIaudzA7eLHWCxuXHONQMO8eErHXKBGeAkXdiPPP27sb8STmOOTww3GnTkL1I4N2gZh+3gxkPeraa7IMgK/511MfVcrft/RW8DIGHuiwQI3NbLBo7D9ic3t8eUt3Wu6M28kSU3/c= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Tue, Jun 11, 2024 at 12:42=E2=80=AFPM Sean Christopherson wrote: > > On Tue, Jun 11, 2024, James Houghton wrote: > > On Mon, Jun 10, 2024 at 10:34=E2=80=AFPM Yu Zhao wr= ote: > > > > > > On Mon, Jun 10, 2024 at 6:22=E2=80=AFPM James Houghton wrote: > > > > > > > > This new notifier is for multi-gen LRU specifically > > > > > > Let me call it out before others do: we can't be this self-serving. > > > > > > > as it wants to be > > > > able to get and clear age information from secondary MMUs only if i= t can > > > > be done "fast". > > > > > > > > By having this notifier specifically created for MGLRU, what "fast" > > > > means comes down to what is "fast" enough to improve MGLRU's abilit= y to > > > > reclaim most of the time. > > > > > > > > Signed-off-by: James Houghton > > > > > > If we'd like this to pass other MM reviewers, especially the MMU > > > notifier maintainers, we'd need to design a generic API that can > > > benefit all the *existing* users: idle page tracking [1], DAMON [2] > > > and MGLRU. > > > > > > Also I personally prefer to extend the existing callbacks by adding > > > new parameters, and on top of that, I'd try to consolidate the > > > existing callbacks -- it'd be less of a hard sell if my changes resul= t > > > in less code, not more. > > > > > > (v2 did all these, btw.) > > > > I think consolidating the callbacks is cleanest, like you had it in > > v2. I really wasn't sure about this change honestly, but it was my > > attempt to incorporate feedback like this[3] from v4. I'll consolidate > > the callbacks like you had in v2. > > James, wait for others to chime in before committing yourself to a course= of > action, otherwise you're going to get ping-ponged to hell and back. Ah yeah. I really mean "I'll do it, provided the other feedback is in line with this". > > > Instead of the bitmap like you had, I imagine we'll have some kind of > > flags argument that has bits like MMU_NOTIFIER_YOUNG_CLEAR, > > MMU_NOTIFIER_YOUNG_FAST_ONLY, and other ones as they come up. Does > > that sound ok? > > Why do we need a bundle of flags? If we extend .clear_young() and .test_= young() > as Yu suggests, then we only need a single "bool fast_only". We don't need to. In my head it's a little easier to collapse them (slightly less code, and at the callsite you have a flag with a name instead of a true/false). Making it a bool SGTM. > As for adding a fast_only versus dedicated APIs, I don't have a strong pr= eference. > Extending will require a small amount of additional churn, e.g. to pass i= n false, > but that doesn't seem problematic on its own. On the plus side, there wo= uld be > less copy+paste in include/linux/mmu_notifier.h (though that could be sol= ved with > macros :-) ). I think having the extra bool is cleaner than the new fast_only notifier, definitely. > > E.g. > > -- > diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c > index 7b77ad6cf833..07872ae00fa6 100644 > --- a/mm/mmu_notifier.c > +++ b/mm/mmu_notifier.c > @@ -384,7 +384,8 @@ int __mmu_notifier_clear_flush_young(struct mm_struct= *mm, > > int __mmu_notifier_clear_young(struct mm_struct *mm, > unsigned long start, > - unsigned long end) > + unsigned long end, > + bool fast_only) > { > struct mmu_notifier *subscription; > int young =3D 0, id; > @@ -393,9 +394,12 @@ int __mmu_notifier_clear_young(struct mm_struct *mm, > hlist_for_each_entry_rcu(subscription, > &mm->notifier_subscriptions->list, hlist= , > srcu_read_lock_held(&srcu)) { > - if (subscription->ops->clear_young) > - young |=3D subscription->ops->clear_young(subscri= ption, > - mm, start= , end); > + if (!subscription->ops->clear_young || > + fast_only && !subscription->ops->has_fast_aging) > + continue; > + > + young |=3D subscription->ops->clear_young(subscription, > + mm, start, end); KVM changing has_fast_aging dynamically would be slow, wouldn't it? I feel like it's simpler to just pass in fast_only into `clear_young` itself (and this is how I interpreted what you wrote above anyway). > } > srcu_read_unlock(&srcu, id); > > @@ -403,7 +407,8 @@ int __mmu_notifier_clear_young(struct mm_struct *mm, > } > > int __mmu_notifier_test_young(struct mm_struct *mm, > - unsigned long address) > + unsigned long address, > + bool fast_only) > { > struct mmu_notifier *subscription; > int young =3D 0, id; > @@ -412,12 +417,15 @@ int __mmu_notifier_test_young(struct mm_struct *mm, > hlist_for_each_entry_rcu(subscription, > &mm->notifier_subscriptions->list, hlist= , > srcu_read_lock_held(&srcu)) { > - if (subscription->ops->test_young) { > - young =3D subscription->ops->test_young(subscript= ion, mm, > - address); > - if (young) > - break; > - } > + if (!subscription->ops->test_young) > + continue; > + > + if (fast_only && !subscription->ops->has_fast_aging) > + continue; > + > + young =3D subscription->ops->test_young(subscription, mm,= address); > + if (young) > + break; > } > srcu_read_unlock(&srcu, id); > -- > > It might also require multiplexing the return value to differentiate betw= een > "young" and "failed". Ugh, but the code already does that, just in a bes= poke way. Yeah, that is necessary. > Double ugh. Peeking ahead at the "failure" code, NAK to adding > kvm_arch_young_notifier_likely_fast for all the same reasons I objected t= o > kvm_arch_has_test_clear_young() in v1. Please stop trying to do anything= like > that, I will NAK each every attempt to have core mm/ code call directly i= nto KVM. Sorry to make you repeat yourself; I'll leave it out of v6. I don't like it either, but I wasn't sure how important it was to avoid calling into unnecessary notifiers if the TDP MMU were completely disabled. > Anyways, back to this code, before we spin another version, we need to ag= ree on > exactly what behavior we want out of secondary MMUs. Because to me, the = behavior > proposed in this version doesn't make any sense. > > Signalling failure because KVM _might_ have relevant aging information in= SPTEs > that require taking kvm->mmu_lock is a terrible tradeoff. And for the te= st_young > case, it's flat out wrong, e.g. if a page is marked Accessed in the TDP M= MU, then > KVM should return "young", not "failed". Sorry for this oversight. What about something like: 1. test (and maybe clear) A bits on TDP MMU 2. If accessed && !should_clear: return (fast) 3. if (fast_only): return (fast) 4. If !(must check shadow MMU): return (fast) 5. test (and maybe clear) A bits in shadow MMU 6. return (slow) Some of this reordering (and maybe a change from kvm_shadow_root_allocated() to checking indirect_shadow_pages or something else) can be done in its own patch. > If KVM is using the TDP MMU, i.e. has_fast_aging=3Dtrue, then there will = be rmaps > if and only if L1 ran a nested VM at some point. But as proposed, KVM do= esn't > actually check if there are any shadow TDP entries to process. That coul= d be > fixed by looking at kvm->arch.indirect_shadow_pages, but even then it's n= ot clear > that bailing if kvm->arch.indirect_shadow_pages > 0 makes sense. > > E.g. if L1 happens to be running an L2, but <10% of the VM's memory is ex= posed to > L2, then "failure" is pretty much guaranteed to a false positive. And ev= en for > the pages that are exposed to L2, "failure" will occur if and only if the= pages > are being accessed _only_ by L2. > > There most definitely are use cases where the majority of a VM's memory i= s accessed > only by L2. But if those use cases are performing poorly under MGLRU, th= en IMO > we should figure out a way to enhance KVM to do a fast harvest of nested = TDP > Accessed information, not make MGRLU+KVM suck for a VMs that run nested V= Ms. This makes sense. I don't have data today to say that we would get a huge win from speeding up harvesting Accessed information from the shadow MMU would be helpful. Getting this information for the TDP MMU is at least better than no information at all. > > Oh, and calling into mmu_notifiers to do the "slow" version if the fast v= ersion > fails is suboptimal. Agreed. I didn't like this when I wrote it. This can be easily fixed by making mmu_notifier_clear_young() return "fast" and "young or not", which I will do. > So rather than failing the fast aging, I think what we want is to know if= an > mmu_notifier found a young SPTE during a fast lookup. E.g. something lik= e this > in KVM, where using kvm_has_shadow_mmu_sptes() instead of kvm_memslots_ha= ve_rmaps() > is an optional optimization to avoid taking mmu_lock for write in paths w= here a > (very rare) false negative is acceptable. > > static bool kvm_has_shadow_mmu_sptes(struct kvm *kvm) > { > return !tdp_mmu_enabled || READ_ONCE(kvm->arch.indirect_shadow_pa= ges); > } > > static int __kvm_age_gfn(struct kvm *kvm, struct kvm_gfn_range *range, > bool fast_only) > { > int young =3D 0; > > if (!fast_only && kvm_has_shadow_mmu_sptes(kvm)) { > write_lock(&kvm->mmu_lock); > young =3D kvm_handle_gfn_range(kvm, range, kvm_age_rmap); > write_unlock(&kvm->mmu_lock); > } > > if (tdp_mmu_enabled && kvm_tdp_mmu_age_gfn_range(kvm, range)) > young =3D 1 | MMU_NOTIFY_WAS_FAST; I don't think this line is quite right. We might set MMU_NOTIFY_WAS_FAST even when we took the mmu_lock. I understand what you mean though, thanks. > > return (int)young; > } > > and then in lru_gen_look_around(): > > if (spin_is_contended(pvmw->ptl)) > return false; > > /* exclude special VMAs containing anon pages from COW */ > if (vma->vm_flags & VM_SPECIAL) > return false; > > young =3D ptep_clear_young_notify(vma, addr, pte); > if (!young) > return false; > > if (!(young & MMU_NOTIFY_WAS_FAST)) > return true; > > young =3D 1; > > with the lookaround done using ptep_clear_young_notify_fast(). > > The MMU_NOTIFY_WAS_FAST flag is gross, but AFAICT it would Just Work with= out > needing to update all users of ptep_clear_young_notify() and friends. Sounds good to me. Thanks for all the feedback!