On 05/09/2020 03:39 AM, Mike Kravetz wrote:

On 5/7/20 8:07 PM, Anshuman Khandual wrote:

quoted

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/ (local)

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 <redacted>
---
 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