mm: add page_check_address_transhuge helper

PATCH In review Vladimir Davydov 4 participants 9 messages Nov 20, 2015 – Nov 24, 2015

Subsystems (3)

Memory management MM misc MM rmap

lore

Share & tools

lore

Vladimir Davydov

author

2015-11-20 09:32 UTC #0 lore raw

PATCH v2 1/1 mm: add page_check_address_transhuge helper

page_referenced_one() and page_idle_clear_pte_refs_one() duplicate the code for looking up pte of a (possibly transhuge) page. Move this code to a new helper function, page_check_address_transhuge(), and make the above mentioned functions use it.

This is just a cleanup, no functional changes are intended.

Signed-off-by Vladimir Davydov <[email protected]>

Reviewed-by Kirill A. Shutemov <[email protected]>

---
Changes in v2:
- rebase on top of v4.4-rc1-mmotm-2015-11-18-15-38
- fix comment to page_check_address_transhuge as suggested by Kirill

 include/linux/rmap.h |   8 ++++
 mm/page_idle.c       |  63 ++++-------------------------
 mm/rmap.c            | 110 ++++++++++++++++++++++++++++++---------------------
 3 files changed, 81 insertions(+), 100 deletions(-)

include/linux/rmap.h +8 -0

index ebf3750e42b2..b9eedc63e9e6 100644
@@ -216,6 +216,14 @@ static inline pte_t *page_check_address(struct page *page, struct mm_struct *mm,
 }
 
 /*
+ * Used by idle page tracking to check if a page was referenced via page
+ * tables.
+ */
+bool page_check_address_transhuge(struct page *page, struct mm_struct *mm,
+				  unsigned long address, pmd_t **pmdp,
+				  pte_t **ptep, spinlock_t **ptlp);
+
+/*
  * Used by swapoff to help locate where page is expected in vma.
  */
 unsigned long page_address_in_vma(struct page *, struct vm_area_struct *);

mm/page_idle.c +7 ~~-56~~

78 lines not on this page · raw message

mm/rmap.c +66 ~~-44~~

180 lines not on this page · raw message

Signature · 7 lines

2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sergey Senozhatsky

2015-11-24 04:28 UTC #1 lore raw

Hello,

On (11/20/15 12:31), Vladimir Davydov wrote:
[..]

Quoted 16 lines

mm/rmap.c +7 -9

-	if (ptep_clear_flush_young_notify(vma, address, pte)) {
-		/*
-		 * Don't treat a reference through a sequentially read
-		 * mapping as such.  If the page has been used in
-		 * another mapping, we will catch it; if this other
-		 * mapping is already gone, the unmap path will have
-		 * set PG_referenced or activated the page.
-		 */
-		if (likely(!(vma->vm_flags & VM_SEQ_READ)))
+	if (pte) {
+		if (ptep_clear_flush_young_notify(vma, address, pte)) {
+			/*
+			 * Don't treat a reference through a sequentially read
+			 * mapping as such.  If the page has been used in
+			 * another mapping, we will catch it; if this other
+			 * mapping is already gone, the unmap path will have

mm/rmap.c check_pte: · line 935

Quoted 10 lines

mm/rmap.c +8 -0

+			 * set PG_referenced or activated the page.
+			 */
+			if (likely(!(vma->vm_flags & VM_SEQ_READ)))
+				referenced++;
+		}
+		pte_unmap(pte);
+	} else {
+		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
 	}

# CONFIG_TRANSPARENT_HUGEPAGE is not set

x86_64, 4.4.0-rc2-mm1

mm/built-in.o: In function `page_referenced_one':
rmap.c:(.text+0x32070): undefined reference to `pmdp_clear_flush_young'

-ss

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Kirill A. Shutemov

2015-11-24 09:09 UTC #2 lore raw

Quoted 33 lines · Sergey Senozhatsky

Hello,

On (11/20/15 12:31), Vladimir Davydov wrote:
[..]

Quoted 16 lines

(quoted diff) +7 -9

-	if (ptep_clear_flush_young_notify(vma, address, pte)) {
-		/*
-		 * Don't treat a reference through a sequentially read
-		 * mapping as such.  If the page has been used in
-		 * another mapping, we will catch it; if this other
-		 * mapping is already gone, the unmap path will have
-		 * set PG_referenced or activated the page.
-		 */
-		if (likely(!(vma->vm_flags & VM_SEQ_READ)))
+	if (pte) {
+		if (ptep_clear_flush_young_notify(vma, address, pte)) {
+			/*
+			 * Don't treat a reference through a sequentially read
+			 * mapping as such.  If the page has been used in
+			 * another mapping, we will catch it; if this other
+			 * mapping is already gone, the unmap path will have

Review

Quoted 10 lines

(quoted diff) +8 -0

+			 * set PG_referenced or activated the page.
+			 */
+			if (likely(!(vma->vm_flags & VM_SEQ_READ)))
+				referenced++;
+		}
+		pte_unmap(pte);
+	} else {
+		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
 	}

# CONFIG_TRANSPARENT_HUGEPAGE is not set

Quoted 5 lines

x86_64, 4.4.0-rc2-mm1
mm/built-in.o: In function `page_referenced_one':
rmap.c:(.text+0x32070): undefined reference to `pmdp_clear_flush_young'

Something like this?

mm/page_idle.c +5 -1

index 374931f32ebc..4ea9c4ef5146 100644
@@ -66,8 +66,12 @@ static int page_idle_clear_pte_refs_one(struct page *page,
 	if (pte) {
 		referenced = ptep_clear_young_notify(vma, addr, pte);
 		pte_unmap(pte);
-	} else
+	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 		referenced = pmdp_clear_young_notify(vma, addr, pmd);
+	} else {
+		/* unexpected pmd-mapped page? */
+		WARN_ON_ONCE(1);
+	}
 
 	spin_unlock(ptl);

mm/rmap.c +4 -1

index 27916086ac50..499b24511b1f 100644
@@ -929,9 +929,12 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 				referenced++;
 		}
 		pte_unmap(pte);
-	} else {
+	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
+	} else {
+		/* unexpected pmd-mapped page? */
+		WARN_ON_ONCE(1);
 	}
 	spin_unlock(ptl);

Signature · 7 lines

 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Sergey Senozhatsky

2015-11-24 09:22 UTC #3 lore raw

On (11/24/15 11:09), Kirill A. Shutemov wrote:
[..]

Quoted 25 lines

mm/page_idle.c +5 -1

index 374931f32ebc..4ea9c4ef5146 100644
@@ -66,8 +66,12 @@ static int page_idle_clear_pte_refs_one(struct page *page,
 	if (pte) {
 		referenced = ptep_clear_young_notify(vma, addr, pte);
 		pte_unmap(pte);
-	} else
+	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 		referenced = pmdp_clear_young_notify(vma, addr, pmd);
+	} else {
+		/* unexpected pmd-mapped page? */
+		WARN_ON_ONCE(1);
+	}
 
 	spin_unlock(ptl);

mm/rmap.c +0 -0

index 27916086ac50..499b24511b1f 100644
@@ -929,9 +929,12 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 				referenced++;
 		}

mm/rmap.c static int page_referenced_one(struct page *page, struct vm_area_struct *vma, · line 939

Quoted 10 lines

mm/rmap.c +4 -1

 		pte_unmap(pte);
-	} else {
+	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
+	} else {
+		/* unexpected pmd-mapped page? */
+		WARN_ON_ONCE(1);
 	}
 	spin_unlock(ptl);

yes, works for me.

-ss

Vladimir Davydov

author

2015-11-24 09:36 UTC #4 lore raw

In reply to Kirill A. Shutemov

Quoted 1 line · Kirill A. Shutemov

On Tue, Nov 24, 2015 at 01:29:41PM +0900, Sergey Senozhatsky wrote:

...

Quoted 4 lines

Quoted 2 lines
mm/built-in.o: In function `page_referenced_one':
rmap.c:(.text+0x32070): undefined reference to `pmdp_clear_flush_young'
Something like this?

...

Review static int page_referenced_one(struct page *page, struct vm_area_struct *vma, · line 939

Quoted 13 lines

(quoted diff) +4 -1

@@ -929,9 +929,12 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 				referenced++;
 		}
 		pte_unmap(pte);
-	} else {
+	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
+	} else {
+		/* unexpected pmd-mapped page? */
+		WARN_ON_ONCE(1);
 	}
 	spin_unlock(ptl);

I think we'd better compile out page_check_address_transhuge altogether if CONFIG_TRANSPARENT_HUGEPAGE is disabled and use page_check_address instead. This would also reduce the kernel size a bit.
---

include/linux/rmap.h +11 -0

index b9eedc63e9e6..77d1ba57d495 100644
@@ -219,9 +219,20 @@ static inline pte_t *page_check_address(struct page *page, struct mm_struct *mm,
  * Used by idle page tracking to check if a page was referenced via page
  * tables.
  */
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 bool page_check_address_transhuge(struct page *page, struct mm_struct *mm,
 				  unsigned long address, pmd_t **pmdp,
 				  pte_t **ptep, spinlock_t **ptlp);
+#else
+static inline bool page_check_address_transhuge(struct page *page,
+				struct mm_struct *mm, unsigned long address,
+				pmd_t **pmdp, pte_t **ptep, spinlock_t **ptlp)
+{
+	*ptep = page_check_address(page, mm, address, ptlp, 0);
+	*pmdp = NULL;
+	return !!*ptep;
+}
+#endif
 
 /*
  * Used by swapoff to help locate where page is expected in vma.

mm/rmap.c +2 -0

index 27916086ac50..6f371261dd12 100644
@@ -798,6 +798,7 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
 	return 1;
 }
 
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 /*
  * Check that @page is mapped at @address into @mm. In contrast to
  * page_check_address(), this function can handle transparent huge pages.

@@ -885,6 +886,7 @@ found:
 	*ptlp = ptl;
 	return true;
 }
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 struct page_referenced_arg {
 	int mapcount;

Vladimir Davydov

author

2015-11-24 09:47 UTC #5 lore raw

On Tue, Nov 24, 2015 at 12:36:17PM +0300, Vladimir Davydov wrote:
...

Quoted 3 lines

I think we'd better compile out page_check_address_transhuge altogether if CONFIG_TRANSPARENT_HUGEPAGE is disabled and use page_check_address instead. This would also reduce the kernel size a bit.

Sergey, could you please check if the patch below fixes build for you?

Thanks,
Vladimir

Quoted 40 lines

---

include/linux/rmap.h +11 -0

index b9eedc63e9e6..77d1ba57d495 100644
@@ -219,9 +219,20 @@ static inline pte_t *page_check_address(struct page *page, struct mm_struct *mm,
  * Used by idle page tracking to check if a page was referenced via page
  * tables.
  */
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 bool page_check_address_transhuge(struct page *page, struct mm_struct *mm,
 				  unsigned long address, pmd_t **pmdp,
 				  pte_t **ptep, spinlock_t **ptlp);
+#else
+static inline bool page_check_address_transhuge(struct page *page,
+				struct mm_struct *mm, unsigned long address,
+				pmd_t **pmdp, pte_t **ptep, spinlock_t **ptlp)
+{
+	*ptep = page_check_address(page, mm, address, ptlp, 0);
+	*pmdp = NULL;
+	return !!*ptep;
+}
+#endif
 
 /*
  * Used by swapoff to help locate where page is expected in vma.

mm/rmap.c +1 -0

index 27916086ac50..6f371261dd12 100644
@@ -798,6 +798,7 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
 	return 1;
 }
 
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 /*
  * Check that @page is mapped at @address into @mm. In contrast to
  * page_check_address(), this function can handle transparent huge pages.

mm/rmap.c found: · line 892

Quoted 7 lines

mm/rmap.c +1 -0

@@ -885,6 +886,7 @@ found:
 	*ptlp = ptl;
 	return true;
 }
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 struct page_referenced_arg {
 	int mapcount;

Sergey Senozhatsky

2015-11-24 10:08 UTC #6 lore raw

Hello,

Quoted 2 lines · Vladimir Davydov

Sergey, could you please check if the patch below fixes build for you?

yes, it does.

add/remove: 0/1 grow/shrink: 0/2 up/down: 0/-649 (-649)
function old new delta
__warned 2632 2631 -1
page_referenced_one 218 159 -59
page_check_address_transhuge 589 - -589

-ss

Kirill A. Shutemov

2015-11-24 09:57 UTC #7 lore raw

In reply to Vladimir Davydov

Quoted 17 lines · Vladimir Davydov

Quoted 1 line · Kirill A. Shutemov
On Tue, Nov 24, 2015 at 01:29:41PM +0900, Sergey Senozhatsky wrote:
...
Quoted 4 lines
Quoted 2 lines
mm/built-in.o: In function `page_referenced_one':
rmap.c:(.text+0x32070): undefined reference to `pmdp_clear_flush_young'
Something like this?
...
Quoted 9 lines
(quoted diff) +2 -1
@@ -929,9 +929,12 @@ static int page_referenced_one(struct page *page, struct vm_area_struct *vma,
 				referenced++;
 		}
 		pte_unmap(pte);
-	} else {
+	} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 		if (pmdp_clear_flush_young_notify(vma, address, pmd))
 			referenced++;
+	} else {

Quoted 8 lines

Review static int page_referenced_one(struct page *page, struct vm_area_struct *vma, · line 939
Quoted 4 lines
(quoted diff) +2 -0
+		/* unexpected pmd-mapped page? */
+		WARN_ON_ONCE(1);
 	}
 	spin_unlock(ptl);
I think we'd better compile out page_check_address_transhuge altogether if CONFIG_TRANSPARENT_HUGEPAGE is disabled and use page_check_address instead. This would also reduce the kernel size a bit.

Looks good to me, if compiler is clever enough to remove pmdp_clear_flush_young_notify() call for !THP. I'm not sure it is.

Quoted 46 lines

---

include/linux/rmap.h +11 -0

index b9eedc63e9e6..77d1ba57d495 100644
@@ -219,9 +219,20 @@ static inline pte_t *page_check_address(struct page *page, struct mm_struct *mm,
  * Used by idle page tracking to check if a page was referenced via page
  * tables.
  */
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 bool page_check_address_transhuge(struct page *page, struct mm_struct *mm,
 				  unsigned long address, pmd_t **pmdp,
 				  pte_t **ptep, spinlock_t **ptlp);
+#else
+static inline bool page_check_address_transhuge(struct page *page,
+				struct mm_struct *mm, unsigned long address,
+				pmd_t **pmdp, pte_t **ptep, spinlock_t **ptlp)
+{
+	*ptep = page_check_address(page, mm, address, ptlp, 0);
+	*pmdp = NULL;
+	return !!*ptep;
+}
+#endif
 
 /*
  * Used by swapoff to help locate where page is expected in vma.

mm/rmap.c +2 -0

index 27916086ac50..6f371261dd12 100644
@@ -798,6 +798,7 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
 	return 1;
 }
 
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 /*
  * Check that @page is mapped at @address into @mm. In contrast to
  * page_check_address(), this function can handle transparent huge pages.

@@ -885,6 +886,7 @@ found:
 	*ptlp = ptl;
 	return true;
 }
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 struct page_referenced_arg {
 	int mapcount;

Signature · 7 lines

 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Johannes Weiner

Tested-by

2015-11-24 18:15 UTC #8 lore raw

In reply to Vladimir Davydov

Quoted 12 lines · Vladimir Davydov

include/linux/rmap.h +1 -0

index b9eedc63e9e6..77d1ba57d495 100644
@@ -219,9 +219,20 @@ static inline pte_t *page_check_address(struct page *page, struct mm_struct *mm,
  * Used by idle page tracking to check if a page was referenced via page
  * tables.
  */
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
 bool page_check_address_transhuge(struct page *page, struct mm_struct *mm,
 				  unsigned long address, pmd_t **pmdp,
 				  pte_t **ptep, spinlock_t **ptlp);

include/linux/rmap.h static inline pte_t *page_check_address(struct page *page, struct mm_struct *mm, · line 235

Quoted 10 lines

include/linux/rmap.h +10 -0

+#else
+static inline bool page_check_address_transhuge(struct page *page,
+				struct mm_struct *mm, unsigned long address,
+				pmd_t **pmdp, pte_t **ptep, spinlock_t **ptlp)
+{
+	*ptep = page_check_address(page, mm, address, ptlp, 0);
+	*pmdp = NULL;
+	return !!*ptep;
+}
+#endif

Tested-by Johannes Weiner <[email protected]>