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 6ED16C25B48 for ; Tue, 24 Oct 2023 23:16:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DB3D96B02F4; Tue, 24 Oct 2023 19:16:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D62E56B02F5; Tue, 24 Oct 2023 19:16:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C2B336B02F6; Tue, 24 Oct 2023 19:16:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id B02606B02F4 for ; Tue, 24 Oct 2023 19:16:52 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 79019A0AF6 for ; Tue, 24 Oct 2023 23:16:52 +0000 (UTC) X-FDA: 81381917064.03.AA84BA8 Received: from mail-ua1-f44.google.com (mail-ua1-f44.google.com [209.85.222.44]) by imf23.hostedemail.com (Postfix) with ESMTP id 44A6314000F for ; Tue, 24 Oct 2023 23:16:49 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YO5iHrrF; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf23.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.44 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1698189409; 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=KOu+GUGt5bf45brXFu8qbvUcorQRuKU9cK9mjz8tCvw=; b=2xTbNoJ2BF1Ha95UFw1RBwcA7bO6x+X1Y6YnJom3BWos6VpAHCHBCsz1zWKyEn5GOYj7fz 9AA5FRWwzFVlrun0yYy6uIgqaE5VGJI0MJ3S5GhDAJ1rnOLeJdV7vY2Km36hG/1even9Dq p9CKyooDQFz71riA/vIWOZ2ZHXb2m54= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YO5iHrrF; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf23.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.222.44 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1698189409; a=rsa-sha256; cv=none; b=4doHLFePjIZwGl6kSo7Tdl+SFfcc28tYDtZAuB3kyu/9+qPEAY+KwNb3tlrq1zlBV04gnM TpoxqFrbEZKmLwhSRZhFYUnNyX4yDd2pKl7k5rQOs2iCLqVsl/7HAOj5OB16Xk8fyTHxEZ MeapZxJ0qTMSQcVBOCM6VXNY0Bh8cYo= Received: by mail-ua1-f44.google.com with SMTP id a1e0cc1a2514c-7b6b4c2f1a0so1861328241.1 for ; Tue, 24 Oct 2023 16:16:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698189408; x=1698794208; 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=KOu+GUGt5bf45brXFu8qbvUcorQRuKU9cK9mjz8tCvw=; b=YO5iHrrFh4HfTfkpcKRnDj0WbrIGVdW2ZeZtZmLk5OdKhNAuHMP2JuXmMEotNWJ8nc 4y/5OWa9BS7hHEjqcPYcb4neyiJiSjoq+lbmTbqs5g2B3ady0fZ0007eho5/E9bQ6xSB LDjU6ebEzc5+KMxUwztGK2Xxaay/D1Hj/lm65saC3ji0DSmD4NhydJYKez+mIr9u+SR9 uY51W7WcZZJ52uOmpX1/tYAEXswzag9Z9WqkD56Tx3sX10uMS0FvWzDYHAz8ru6mMAV4 c2afWGQmw5ZqJen5yt955Uab3MuIaLMtxPS8HP1gFUrM4fM+KLs7x3I4OjtBlpuu6ByO nuLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698189408; x=1698794208; 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=KOu+GUGt5bf45brXFu8qbvUcorQRuKU9cK9mjz8tCvw=; b=arTWwfi95yu0qA9nkE3eMU6BGG7TTeD2R3HfYMRX4KQ0NuTc7b7esTf140n7Dm5kY9 7Wm4c36hsFrRjmHMo4U0B/GBzHZngWsn1bxSeZRjbw3QiXvbbSQae+Ao/xzEHMzVpfIT LBdMENEqizsMDYzmbJ+mN0dmBpjqk0PeNRTB4XFPEYFhBvC5J8QvOEPbmt0Rv1V+XnTA svLZ5RB9AD+aboaKiDEQE7i259bVGu42CUWo/7VoLVrbVA8u2I4ebyI4Wu+TnVZhjVa8 WQFebCknZjl/BEhesgVarW3jb1qPVJvWT+yJxtBnKAbs/NsGLgnfwSGRag+v8YeLQSyq 90Kg== X-Gm-Message-State: AOJu0YztHbWOqvBfnYAePfDCYNWfszrpKI6rXEQgA95wSGw/EBJe/xj1 4k3PDaZknE59HFEZfgyqd/ka3QyIUCHeQIbmIiM= X-Google-Smtp-Source: AGHT+IFUhISub6RvDgagu1NeIrq/YwoFByzkIenUrghjxImmwcBRQQXlc1x+4E33oYimKOg/OqtJXARn26JO99YQRlk= X-Received: by 2002:a05:6102:20dc:b0:452:66a7:1ac with SMTP id i28-20020a05610220dc00b0045266a701acmr13577539vsr.6.1698189408226; Tue, 24 Oct 2023 16:16:48 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Wed, 25 Oct 2023 07:16:36 +0800 Message-ID: Subject: Re: [PATCH] arm64: mm: drop tlb flush operation when clearing the access bit To: Baolin Wang Cc: catalin.marinas@arm.com, will@kernel.org, akpm@linux-foundation.org, v-songbaohua@oppo.com, yuzhao@google.com, linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 44A6314000F X-Stat-Signature: y5kwqwaa3hb1unmy7ci1baky74pgg4br X-Rspam-User: X-HE-Tag: 1698189409-786940 X-HE-Meta: U2FsdGVkX199H5O9wLsIy5tgT5mb5PUdao9sTxfbKyRkEcwAwG8XnxdMdRk9DMOiHpQOkCMzlpwGm4XwrS8/5X6899s1KKEeXDx/Soy6vWPmfV0Shf7NdH7/vG+paL2xsVxRP4vCAg2Qn89ljjQXDWR4yI9x3gM7gBkPj8FzL1V8BhmxeQreARUTXbcu1MKH2ksmSbLOnJyt0YU/Ts3kHYcwOR5d4wRSnHbBYmDb6uB0FCZGTCiHGEVpKP7q0i6OPk5ffwTWBR+vtBYjvTtB/t+BulfC91WhAcLKt8lgCSksDgSd/cmp9KM/+pZ72bdWR7IprR5MVN2uCYQCRMc58jNvuqSk09znk83jEzyJmhdvnLvOMRxGXMWtDtSEC3FAXp/btWDbbNODNkAnR/9watcZ1tU38is+8XQMwqbdJWyrnrg6wtT0dtfke+I1NWucRd24UhmAiSpHzmn+MC4KLyY3CcGRAUDXw08OGxyXLJ2YZMIqZZes6P3YUmgjDEiHr09xRvRNiobalZvAYtBGnjkcEjaL0SYb+YBfrs8EGqBMvMSIDWMWY8N2GTWb9j/gwYNkwCyMnLw1Bd2fNyw5XK/01mmcuoK7G6MLHMgyfYL4beGG+onkuw50GKQoLjZYm70aLY/4byY0A/hiV7v0N2re32TWjpRwCABUxg4kpmXPwuQBSeFOjPNBU5+vkhZTn/2/B9DVIjEBl1/YX7P+kRdpRqS6Z/WzkzzqRle3MzVpVH+fv6usleqAQCXNMbiONKDbOXhI+Tcd3RjvQEWE5fUrq1JtJ/pY/Nl+nlO1bBjqi4HoLo5Eb6RclVkIgIP9vbJipL0jKghRfhMIyB14UTJ3LK+Ts8EtV41X00AIjfPMKDP/p5azqmx+dk7jEv6STMtUvvEDF/NYbOqlKfX88eF8vbYRl17zM9++FvZlw2k/jh1mNQDBRvEorSaRRq7oHjxk2FnBWFYQ5UdqYdw VI0/TtF8 IUIAL9PI7/C+Fc5C27ql6LGyyO6HuO2CcHV+yCdWXkn9dACcYYVXhBK5CiL+cSk2x+qzDYKLc7NEPD8ZRoefNkuYvIugJZvsHKOZKvEf92HImH+Jp7yO0gu9pIEZwkqUzI9hovyZHH9cPg4dkoK6QP29/2WMEmPEWWW0tVCMdSeKNjBjuRY+9T+fpgrQ0j8uzc10ohmhOmW78J2VVtLS5ZcUQrC+p3fxO5GRgDwDr4JvZTWcnbdR4RsSCBYa9XuypNVsvABJopKjSvaVgbutdvuNVwE5tjyQeT1SKegSg6HdPFwZHFavcOjcu/1KrLjM9eno4zR6in45cWbiDYNsmTaCLi17V0gEzXZXI7sEk6R6wpt4ZrQwMbUXuwAm7qYm77ebG9UKikc316OJcwPy8ZKhL4lqoGg/Ns7ffOMOAu7bXD/gC9fM6BFoAgs9xPP35X2DaSrhCw2a60Sw= 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, Oct 24, 2023 at 8:57=E2=80=AFPM Baolin Wang wrote: > > Now ptep_clear_flush_young() is only called by folio_referenced() to > check if the folio was referenced, and now it will call a tlb flush on > ARM64 architecture. However the tlb flush can be expensive on ARM64 > servers, especially for the systems with a large CPU numbers. > > Similar to the x86 architecture, below comments also apply equally to > ARM64 architecture. So we can drop the tlb flush operation in > ptep_clear_flush_young() on ARM64 architecture to improve the performance= . > " > /* Clearing the accessed bit without a TLB flush > * doesn't cause data corruption. [ It could cause incorrect > * page aging and the (mistaken) reclaim of hot pages, but the > * chance of that should be relatively low. ] > * > * So as a performance optimization don't flush the TLB when > * clearing the accessed bit, it will eventually be flushed by > * a context switch or a VM operation anyway. [ In the rare > * event of it not getting flushed for a long time the delay > * shouldn't really matter because there's no real memory > * pressure for swapout to react to. ] > */ > " > Running the thpscale to show some obvious improvements for compaction > latency with this patch: > base patched > Amean fault-both-1 1093.19 ( 0.00%) 1084.57 * 0.79%* > Amean fault-both-3 2566.22 ( 0.00%) 2228.45 * 13.16%* > Amean fault-both-5 3591.22 ( 0.00%) 3146.73 * 12.38%* > Amean fault-both-7 4157.26 ( 0.00%) 4113.67 * 1.05%* > Amean fault-both-12 6184.79 ( 0.00%) 5218.70 * 15.62%* > Amean fault-both-18 9103.70 ( 0.00%) 7739.71 * 14.98%* > Amean fault-both-24 12341.73 ( 0.00%) 10684.23 * 13.43%* > Amean fault-both-30 15519.00 ( 0.00%) 13695.14 * 11.75%* > Amean fault-both-32 16189.15 ( 0.00%) 14365.73 * 11.26%* > base patched > Duration User 167.78 161.03 > Duration System 1836.66 1673.01 > Duration Elapsed 2074.58 2059.75 > > Barry Song submitted a similar patch [1] before, that replaces the > ptep_clear_flush_young_notify() with ptep_clear_young_notify() in > folio_referenced_one(). However, I'm not sure if removing the tlb flush > operation is applicable to every architecture in kernel, so dropping > the tlb flush for ARM64 seems a sensible change. > > Note: I am okay for both approach, if someone can help to ensure that > all architectures do not need the tlb flush when clearing the accessed > bit, then I also think Barry's patch is better (hope Barry can resend > his patch). > Thanks! ptep_clear_flush_young() with "flush" in its name clearly says it needs a flush. but it happens in arm64, all other code which needs a flush has called other variants, for example __flush_tlb_page_nosync(): static inline void arch_tlbbatch_add_pending(struct arch_tlbflush_unmap_batch *batch, struct mm_struct *mm, unsigned long uaddr) { __flush_tlb_page_nosync(mm, uaddr); } so it seems folio_referenced is the only left user of this ptep_clear_flush_young(). The fact makes Baolin's patch look safe now. but this function still has "flush" in its name, so one day, one person mig= ht call it with the understanding that it will flush tlb but actually it won't. This is bad smell in code. I guess one side effect of not flushing tlb while clearing the access flag is that hardware won't see this cleared flag in the tlb, so it might not set this b= it in memory even though the bit has been cleared before in memory(but not in tlb= ) while the page is accessed *again*. next time, someone reads the access flag in memory again by folio_reference= d, he/she will see the page is cold as hardware has lost a chance to set the bit again since it finds tlb already has a true access flag. But anyway, tlb is so small, it will be flushed by context switch and other running code soon. so it seems we don't actually require the access flag being inst= antly updated. the time gap, in which access flag might lose the new set by hardw= are, seems to be too short to really affect the accuracy of page reclamation. bu= t its cost is large. (A). Constant flush cost vs. (B). very very occasional reclaimed hot page, B might be a correct choice. > [1] https://lore.kernel.org/lkml/20220617070555.344368-1-21cnbao@gmail.co= m/ > Signed-off-by: Baolin Wang > --- > arch/arm64/include/asm/pgtable.h | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pg= table.h > index 0bd18de9fd97..2979d796ba9d 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -905,21 +905,22 @@ static inline int ptep_test_and_clear_young(struct = vm_area_struct *vma, > static inline int ptep_clear_flush_young(struct vm_area_struct *vma, > unsigned long address, pte_t *pt= ep) > { > - int young =3D ptep_test_and_clear_young(vma, address, ptep); > - > - if (young) { > - /* > - * We can elide the trailing DSB here since the worst tha= t can > - * happen is that a CPU continues to use the young entry = in its > - * TLB and we mistakenly reclaim the associated page. The > - * window for such an event is bounded by the next > - * context-switch, which provides a DSB to complete the T= LB > - * invalidation. > - */ > - flush_tlb_page_nosync(vma, address); > - } > - > - return young; > + /* > + * This comment is borrowed from x86, but applies equally to ARM6= 4: > + * > + * Clearing the accessed bit without a TLB flush doesn't cause > + * data corruption. [ It could cause incorrect page aging and > + * the (mistaken) reclaim of hot pages, but the chance of that > + * should be relatively low. ] > + * > + * So as a performance optimization don't flush the TLB when > + * clearing the accessed bit, it will eventually be flushed by > + * a context switch or a VM operation anyway. [ In the rare > + * event of it not getting flushed for a long time the delay > + * shouldn't really matter because there's no real memory > + * pressure for swapout to react to. ] > + */ > + return ptep_test_and_clear_young(vma, address, ptep); > } > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > -- > 2.39.3 > Thanks Barry