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 2DF95EB64D8 for ; Tue, 20 Jun 2023 07:31:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:References:From:Subject:Cc: To:Message-Id:Date:Mime-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=8FbQS1MRyPGx1XXWROajq6PW+trMLeIPcQCwtMd0gxU=; b=VsnzcALRwHEdBt f8aSyx2FOAgJucPviYmttuTTPNQhi9TcSBbSinVe8xbV6RD6ZGWZCzFU3xjj3kdTKahLO7ATznbky 9svOVOtw7r/FfiwxaSEFexhCS/yxXxBTC243WjK6edXgFMbxaepq0eVWHlt2dM/wC7nPywb+/pmyf 0odV7ZnHz8GSfwhw7LCXOzIY1bbIgycLjo2SsPeVUoEFELB4ssO308HlFmpXfhXZ7t1yjbCz0wK6O x4uenRN1LdycjFvfamoMvHX/2RZ2a1mJWu8AUhmyhiu0XajS/PCfay/DV1ZQxo42xAsxRlp6JFF4J kzCir/6Wu4VilhgRaSGw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qBVov-00AOFf-2b; Tue, 20 Jun 2023 07:30:33 +0000 Received: from mail-pl1-x634.google.com ([2607:f8b0:4864:20::634]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qBVos-00AOEJ-0Z for linux-arm-kernel@lists.infradead.org; Tue, 20 Jun 2023 07:30:31 +0000 Received: by mail-pl1-x634.google.com with SMTP id d9443c01a7336-1b52fd56b7aso17387465ad.2 for ; Tue, 20 Jun 2023 00:30:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687246228; x=1689838228; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=pYV4rW6gKPpoa3eFGZn6o9TgBp896EY80rdpl9Nfr2s=; b=kQcnC5U4PfyLQDSNkgJ+ZABIfGbRvyggeR2C8Y/N456McznJImrS63FChLkZaxR5tR Yq9f7Fxc2W99C/vYKA7o90DjifVQOxR0zUmPd2QEoj+ZsFWgSwqMn23Ryj/MQnhi07XE +FR5gSzkjkQbqq3p+O+0CpJeIdKu999apYycR+66zy89IamAqsRPerriuP6fbapd6VKd 9HKmNtYaXYD5JqcEUNTemZQjEs9szfcsmT7wGpnlhLG3lNn6gmpsq11BV3U2q6pLf8KZ hLgEt3KZCnMPg6XY/thkZMJk7T5TBObUL6+V3k5F2FiirDyfxWxH5sIOasXCkEmGX1a3 i+fw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687246228; x=1689838228; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=pYV4rW6gKPpoa3eFGZn6o9TgBp896EY80rdpl9Nfr2s=; b=K2M2pHMnqaCrZn/Whc8JlR9hsnzQhgkIoijsvRCFs4ou5Dwfdle0N7F3yUUFx30ZxD oVobVDCnjZA3cdCByEcxbqS30CuJaEWW76/DMmab5J7haME9MSwqM/OacCKb26L7u6Lp PRFq/cp6M6xkn54PPqe7m1inUt+eiz/kYT2RxOMR8rhATb7H/KU4IGgBjkv0bLpGEaeG yScdyj35D1MyqhX+ER1kl++z3jVe+aJwHCkZtZdkzlkytnalCIbT/B+nny8Ahd1WG/Iv 5Nbt2g+/5eocIFn4IsB3XtDIFB39N/TdO7imPXEVyWcWNWu0vnkAnRjv87fkVPLV5sVj C0CA== X-Gm-Message-State: AC+VfDyrGuYqpRuLHrsVgK6wQYHvWja772FYxlj7+rQ/9aWokiHH9Huf CYQ7Dj8H/QoaBcKfBlxcg4A= X-Google-Smtp-Source: ACHHUZ43eep1AAmMl46MrK15oMgQPLrGXyP67yrmCY8AmGwY+QIfVnfreQMQhLQu3G2HRVKs6stwBg== X-Received: by 2002:a17:902:c14a:b0:1b2:404c:7d46 with SMTP id 10-20020a170902c14a00b001b2404c7d46mr6394563plj.54.1687246227734; Tue, 20 Jun 2023 00:30:27 -0700 (PDT) Received: from localhost ([124.170.190.103]) by smtp.gmail.com with ESMTPSA id iy22-20020a170903131600b001b04b1fae4dsm958860plb.35.2023.06.20.00.30.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 20 Jun 2023 00:30:27 -0700 (PDT) Mime-Version: 1.0 Date: Tue, 20 Jun 2023 17:30:09 +1000 Message-Id: To: "Yu Zhao" , "Andrew Morton" , "Paolo Bonzini" Cc: "Alistair Popple" , "Anup Patel" , "Ben Gardon" , "Borislav Petkov" , "Catalin Marinas" , "Chao Peng" , "Christophe Leroy" , "Dave Hansen" , "Fabiano Rosas" , "Gaosheng Cui" , "Gavin Shan" , "H. Peter Anvin" , "Ingo Molnar" , "James Morse" , "Jason A. Donenfeld" , "Jason Gunthorpe" , "Jonathan Corbet" , "Marc Zyngier" , "Masami Hiramatsu" , "Michael Ellerman" , "Michael Larabel" , "Mike Rapoport" , "Oliver Upton" , "Paul Mackerras" , "Peter Xu" , "Sean Christopherson" , "Steven Rostedt" , "Suzuki K Poulose" , "Thomas Gleixner" , "Thomas Huth" , "Will Deacon" , "Zenghui Yu" , , , , , , , , , , Subject: Re: [PATCH mm-unstable v2 01/10] mm/kvm: add mmu_notifier_ops->test_clear_young() From: "Nicholas Piggin" X-Mailer: aerc 0.14.0 References: <20230526234435.662652-1-yuzhao@google.com> <20230526234435.662652-2-yuzhao@google.com> In-Reply-To: <20230526234435.662652-2-yuzhao@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230620_003030_219518_39AA3111 X-CRM114-Status: GOOD ( 43.52 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sat May 27, 2023 at 9:44 AM AEST, Yu Zhao wrote: > Add mmu_notifier_ops->test_clear_young() to supersede test_young() > and clear_young(). > > test_clear_young() has a fast path, which if supported, allows its > callers to safely clear the accessed bit without taking > kvm->mmu_lock. > > The fast path requires arch-specific code that generally relies on > RCU and CAS: the former protects KVM page tables from being freed > while the latter clears the accessed bit atomically against both the > hardware and other software page table walkers. If the fast path is > unsupported, test_clear_young() falls back to the existing slow path > where kvm->mmu_lock is then taken. > > test_clear_young() can also operate on a range of KVM PTEs > individually according to a bitmap, if the caller provides it. It would be better if you could do patch 1 that only touches the mmu_notifier code and implements mmu_notifier_test_clear_young() in terms of existing callbacks, and next patch swaps KVM to new callbacks and remove the old ones. > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h > index 64a3e051c3c4..dfdbb370682d 100644 > --- a/include/linux/mmu_notifier.h > +++ b/include/linux/mmu_notifier.h > @@ -60,6 +60,8 @@ enum mmu_notifier_event { > }; > > #define MMU_NOTIFIER_RANGE_BLOCKABLE (1 << 0) > +#define MMU_NOTIFIER_RANGE_LOCKLESS (1 << 1) > +#define MMU_NOTIFIER_RANGE_YOUNG (1 << 2) > > struct mmu_notifier_ops { > /* > @@ -122,6 +124,10 @@ struct mmu_notifier_ops { > struct mm_struct *mm, > unsigned long address); > > + int (*test_clear_young)(struct mmu_notifier *mn, struct mm_struct *mm, > + unsigned long start, unsigned long end, > + bool clear, unsigned long *bitmap); This should have a comment like the others. Callback wants to know how to implement it. Could add a _range on it as well while you're here, to correct that inconsistency. > + > /* > * change_pte is called in cases that pte mapping to page is changed: > * for example, when ksm remaps pte to point to a new shared page. > @@ -392,6 +398,9 @@ extern int __mmu_notifier_clear_young(struct mm_struct *mm, > unsigned long end); > extern int __mmu_notifier_test_young(struct mm_struct *mm, > unsigned long address); > +extern int __mmu_notifier_test_clear_young(struct mm_struct *mm, > + unsigned long start, unsigned long end, > + bool clear, unsigned long *bitmap); > extern void __mmu_notifier_change_pte(struct mm_struct *mm, > unsigned long address, pte_t pte); > extern int __mmu_notifier_invalidate_range_start(struct mmu_notifier_range *r); > @@ -440,6 +449,35 @@ static inline int mmu_notifier_test_young(struct mm_struct *mm, > return 0; > } > > +/* > + * mmu_notifier_test_clear_young() returns nonzero if any of the KVM PTEs within > + * a given range was young. Specifically, it returns MMU_NOTIFIER_RANGE_LOCKLESS > + * if the fast path was successful, MMU_NOTIFIER_RANGE_YOUNG otherwise. > + * > + * The last parameter to the function is a bitmap and only the fast path > + * supports it: if it is NULL, the function falls back to the slow path if the > + * fast path was unsuccessful; otherwise, the function bails out. Then if it was NULL, you would just not populate it. Minmize differences and cases for the caller/implementations. > + * > + * The bitmap has the following specifications: > + * 1. The number of bits should be at least (end-start)/PAGE_SIZE. > + * 2. The offset of each bit should be relative to the end, i.e., the offset > + * corresponding to addr should be (end-addr)/PAGE_SIZE-1. This is convenient > + * for batching while forward looping. > + * > + * When testing, this function sets the corresponding bit in the bitmap for each > + * young KVM PTE. When clearing, this function clears the accessed bit for each > + * young KVM PTE whose corresponding bit in the bitmap is set. I think this is over-designed as a first pass. The secondary MMU should just implement the call always. If it can't do it locklessly, then just do individual lookups. If the benefit is in the batching as you say then the locked version will get similar benefit. Possibly more because locks like some amount of batching when contended. I think that would reduce some concerns about cases of secondary MMUs that do not not support the lockless version yet, and avoid proliferation of code paths by platform. _If_ that was insufficient then I would like to see numbers and profiles and incremental patch to expose more complexity like this. Also mmu notifier code should say nothing about KVM PTEs or use kvm names in any code or comments either. "if the page was accessed via the secondary MMU" or similar is mutually understandable to KVM and mm developers. > @@ -880,6 +887,72 @@ static int kvm_mmu_notifier_test_young(struct mmu_notifier *mn, > kvm_test_age_gfn); > } > > +struct test_clear_young_args { > + unsigned long *bitmap; > + unsigned long end; > + bool clear; > + bool young; > +}; > + > +bool kvm_should_clear_young(struct kvm_gfn_range *range, gfn_t gfn) > +{ > + struct test_clear_young_args *args = range->args; > + > + VM_WARN_ON_ONCE(gfn < range->start || gfn >= range->end); > + > + args->young = true; > + > + if (args->bitmap) { > + int offset = hva_to_gfn_memslot(args->end - 1, range->slot) - gfn; > + > + if (args->clear) > + return test_bit(offset, args->bitmap); > + > + __set_bit(offset, args->bitmap); > + } > + > + return args->clear; > +} I don't quite understnd what's going on here. This is actually the function that notes the young pte, despite its name suggesting it is only a query. Shouldn't it set the bitmap bit even in the clear case? And why is it testing at all? Oh, it seems to be some strange mix of test *or* clear young. With the bitmap being a predicate in some cases for the clear case. This is a fairly confusing multi-modal API then. I think it should take 2 bitmaps, one is the young bitmap and the other is the predicate bitmap, and either/or can be NULL. Also this kvm_should_clear_young helper is clunky and misnamed. If you just provided an inline helper to get test_clear_young bitmap offset from gfn, then set/clear bit in the caller is quite trivial. > + > +static int kvm_mmu_notifier_test_clear_young(struct mmu_notifier *mn, struct mm_struct *mm, > + unsigned long start, unsigned long end, > + bool clear, unsigned long *bitmap) > +{ > + struct kvm *kvm = mmu_notifier_to_kvm(mn); > + struct kvm_hva_range range = { > + .start = start, > + .end = end, > + .on_lock = (void *)kvm_null_fn, > + .on_unlock = (void *)kvm_null_fn, > + }; > + > + trace_kvm_age_hva(start, end); > + > + if (kvm_arch_has_test_clear_young()) { > + struct test_clear_young_args args = { > + .bitmap = bitmap, > + .end = end, > + .clear = clear, > + }; > + > + range.args = &args; > + range.lockless = true; > + range.handler = kvm_arch_test_clear_young; > + > + if (!__kvm_handle_hva_range(kvm, &range)) > + return args.young ? MMU_NOTIFIER_RANGE_LOCKLESS : 0; > + } > + > + if (bitmap) > + return 0; > + > + range.args = NULL; > + range.lockless = false; > + range.handler = clear ? kvm_age_gfn : kvm_test_age_gfn; Minor thing, but KVM's "young" handling has been called "age" until now. Any reason not to stick with that theme? Thanks, Nick _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel