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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 873C9C43334 for ; Tue, 19 Jul 2022 06:33:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237041AbiGSGda (ORCPT ); Tue, 19 Jul 2022 02:33:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47668 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237036AbiGSGd0 (ORCPT ); Tue, 19 Jul 2022 02:33:26 -0400 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B824627167 for ; Mon, 18 Jul 2022 23:33:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658212404; x=1689748404; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=wFz1wL18KeXVJ5cGHIGIkpG2nND2KpkYSqet4mEbQpY=; b=na33nfPJ8dOD7+0bWT9Ct3u3hnwsiiCIuUKON6DNZPlDEEO44SZCBUQV pNaPmRLSRa2x2DOT4VT/EcwQLhQ+lxx7x8+hOoESJZuAUpBMaUT5Dh3zq CJxKWWI5h9Gc4NNp8DsISd7Z80xT5wPaSlGRvDk2W3bWFWZzM8yOFnbHF PJ8zqkpOXLYVw83sIbXRyRsD9Tf+uS1wlTnJPnNkF53mEXcmnTwDzXqeN B8H1U0HQd0zxrVecP3n8dc6V3qgTEm2aIwh6f+sypJKids4Bkjd6sV4xK vj7b7EA5/7P/89IdK3hGjyM+D6e8REiqPOkNr8qBMKZlkai3BKmBxQQ/s A==; X-IronPort-AV: E=McAfee;i="6400,9594,10412"; a="287557627" X-IronPort-AV: E=Sophos;i="5.92,283,1650956400"; d="scan'208";a="287557627" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 23:33:24 -0700 X-IronPort-AV: E=Sophos;i="5.92,283,1650956400"; d="scan'208";a="739750617" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.239.13.94]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 23:33:20 -0700 From: "Huang, Ying" To: Barry Song <21cnbao@gmail.com> Cc: Anshuman Khandual , Andrew Morton , Catalin Marinas , LAK , Linux-MM , Steven Price , Will Deacon , Andrea Arcangeli , =?utf-8?B?6YOt5YGl?= , hanchuanhua , Johannes Weiner , Hugh Dickins , LKML , Minchan Kim , Yang Shi , Barry Song , =?utf-8?B?5byg6K+X5piOKFNpbW9uIFpoYW5nKQ==?= Subject: Re: [RESEND PATCH v3] arm64: enable THP_SWAP for arm64 References: <20220718090050.2261-1-21cnbao@gmail.com> <87mtd62apo.fsf@yhuang6-desk2.ccr.corp.intel.com> <87zgh5232o.fsf@yhuang6-desk2.ccr.corp.intel.com> <416a06f6-ca7d-d4a9-2cda-af0ad6e28261@arm.com> <87o7xl1wpb.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Tue, 19 Jul 2022 14:33:17 +0800 In-Reply-To: (Barry Song's message of "Tue, 19 Jul 2022 18:13:32 +1200") Message-ID: <87k0891uj6.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Barry Song <21cnbao@gmail.com> writes: > On Tue, Jul 19, 2022 at 5:47 PM Huang, Ying wrote: >> >> Barry Song <21cnbao@gmail.com> writes: >> >> > On Tue, Jul 19, 2022 at 3:59 PM Barry Song <21cnbao@gmail.com> wrote: >> >> >> >> On Tue, Jul 19, 2022 at 3:35 PM Anshuman Khandual >> >> wrote: >> >> > >> >> > >> >> > >> >> > On 7/19/22 08:58, Huang, Ying wrote: >> >> > > Anshuman Khandual writes: >> >> > > >> >> > >> On 7/19/22 06:53, Barry Song wrote: >> >> > >>> On Tue, Jul 19, 2022 at 12:44 PM Huang, Ying wrote: >> >> > >>>> >> >> > >>>> Barry Song <21cnbao@gmail.com> writes: >> >> > >>>> >> >> > >>>>> From: Barry Song >> >> > >>>>> >> >> > >>>>> THP_SWAP has been proven to improve the swap throughput significantly >> >> > >>>>> on x86_64 according to commit bd4c82c22c367e ("mm, THP, swap: delay >> >> > >>>>> splitting THP after swapped out"). >> >> > >>>>> As long as arm64 uses 4K page size, it is quite similar with x86_64 >> >> > >>>>> by having 2MB PMD THP. THP_SWAP is architecture-independent, thus, >> >> > >>>>> enabling it on arm64 will benefit arm64 as well. >> >> > >>>>> A corner case is that MTE has an assumption that only base pages >> >> > >>>>> can be swapped. We won't enable THP_SWAP for ARM64 hardware with >> >> > >>>>> MTE support until MTE is reworked to coexist with THP_SWAP. >> >> > >>>>> >> >> > >>>>> A micro-benchmark is written to measure thp swapout throughput as >> >> > >>>>> below, >> >> > >>>>> >> >> > >>>>> unsigned long long tv_to_ms(struct timeval tv) >> >> > >>>>> { >> >> > >>>>> return tv.tv_sec * 1000 + tv.tv_usec / 1000; >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> main() >> >> > >>>>> { >> >> > >>>>> struct timeval tv_b, tv_e;; >> >> > >>>>> #define SIZE 400*1024*1024 >> >> > >>>>> volatile void *p = mmap(NULL, SIZE, PROT_READ | PROT_WRITE, >> >> > >>>>> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); >> >> > >>>>> if (!p) { >> >> > >>>>> perror("fail to get memory"); >> >> > >>>>> exit(-1); >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> madvise(p, SIZE, MADV_HUGEPAGE); >> >> > >>>>> memset(p, 0x11, SIZE); /* write to get mem */ >> >> > >>>>> >> >> > >>>>> gettimeofday(&tv_b, NULL); >> >> > >>>>> madvise(p, SIZE, MADV_PAGEOUT); >> >> > >>>>> gettimeofday(&tv_e, NULL); >> >> > >>>>> >> >> > >>>>> printf("swp out bandwidth: %ld bytes/ms\n", >> >> > >>>>> SIZE/(tv_to_ms(tv_e) - tv_to_ms(tv_b))); >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> Testing is done on rk3568 64bit quad core processor Quad Core >> >> > >>>>> Cortex-A55 platform - ROCK 3A. >> >> > >>>>> thp swp throughput w/o patch: 2734bytes/ms (mean of 10 tests) >> >> > >>>>> thp swp throughput w/ patch: 3331bytes/ms (mean of 10 tests) >> >> > >>>>> >> >> > >>>>> Cc: "Huang, Ying" >> >> > >>>>> Cc: Minchan Kim >> >> > >>>>> Cc: Johannes Weiner >> >> > >>>>> Cc: Hugh Dickins >> >> > >>>>> Cc: Andrea Arcangeli >> >> > >>>>> Cc: Anshuman Khandual >> >> > >>>>> Cc: Steven Price >> >> > >>>>> Cc: Yang Shi >> >> > >>>>> Signed-off-by: Barry Song >> >> > >>>>> --- >> >> > >>>>> -v3: >> >> > >>>>> * refine the commit log; >> >> > >>>>> * add a benchmark result; >> >> > >>>>> * refine the macro of arch_thp_swp_supported >> >> > >>>>> Thanks to the comments of Anshuman, Andrew, Steven >> >> > >>>>> >> >> > >>>>> arch/arm64/Kconfig | 1 + >> >> > >>>>> arch/arm64/include/asm/pgtable.h | 6 ++++++ >> >> > >>>>> include/linux/huge_mm.h | 12 ++++++++++++ >> >> > >>>>> mm/swap_slots.c | 2 +- >> >> > >>>>> 4 files changed, 20 insertions(+), 1 deletion(-) >> >> > >>>>> >> >> > >>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> >> > >>>>> index 1652a9800ebe..e1c540e80eec 100644 >> >> > >>>>> --- a/arch/arm64/Kconfig >> >> > >>>>> +++ b/arch/arm64/Kconfig >> >> > >>>>> @@ -101,6 +101,7 @@ config ARM64 >> >> > >>>>> select ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP >> >> > >>>>> select ARCH_WANT_LD_ORPHAN_WARN >> >> > >>>>> select ARCH_WANTS_NO_INSTR >> >> > >>>>> + select ARCH_WANTS_THP_SWAP if ARM64_4K_PAGES >> >> > >>>>> select ARCH_HAS_UBSAN_SANITIZE_ALL >> >> > >>>>> select ARM_AMBA >> >> > >>>>> select ARM_ARCH_TIMER >> >> > >>>>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >> >> > >>>>> index 0b6632f18364..78d6f6014bfb 100644 >> >> > >>>>> --- a/arch/arm64/include/asm/pgtable.h >> >> > >>>>> +++ b/arch/arm64/include/asm/pgtable.h >> >> > >>>>> @@ -45,6 +45,12 @@ >> >> > >>>>> __flush_tlb_range(vma, addr, end, PUD_SIZE, false, 1) >> >> > >>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ >> >> > >>>>> >> >> > >>>>> +static inline bool arch_thp_swp_supported(void) >> >> > >>>>> +{ >> >> > >>>>> + return !system_supports_mte(); >> >> > >>>>> +} >> >> > >>>>> +#define arch_thp_swp_supported arch_thp_swp_supported >> >> > >>>>> + >> >> > >>>>> /* >> >> > >>>>> * Outside of a few very special situations (e.g. hibernation), we always >> >> > >>>>> * use broadcast TLB invalidation instructions, therefore a spurious page >> >> > >>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >> >> > >>>>> index de29821231c9..4ddaf6ad73ef 100644 >> >> > >>>>> --- a/include/linux/huge_mm.h >> >> > >>>>> +++ b/include/linux/huge_mm.h >> >> > >>>>> @@ -461,4 +461,16 @@ static inline int split_folio_to_list(struct folio *folio, >> >> > >>>>> return split_huge_page_to_list(&folio->page, list); >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> +/* >> >> > >>>>> + * archs that select ARCH_WANTS_THP_SWAP but don't support THP_SWP due to >> >> > >>>>> + * limitations in the implementation like arm64 MTE can override this to >> >> > >>>>> + * false >> >> > >>>>> + */ >> >> > >>>>> +#ifndef arch_thp_swp_supported >> >> > >>>>> +static inline bool arch_thp_swp_supported(void) >> >> > >>>>> +{ >> >> > >>>>> + return true; >> >> > >>>>> +} >> >> > >>>> >> >> > >>>> How about the following? >> >> > >>>> >> >> > >>>> static inline bool arch_wants_thp_swap(void) >> >> > >>>> { >> >> > >>>> return IS_ENABLED(ARCH_WANTS_THP_SWAP); >> >> > >>>> } >> >> > >>> >> >> > >>> This looks good. then i'll need to change arm64 to >> >> > >>> >> >> > >>> +static inline bool arch_thp_swp_supported(void) >> >> > >>> +{ >> >> > >>> + return IS_ENABLED(ARCH_WANTS_THP_SWAP) && !system_supports_mte(); >> >> > >>> +} >> >> > >> >> >> > >> Why ? CONFIG_THP_SWAP depends on ARCH_WANTS_THP_SWAP. In folio_alloc_swap(), >> >> > >> IS_ENABLED(CONFIG_THP_SWAP) enabled, will also imply ARCH_WANTS_THP_SWAP too >> >> > >> is enabled. Hence checking for ARCH_WANTS_THP_SWAP again does not make sense >> >> > >> either in the generic fallback stub, or in arm64 platform override. Because >> >> > >> without ARCH_WANTS_THP_SWAP enabled, arch_thp_swp_supported() should never >> >> > >> be called in the first place. >> >> > > >> >> > > For the only caller now, the checking looks redundant. But the original >> >> > > proposed implementation as follows, >> >> > > >> >> > > static inline bool arch_thp_swp_supported(void) >> >> > > { >> >> > > return true; >> >> > > } >> >> > > >> >> > > will return true even on architectures that don't support/want THP swap. >> >> > >> >> > But the function will never be called on for those platforms. >> >> > >> >> > > That will confuse people too. >> >> > >> >> > I dont see how. >> >> > >> >> > > >> >> > > And the "redundant" checking has no run time overhead, because compiler >> >> > > will do the trick. >> >> > I understand that, but dont think this indirection is necessary. >> >> >> >> Hi Anshuman, Hi Ying, >> >> Thanks for the comments of both of you. Does the below look ok? >> >> >> >> generic, >> >> >> >> static inline bool arch_wants_thp_swap(void) >> >> { >> >> return IS_ENABLED(CONFIG_THP_SWAP); >> >> } >> >> >> > >> > sorry, i actually meant arch_thp_swp_supported() but not >> > arch_wants_thp_swap() in generic code, >> > >> > static inline bool arch_thp_swp_supported(void) >> > { >> > return IS_ENABLED(CONFIG_THP_SWAP); >> > } >> >> IS_ENABLED(CONFIG_THP_SWAP) doesn't match the name too. It's an option >> selected by users. arch_thp_swp_supported() is to report the >> capability. > > Hi Ying, > CONFIG_THP_SWAP implicitly includes ARCH_WANTS_THP_SWAP. So it seems > a bit odd to have still another arch_wants_thp_swap(). > if the name of arch_thp_swp_supported is not sensible to you, will > thp_swp_supported() > without arch_ make more sense? a similar example is, > > static inline bool gigantic_page_runtime_supported(void) > { > return IS_ENABLED(CONFIG_ARCH_HAS_GIGANTIC_PAGE); > } Here, the capability of the architecture is reported. But CONFIG_THP_SWAP is a user option. I'm OK with the function name "arch_thp_swp_supported()". I just think that we should implement the function in a way that is consistent with the function name as much as possible. That is, don't return true on architectures that THP swap isn't supported in fact. > Otherwise, can we just keep the code as is according to Anshuman's suggestion? Although I still think my way is better, I will not force you to do that. If you don't think that is better, you can use your original implementation. Best Regards, Huang, Ying > Thanks > Barry > > } > > > >> >> Best Regards, >> Huang, Ying >> >> >> arm64, >> >> >> >> static inline bool arch_thp_swp_supported(void) >> >> { >> >> return IS_ENABLED(CONFIG_THP_SWAP) && !system_supports_mte(); >> >> } >> >> >> >> caller, >> >> >> >> folio_alloc_swap(struct folio *folio) >> >> { >> >> >> >> if (folio_test_large(folio)) { >> >> - if (IS_ENABLED(CONFIG_THP_SWAP)) >> >> + if (arch_thp_swp_supported()) >> >> get_swap_pages(1, &entry, folio_nr_pages(folio)); >> >> goto out; >> >> } >> >> >> >> Thanks >> >> Barry 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 53066C433EF for ; Tue, 19 Jul 2022 06:34:44 +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:MIME-Version:Message-ID:In-Reply-To: Date:References:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=lqgkGyAzVr5YsyTDMCF9M0BcHUJjwuYHzg1gXIxUqxY=; b=u7q8o4mBHpUNsw +dBjZwXMS5WFNsyJ8FumuLCa05XwJubgH0x+eToH7FgFT0MBxoUASQLh3aWaDzm19vK2Zw4FD8+B+ +G/MYXNETOElKpfUNPaCDYwr81GY3zzWTtzuAEgUK5MbXsZ5J5cRqVII+HvXiZq4D7qhkwzl5fP2U JC599xIdKdCyolvJ13Rouqk20TQiSxWCy68BScVo0ft8zmb++JC9YXOlmIdxkxKkAC4EwMgWN1fBR wbvCD0rvfGDSOndkeVocMzAaL84bs7M3who6+h/2Y0bQQTfpWiTQ5D5WZY0ML3hPKX1KUpMPg80g0 hpp9aFIQe7I9D+2Lne0A==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oDgnS-005fxn-CY; Tue, 19 Jul 2022 06:33:30 +0000 Received: from mga03.intel.com ([134.134.136.65]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oDgnP-005fwj-He for linux-arm-kernel@lists.infradead.org; Tue, 19 Jul 2022 06:33:29 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658212407; x=1689748407; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=wFz1wL18KeXVJ5cGHIGIkpG2nND2KpkYSqet4mEbQpY=; b=nR68QOFFvOFsFJUf94kT/7l+tLZGtgXYRgeKeuRixQ63aoL64vZtl6Uu QBuU9a/qaEJt7qnLtx0g9boxsykU17bOtIuTH9bQEe28jN+z+nooat/gH G6zhZSl12CI5ZCWgtTvTImhwHATiUBcUJmZT7+zdEp7DmKnvixWPnJJfV t64bsDfuriTOR+V7c5zxOVBK4HYUc6ea/wtRMz5OZ+zgRxgqd1IASkTtd IruE4V1EdHzkUvbDfl5/ikq7YywwYk3UEIQ7bsFC6TtJ/NSlTlIbo38oE mdHAsIaJUAh9tE028IQyPUsweGAZnJfn6Bv1iQDzl14OT0EC5lakhfT/b g==; X-IronPort-AV: E=McAfee;i="6400,9594,10412"; a="287557628" X-IronPort-AV: E=Sophos;i="5.92,283,1650956400"; d="scan'208";a="287557628" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 23:33:24 -0700 X-IronPort-AV: E=Sophos;i="5.92,283,1650956400"; d="scan'208";a="739750617" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.239.13.94]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Jul 2022 23:33:20 -0700 From: "Huang, Ying" To: Barry Song <21cnbao@gmail.com> Cc: Anshuman Khandual , Andrew Morton , Catalin Marinas , LAK , Linux-MM , Steven Price , Will Deacon , Andrea Arcangeli , =?utf-8?B?6YOt5YGl?= , hanchuanhua , Johannes Weiner , Hugh Dickins , LKML , Minchan Kim , Yang Shi , Barry Song , =?utf-8?B?5byg6K+X5piOKFNpbW9uIFpoYW5nKQ==?= Subject: Re: [RESEND PATCH v3] arm64: enable THP_SWAP for arm64 References: <20220718090050.2261-1-21cnbao@gmail.com> <87mtd62apo.fsf@yhuang6-desk2.ccr.corp.intel.com> <87zgh5232o.fsf@yhuang6-desk2.ccr.corp.intel.com> <416a06f6-ca7d-d4a9-2cda-af0ad6e28261@arm.com> <87o7xl1wpb.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Tue, 19 Jul 2022 14:33:17 +0800 In-Reply-To: (Barry Song's message of "Tue, 19 Jul 2022 18:13:32 +1200") Message-ID: <87k0891uj6.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220718_233327_654223_7CA15BD0 X-CRM114-Status: GOOD ( 39.39 ) 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 Barry Song <21cnbao@gmail.com> writes: > On Tue, Jul 19, 2022 at 5:47 PM Huang, Ying wrote: >> >> Barry Song <21cnbao@gmail.com> writes: >> >> > On Tue, Jul 19, 2022 at 3:59 PM Barry Song <21cnbao@gmail.com> wrote: >> >> >> >> On Tue, Jul 19, 2022 at 3:35 PM Anshuman Khandual >> >> wrote: >> >> > >> >> > >> >> > >> >> > On 7/19/22 08:58, Huang, Ying wrote: >> >> > > Anshuman Khandual writes: >> >> > > >> >> > >> On 7/19/22 06:53, Barry Song wrote: >> >> > >>> On Tue, Jul 19, 2022 at 12:44 PM Huang, Ying wrote: >> >> > >>>> >> >> > >>>> Barry Song <21cnbao@gmail.com> writes: >> >> > >>>> >> >> > >>>>> From: Barry Song >> >> > >>>>> >> >> > >>>>> THP_SWAP has been proven to improve the swap throughput significantly >> >> > >>>>> on x86_64 according to commit bd4c82c22c367e ("mm, THP, swap: delay >> >> > >>>>> splitting THP after swapped out"). >> >> > >>>>> As long as arm64 uses 4K page size, it is quite similar with x86_64 >> >> > >>>>> by having 2MB PMD THP. THP_SWAP is architecture-independent, thus, >> >> > >>>>> enabling it on arm64 will benefit arm64 as well. >> >> > >>>>> A corner case is that MTE has an assumption that only base pages >> >> > >>>>> can be swapped. We won't enable THP_SWAP for ARM64 hardware with >> >> > >>>>> MTE support until MTE is reworked to coexist with THP_SWAP. >> >> > >>>>> >> >> > >>>>> A micro-benchmark is written to measure thp swapout throughput as >> >> > >>>>> below, >> >> > >>>>> >> >> > >>>>> unsigned long long tv_to_ms(struct timeval tv) >> >> > >>>>> { >> >> > >>>>> return tv.tv_sec * 1000 + tv.tv_usec / 1000; >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> main() >> >> > >>>>> { >> >> > >>>>> struct timeval tv_b, tv_e;; >> >> > >>>>> #define SIZE 400*1024*1024 >> >> > >>>>> volatile void *p = mmap(NULL, SIZE, PROT_READ | PROT_WRITE, >> >> > >>>>> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); >> >> > >>>>> if (!p) { >> >> > >>>>> perror("fail to get memory"); >> >> > >>>>> exit(-1); >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> madvise(p, SIZE, MADV_HUGEPAGE); >> >> > >>>>> memset(p, 0x11, SIZE); /* write to get mem */ >> >> > >>>>> >> >> > >>>>> gettimeofday(&tv_b, NULL); >> >> > >>>>> madvise(p, SIZE, MADV_PAGEOUT); >> >> > >>>>> gettimeofday(&tv_e, NULL); >> >> > >>>>> >> >> > >>>>> printf("swp out bandwidth: %ld bytes/ms\n", >> >> > >>>>> SIZE/(tv_to_ms(tv_e) - tv_to_ms(tv_b))); >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> Testing is done on rk3568 64bit quad core processor Quad Core >> >> > >>>>> Cortex-A55 platform - ROCK 3A. >> >> > >>>>> thp swp throughput w/o patch: 2734bytes/ms (mean of 10 tests) >> >> > >>>>> thp swp throughput w/ patch: 3331bytes/ms (mean of 10 tests) >> >> > >>>>> >> >> > >>>>> Cc: "Huang, Ying" >> >> > >>>>> Cc: Minchan Kim >> >> > >>>>> Cc: Johannes Weiner >> >> > >>>>> Cc: Hugh Dickins >> >> > >>>>> Cc: Andrea Arcangeli >> >> > >>>>> Cc: Anshuman Khandual >> >> > >>>>> Cc: Steven Price >> >> > >>>>> Cc: Yang Shi >> >> > >>>>> Signed-off-by: Barry Song >> >> > >>>>> --- >> >> > >>>>> -v3: >> >> > >>>>> * refine the commit log; >> >> > >>>>> * add a benchmark result; >> >> > >>>>> * refine the macro of arch_thp_swp_supported >> >> > >>>>> Thanks to the comments of Anshuman, Andrew, Steven >> >> > >>>>> >> >> > >>>>> arch/arm64/Kconfig | 1 + >> >> > >>>>> arch/arm64/include/asm/pgtable.h | 6 ++++++ >> >> > >>>>> include/linux/huge_mm.h | 12 ++++++++++++ >> >> > >>>>> mm/swap_slots.c | 2 +- >> >> > >>>>> 4 files changed, 20 insertions(+), 1 deletion(-) >> >> > >>>>> >> >> > >>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> >> > >>>>> index 1652a9800ebe..e1c540e80eec 100644 >> >> > >>>>> --- a/arch/arm64/Kconfig >> >> > >>>>> +++ b/arch/arm64/Kconfig >> >> > >>>>> @@ -101,6 +101,7 @@ config ARM64 >> >> > >>>>> select ARCH_WANT_HUGETLB_PAGE_OPTIMIZE_VMEMMAP >> >> > >>>>> select ARCH_WANT_LD_ORPHAN_WARN >> >> > >>>>> select ARCH_WANTS_NO_INSTR >> >> > >>>>> + select ARCH_WANTS_THP_SWAP if ARM64_4K_PAGES >> >> > >>>>> select ARCH_HAS_UBSAN_SANITIZE_ALL >> >> > >>>>> select ARM_AMBA >> >> > >>>>> select ARM_ARCH_TIMER >> >> > >>>>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h >> >> > >>>>> index 0b6632f18364..78d6f6014bfb 100644 >> >> > >>>>> --- a/arch/arm64/include/asm/pgtable.h >> >> > >>>>> +++ b/arch/arm64/include/asm/pgtable.h >> >> > >>>>> @@ -45,6 +45,12 @@ >> >> > >>>>> __flush_tlb_range(vma, addr, end, PUD_SIZE, false, 1) >> >> > >>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ >> >> > >>>>> >> >> > >>>>> +static inline bool arch_thp_swp_supported(void) >> >> > >>>>> +{ >> >> > >>>>> + return !system_supports_mte(); >> >> > >>>>> +} >> >> > >>>>> +#define arch_thp_swp_supported arch_thp_swp_supported >> >> > >>>>> + >> >> > >>>>> /* >> >> > >>>>> * Outside of a few very special situations (e.g. hibernation), we always >> >> > >>>>> * use broadcast TLB invalidation instructions, therefore a spurious page >> >> > >>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >> >> > >>>>> index de29821231c9..4ddaf6ad73ef 100644 >> >> > >>>>> --- a/include/linux/huge_mm.h >> >> > >>>>> +++ b/include/linux/huge_mm.h >> >> > >>>>> @@ -461,4 +461,16 @@ static inline int split_folio_to_list(struct folio *folio, >> >> > >>>>> return split_huge_page_to_list(&folio->page, list); >> >> > >>>>> } >> >> > >>>>> >> >> > >>>>> +/* >> >> > >>>>> + * archs that select ARCH_WANTS_THP_SWAP but don't support THP_SWP due to >> >> > >>>>> + * limitations in the implementation like arm64 MTE can override this to >> >> > >>>>> + * false >> >> > >>>>> + */ >> >> > >>>>> +#ifndef arch_thp_swp_supported >> >> > >>>>> +static inline bool arch_thp_swp_supported(void) >> >> > >>>>> +{ >> >> > >>>>> + return true; >> >> > >>>>> +} >> >> > >>>> >> >> > >>>> How about the following? >> >> > >>>> >> >> > >>>> static inline bool arch_wants_thp_swap(void) >> >> > >>>> { >> >> > >>>> return IS_ENABLED(ARCH_WANTS_THP_SWAP); >> >> > >>>> } >> >> > >>> >> >> > >>> This looks good. then i'll need to change arm64 to >> >> > >>> >> >> > >>> +static inline bool arch_thp_swp_supported(void) >> >> > >>> +{ >> >> > >>> + return IS_ENABLED(ARCH_WANTS_THP_SWAP) && !system_supports_mte(); >> >> > >>> +} >> >> > >> >> >> > >> Why ? CONFIG_THP_SWAP depends on ARCH_WANTS_THP_SWAP. In folio_alloc_swap(), >> >> > >> IS_ENABLED(CONFIG_THP_SWAP) enabled, will also imply ARCH_WANTS_THP_SWAP too >> >> > >> is enabled. Hence checking for ARCH_WANTS_THP_SWAP again does not make sense >> >> > >> either in the generic fallback stub, or in arm64 platform override. Because >> >> > >> without ARCH_WANTS_THP_SWAP enabled, arch_thp_swp_supported() should never >> >> > >> be called in the first place. >> >> > > >> >> > > For the only caller now, the checking looks redundant. But the original >> >> > > proposed implementation as follows, >> >> > > >> >> > > static inline bool arch_thp_swp_supported(void) >> >> > > { >> >> > > return true; >> >> > > } >> >> > > >> >> > > will return true even on architectures that don't support/want THP swap. >> >> > >> >> > But the function will never be called on for those platforms. >> >> > >> >> > > That will confuse people too. >> >> > >> >> > I dont see how. >> >> > >> >> > > >> >> > > And the "redundant" checking has no run time overhead, because compiler >> >> > > will do the trick. >> >> > I understand that, but dont think this indirection is necessary. >> >> >> >> Hi Anshuman, Hi Ying, >> >> Thanks for the comments of both of you. Does the below look ok? >> >> >> >> generic, >> >> >> >> static inline bool arch_wants_thp_swap(void) >> >> { >> >> return IS_ENABLED(CONFIG_THP_SWAP); >> >> } >> >> >> > >> > sorry, i actually meant arch_thp_swp_supported() but not >> > arch_wants_thp_swap() in generic code, >> > >> > static inline bool arch_thp_swp_supported(void) >> > { >> > return IS_ENABLED(CONFIG_THP_SWAP); >> > } >> >> IS_ENABLED(CONFIG_THP_SWAP) doesn't match the name too. It's an option >> selected by users. arch_thp_swp_supported() is to report the >> capability. > > Hi Ying, > CONFIG_THP_SWAP implicitly includes ARCH_WANTS_THP_SWAP. So it seems > a bit odd to have still another arch_wants_thp_swap(). > if the name of arch_thp_swp_supported is not sensible to you, will > thp_swp_supported() > without arch_ make more sense? a similar example is, > > static inline bool gigantic_page_runtime_supported(void) > { > return IS_ENABLED(CONFIG_ARCH_HAS_GIGANTIC_PAGE); > } Here, the capability of the architecture is reported. But CONFIG_THP_SWAP is a user option. I'm OK with the function name "arch_thp_swp_supported()". I just think that we should implement the function in a way that is consistent with the function name as much as possible. That is, don't return true on architectures that THP swap isn't supported in fact. > Otherwise, can we just keep the code as is according to Anshuman's suggestion? Although I still think my way is better, I will not force you to do that. If you don't think that is better, you can use your original implementation. Best Regards, Huang, Ying > Thanks > Barry > > } > > > >> >> Best Regards, >> Huang, Ying >> >> >> arm64, >> >> >> >> static inline bool arch_thp_swp_supported(void) >> >> { >> >> return IS_ENABLED(CONFIG_THP_SWAP) && !system_supports_mte(); >> >> } >> >> >> >> caller, >> >> >> >> folio_alloc_swap(struct folio *folio) >> >> { >> >> >> >> if (folio_test_large(folio)) { >> >> - if (IS_ENABLED(CONFIG_THP_SWAP)) >> >> + if (arch_thp_swp_supported()) >> >> get_swap_pages(1, &entry, folio_nr_pages(folio)); >> >> goto out; >> >> } >> >> >> >> Thanks >> >> Barry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel