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 EAF13C43334 for ; Tue, 19 Jul 2022 07:20:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2716C6B0071; Tue, 19 Jul 2022 03:20:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 220046B0073; Tue, 19 Jul 2022 03:20:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0E9EB6B0074; Tue, 19 Jul 2022 03:20:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id F393A6B0071 for ; Tue, 19 Jul 2022 03:20:09 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C389717C3 for ; Tue, 19 Jul 2022 07:20:09 +0000 (UTC) X-FDA: 79703000538.05.F5FABC5 Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by imf05.hostedemail.com (Postfix) with ESMTP id A074110006D for ; Tue, 19 Jul 2022 07:20:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1658215207; x=1689751207; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=biaESJTSUbQQgecZrk3cqlebtNrxRUqpG5iCexsO7Fo=; b=BTT9IOPBZ9oAc2HJhJUrMlw9JS6keIZ0C+dN6CmqgQFbOFZk7Vi6WwXr 6nFGH152mfgIq7ywrRsgkpNRHXCAEdhdFFC2DOjfV+J9ygaMk4WAv+9jS QvEqplW6jmxdaj2VrpzlB3pIbh85LXidL1DX+loonM6sgp+HJwopPK/ZP BciNzPe5bBZSpWyj959075fbBtnUW9mX5cV+YiIFF1FfmOWvJLdxzkyvV 0p34GIw+gLjNz/QZEOqNOAZdDzBuFxZSZ8lWgXMG86wLnaEIra54DT/dQ XmKH4bTyI7uQQiBohwuBJSrYM3srlCOHunxZCHty6rQhIDp+e1H3RrQfl w==; X-IronPort-AV: E=McAfee;i="6400,9594,10412"; a="287567590" X-IronPort-AV: E=Sophos;i="5.92,283,1650956400"; d="scan'208";a="287567590" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jul 2022 00:20:05 -0700 X-IronPort-AV: E=Sophos;i="5.92,283,1650956400"; d="scan'208";a="630225340" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.239.13.94]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Jul 2022 00:20:01 -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> <87k0891uj6.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Tue, 19 Jul 2022 15:19:57 +0800 In-Reply-To: (Barry Song's message of "Tue, 19 Jul 2022 19:01:03 +1200") Message-ID: <875yjt1sde.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 ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1658215208; a=rsa-sha256; cv=none; b=72MdRN2MSbWE/yn/eUPklB0rLfPbgcnxHqkXBtezTmpcIfQBMz/yEMiATf7c++q5j2jHZg WMfdjMFPd9KpqiriJwJ+5EOG4nnoValeBrWV96WicdzWFUh3wO9xZmHfv1C5Ea3PxzFYjp LcqZkdQ86eszRgMickHmBV0h+uj/kSY= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BTT9IOPB; spf=none (imf05.hostedemail.com: domain of ying.huang@intel.com has no SPF policy when checking 134.134.136.65) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1658215208; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=3PmbG9QgiYJGCaRMnPOCqGGECNHYLpVwBf0+OdtfiCc=; b=BJWi5KI8HxuZubYPisX30UsuEySNMBJ94Bzlo7R0TJwwQ8YAR7u6wbReg4T/uSzj+F/5Ww YtCwf3cZhhfjmQpXiDvxnFP6AXxca3NtDTiS1nWEcZ4SFX0QRH7W/G2BtojNFm/V9s9t8O D9r13jMD2yWXeAneM6F0PbZIchRDBqE= X-Rspamd-Queue-Id: A074110006D Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=BTT9IOPB; spf=none (imf05.hostedemail.com: domain of ying.huang@intel.com has no SPF policy when checking 134.134.136.65) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com X-Rspam-User: X-Rspamd-Server: rspam06 X-Stat-Signature: p77otz7qbyoddgj57sj5hzgnenadux9c X-HE-Tag: 1658215207-675385 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: Barry Song <21cnbao@gmail.com> writes: > On Tue, Jul 19, 2022 at 6:33 PM Huang, Ying wrote: >> >> 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. > > My point is that having a generic thp_swp_supported() which can combine > both IS_ENABLED(CONFIG_THP_SWP) && arch_thp_swp_thing(). > then we can always only call this rather than checking two conditions. > Although there is only one caller for this moment, we might get more > later. So always calling this single function might make our life easier. > > we can treat > > static inline bool arch_thp_swp_supported(void) > { > return IS_ENABLED(CONFIG_THP_SWAP); > } The issue is that IS_ENABLED(CONFIG_THP_SWAP) reports that whether THP swap is selected by the user, not just whether THP swap is supported by the architecture. Best Regards, Huang, Ying > as a virtual function in base class. thp_swp is generically true if platforms > are able to enable CONFIG_THP_SWAP. > > Derived classes like arm64 can overwrite it to false in some particular cases > based on their unique characteristics. > >> >> > 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. > > Ok, fair enough. And thanks for your reviewing :-) > > Thanks > Barry