From: Anshuman Khandual <anshuman.khandual@arm.com> To: Mike Kravetz <mike.kravetz@oracle.com>, linux-mm@kvack.org, akpm@linux-foundation.org Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V3 1/3] arm64/mm: Drop __HAVE_ARCH_HUGE_PTEP_GET Date: Mon, 11 May 2020 09:32:17 +0530 [thread overview] Message-ID: <19ffbe33-3a42-6d90-6c48-19645a898383@arm.com> (raw) In-Reply-To: <7db44202-0d21-d8fb-6998-0210508a488a@oracle.com> On 05/09/2020 03:39 AM, Mike Kravetz wrote: > On 5/7/20 8:07 PM, Anshuman Khandual wrote: >> Platform specific huge_ptep_get() is required only when fetching the huge >> PTE involves more than just dereferencing the page table pointer. This is >> not the case on arm64 platform. Hence huge_ptep_pte() can be dropped along >> with it's __HAVE_ARCH_HUGE_PTEP_GET subscription. Before that, it updates >> the generic huge_ptep_get() with READ_ONCE() which will prevent known page >> table issues with THP on arm64. >> >> https://lore.kernel.org/r/1506527369-19535-1-git-send-email-will.deacon@arm.com/ >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> arch/arm64/include/asm/hugetlb.h | 6 ------ >> include/asm-generic/hugetlb.h | 2 +- >> 2 files changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h >> index 2eb6c234d594..b88878ddc88b 100644 >> --- a/arch/arm64/include/asm/hugetlb.h >> +++ b/arch/arm64/include/asm/hugetlb.h >> @@ -17,12 +17,6 @@ >> extern bool arch_hugetlb_migration_supported(struct hstate *h); >> #endif >> >> -#define __HAVE_ARCH_HUGE_PTEP_GET >> -static inline pte_t huge_ptep_get(pte_t *ptep) >> -{ >> - return READ_ONCE(*ptep); >> -} >> - >> static inline int is_hugepage_only_range(struct mm_struct *mm, >> unsigned long addr, unsigned long len) >> { >> diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h >> index 822f433ac95c..40f85decc2ee 100644 >> --- a/include/asm-generic/hugetlb.h >> +++ b/include/asm-generic/hugetlb.h >> @@ -122,7 +122,7 @@ static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma, >> #ifndef __HAVE_ARCH_HUGE_PTEP_GET >> static inline pte_t huge_ptep_get(pte_t *ptep) >> { >> - return *ptep; >> + return READ_ONCE(*ptep); >> } >> #endif > > I know you made this change in response to Will's comment. And, since > changes were made to consistently use READ_ONCE in arm64 code, it makes > sense for that architecture. > > However, with this change to generic code, you introduce READ_ONCE to > other architectures where it was not used before. Could this possibly > introduce inconsistencies in their use of READ_ONCE? To be honest, I > am not very good at identifying any possible issues this could cause. > However, it does seem possible. Could you please give some more details. Is there any particular problem which might be caused by this new READ_ONCE() here, that you you are concerned about. READ_ONCE() is already getting used in multiple places in core MM which can not be configured out (like mm/gup.c). It is getting used in core HugeTLB (mm/hugetlb.c) as well. AFAICS, there is no standard for using READ_ONCE() while walking page tables entries. We have examples in core MM for both ways. > > Will was nervous about dropping this from arm64. I'm just a little nervous > about adding it to other architectures. > AFAICS, __HAVE_ARCH_HUGE_PTEP_GET should be used on a platform only when a HugeTLB entry could not constructed by dereferencing a page table entry as in the case with ARM (32 bit). Using READ_ONCE() while dereferencing is really not a special case that will need __HAVE_ARCH_HUGE_PTEP_GET. Moving READ_ONCE() into generic definition solves the problem while also taking care of a known problem on arm64. IMHO, it seems like the right thing to do unless there is another problem that pops up some where else because of READ_ONCE().
WARNING: multiple messages have this Message-ID (diff)
From: Anshuman Khandual <anshuman.khandual@arm.com> To: Mike Kravetz <mike.kravetz@oracle.com>, linux-mm@kvack.org, akpm@linux-foundation.org Cc: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH V3 1/3] arm64/mm: Drop __HAVE_ARCH_HUGE_PTEP_GET Date: Mon, 11 May 2020 09:32:17 +0530 [thread overview] Message-ID: <19ffbe33-3a42-6d90-6c48-19645a898383@arm.com> (raw) In-Reply-To: <7db44202-0d21-d8fb-6998-0210508a488a@oracle.com> On 05/09/2020 03:39 AM, Mike Kravetz wrote: > On 5/7/20 8:07 PM, Anshuman Khandual wrote: >> Platform specific huge_ptep_get() is required only when fetching the huge >> PTE involves more than just dereferencing the page table pointer. This is >> not the case on arm64 platform. Hence huge_ptep_pte() can be dropped along >> with it's __HAVE_ARCH_HUGE_PTEP_GET subscription. Before that, it updates >> the generic huge_ptep_get() with READ_ONCE() which will prevent known page >> table issues with THP on arm64. >> >> https://lore.kernel.org/r/1506527369-19535-1-git-send-email-will.deacon@arm.com/ >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-mm@kvack.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> arch/arm64/include/asm/hugetlb.h | 6 ------ >> include/asm-generic/hugetlb.h | 2 +- >> 2 files changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h >> index 2eb6c234d594..b88878ddc88b 100644 >> --- a/arch/arm64/include/asm/hugetlb.h >> +++ b/arch/arm64/include/asm/hugetlb.h >> @@ -17,12 +17,6 @@ >> extern bool arch_hugetlb_migration_supported(struct hstate *h); >> #endif >> >> -#define __HAVE_ARCH_HUGE_PTEP_GET >> -static inline pte_t huge_ptep_get(pte_t *ptep) >> -{ >> - return READ_ONCE(*ptep); >> -} >> - >> static inline int is_hugepage_only_range(struct mm_struct *mm, >> unsigned long addr, unsigned long len) >> { >> diff --git a/include/asm-generic/hugetlb.h b/include/asm-generic/hugetlb.h >> index 822f433ac95c..40f85decc2ee 100644 >> --- a/include/asm-generic/hugetlb.h >> +++ b/include/asm-generic/hugetlb.h >> @@ -122,7 +122,7 @@ static inline int huge_ptep_set_access_flags(struct vm_area_struct *vma, >> #ifndef __HAVE_ARCH_HUGE_PTEP_GET >> static inline pte_t huge_ptep_get(pte_t *ptep) >> { >> - return *ptep; >> + return READ_ONCE(*ptep); >> } >> #endif > > I know you made this change in response to Will's comment. And, since > changes were made to consistently use READ_ONCE in arm64 code, it makes > sense for that architecture. > > However, with this change to generic code, you introduce READ_ONCE to > other architectures where it was not used before. Could this possibly > introduce inconsistencies in their use of READ_ONCE? To be honest, I > am not very good at identifying any possible issues this could cause. > However, it does seem possible. Could you please give some more details. Is there any particular problem which might be caused by this new READ_ONCE() here, that you you are concerned about. READ_ONCE() is already getting used in multiple places in core MM which can not be configured out (like mm/gup.c). It is getting used in core HugeTLB (mm/hugetlb.c) as well. AFAICS, there is no standard for using READ_ONCE() while walking page tables entries. We have examples in core MM for both ways. > > Will was nervous about dropping this from arm64. I'm just a little nervous > about adding it to other architectures. > AFAICS, __HAVE_ARCH_HUGE_PTEP_GET should be used on a platform only when a HugeTLB entry could not constructed by dereferencing a page table entry as in the case with ARM (32 bit). Using READ_ONCE() while dereferencing is really not a special case that will need __HAVE_ARCH_HUGE_PTEP_GET. Moving READ_ONCE() into generic definition solves the problem while also taking care of a known problem on arm64. IMHO, it seems like the right thing to do unless there is another problem that pops up some where else because of READ_ONCE(). _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-05-11 4:02 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-08 3:07 [PATCH V3 0/3] mm/hugetlb: Add some new generic fallbacks Anshuman Khandual 2020-05-08 3:19 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` [PATCH V3 1/3] arm64/mm: Drop __HAVE_ARCH_HUGE_PTEP_GET Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 22:09 ` Mike Kravetz 2020-05-08 22:09 ` Mike Kravetz 2020-05-11 4:02 ` Anshuman Khandual [this message] 2020-05-11 4:02 ` Anshuman Khandual 2020-05-11 18:49 ` Mike Kravetz 2020-05-11 18:49 ` Mike Kravetz 2020-05-08 3:07 ` [PATCH V3 2/3] mm/hugetlb: Define a generic fallback for is_hugepage_only_range() Anshuman Khandual 2020-05-08 3:19 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 22:22 ` Mike Kravetz 2020-05-08 22:22 ` Mike Kravetz 2020-05-08 22:22 ` Mike Kravetz 2020-05-08 22:22 ` Mike Kravetz 2020-05-08 22:22 ` Mike Kravetz 2020-05-08 22:22 ` Mike Kravetz 2020-05-11 3:14 ` Anshuman Khandual 2020-05-11 3:26 ` Anshuman Khandual 2020-05-11 3:14 ` Anshuman Khandual 2020-05-11 3:14 ` Anshuman Khandual 2020-05-11 3:14 ` Anshuman Khandual 2020-05-11 3:14 ` Anshuman Khandual 2020-05-11 18:52 ` Mike Kravetz 2020-05-11 18:52 ` Mike Kravetz 2020-05-11 18:52 ` Mike Kravetz 2020-05-11 18:52 ` Mike Kravetz 2020-05-11 18:52 ` Mike Kravetz 2020-05-11 18:52 ` Mike Kravetz 2020-05-08 3:07 ` [PATCH V3 3/3] mm/hugetlb: Define a generic fallback for arch_clear_hugepage_flags() Anshuman Khandual 2020-05-08 3:19 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-08 3:07 ` Anshuman Khandual 2020-05-11 20:22 ` Mike Kravetz 2020-05-11 20:22 ` Mike Kravetz 2020-05-11 20:22 ` Mike Kravetz 2020-05-11 20:22 ` Mike Kravetz 2020-05-11 20:22 ` Mike Kravetz 2020-05-11 20:22 ` Mike Kravetz
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=19ffbe33-3a42-6d90-6c48-19645a898383@arm.com \ --to=anshuman.khandual@arm.com \ --cc=akpm@linux-foundation.org \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mike.kravetz@oracle.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.