Simplify users of vma_address_end()

RFC In review Matthew Wilcox 2 participants 2 messages Jan 19, 2022 – Jan 24, 2022

Subsystems (3)

Memory management MM core MM rmap

lore

Share & tools

lore

Matthew Wilcox

author

2022-01-19 17:50 UTC #0 lore raw

RFC 1/1 Simplify users of vma_address_end()

Hi Hugh,

What do you think to this simplification? Dave dislikes the ?: usage in this context, and I thought we could usefully put the condition inside the (inline) function.

I could also go for:

	if (!PageCompound(page))
		return address + PAGE_SIZE;
	VM_BUG_ON_PAGE(PageKsm(page), page);    /* KSM page->index unusable */

in case anyone starts to create compound KSM pages.

mm/internal.h +4 -4

index c774075b3893..7cd33ee4df32 100644
@@ -462,13 +462,13 @@ vma_address(struct page *page, struct vm_area_struct *vma)
  * Assumes that vma_address() already returned a good starting address.
  * If page is a compound head, the entire compound page is considered.
  */
-static inline unsigned long
-vma_address_end(struct page *page, struct vm_area_struct *vma)
+static inline unsigned long vma_address_end(struct page *page,
+		unsigned long address, struct vm_area_struct *vma)
 {
 	pgoff_t pgoff;
-	unsigned long address;
 
-	VM_BUG_ON_PAGE(PageKsm(page), page);	/* KSM page->index unusable */
+	if (PageKsm(page) || !PageCompound(page))
+		return address + PAGE_SIZE;
 	pgoff = page_to_pgoff(page) + compound_nr(page);
 	address = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
 	/* Check for address beyond vma (or wrapped through 0?) */

mm/page_vma_mapped.c +1 -9

index f7b331081791..fcd7b9ccfb1e 100644
@@ -181,15 +181,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
 		return true;
 	}
 
-	/*
-	 * Seek to next pte only makes sense for THP.
-	 * But more important than that optimization, is to filter out
-	 * any PageKsm page: whose page->index misleads vma_address()
-	 * and vma_address_end() to disaster.
-	 */
-	end = PageTransCompound(page) ?
-		vma_address_end(page, pvmw->vma) :
-		pvmw->address + PAGE_SIZE;
+	end = vma_address_end(page, pvmw->address, pvmw->vma);
 	if (pvmw->pte)
 		goto next_pte;
 restart:

mm/rmap.c +3 -5

index a531b64d53fa..5d5dc2a60a26 100644
@@ -946,7 +946,7 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
 	 */
 	mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_PAGE,
 				0, vma, vma->vm_mm, address,
-				vma_address_end(page, vma));
+				vma_address_end(page, address, vma));
 	mmu_notifier_invalidate_range_start(&range);
 
 	while (page_vma_mapped_walk(&pvmw)) {

@@ -1453,8 +1453,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 	 * Note that the page can not be free in this function as call of
 	 * try_to_unmap() must hold a reference on the page.
 	 */
-	range.end = PageKsm(page) ?
-			address + PAGE_SIZE : vma_address_end(page, vma);
+	range.end = vma_address_end(page, address, vma);
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
 				address, range.end);
 	if (PageHuge(page)) {

@@ -1757,8 +1756,7 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
 	 * Note that the page can not be free in this function as call of
 	 * try_to_unmap() must hold a reference on the page.
 	 */
-	range.end = PageKsm(page) ?
-			address + PAGE_SIZE : vma_address_end(page, vma);
+	range.end = vma_address_end(page, address, vma);
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
 				address, range.end);
 	if (PageHuge(page)) {

Hugh Dickins

2022-01-24 04:40 UTC #1 lore raw

Quoted 5 lines · Matthew Wilcox

Hi Hugh,
What do you think to this simplification? Dave dislikes the ?: usage in this context, and I thought we could usefully put the condition inside the (inline) function.

Yes, that's an improvement. I think when I created vma_address_end() originally (long before posting), there was only one caller who needed to worry about PageKsm, so I avoided inflicting that awkwardness on the others; but now it looks like most (of its few) callers do have to worry.

But I'll suggest one nit, which you can ignore if you feel strongly
otherwise: I'd prefer vma_address_end(page, vma, address) as a better match to vma_address(page, vma).

Quoted 6 lines

I could also go for:

	if (!PageCompound(page))
		return address + PAGE_SIZE;
	VM_BUG_ON_PAGE(PageKsm(page), page);    /* KSM page->index unusable */

Another way to do it, yes: I don't mind either way.

Thanks,
Hugh

Quoted 79 lines

in case anyone starts to create compound KSM pages.

mm/internal.h +4 -4

index c774075b3893..7cd33ee4df32 100644
@@ -462,13 +462,13 @@ vma_address(struct page *page, struct vm_area_struct *vma)
  * Assumes that vma_address() already returned a good starting address.
  * If page is a compound head, the entire compound page is considered.
  */
-static inline unsigned long
-vma_address_end(struct page *page, struct vm_area_struct *vma)
+static inline unsigned long vma_address_end(struct page *page,
+		unsigned long address, struct vm_area_struct *vma)
 {
 	pgoff_t pgoff;
-	unsigned long address;
 
-	VM_BUG_ON_PAGE(PageKsm(page), page);	/* KSM page->index unusable */
+	if (PageKsm(page) || !PageCompound(page))
+		return address + PAGE_SIZE;
 	pgoff = page_to_pgoff(page) + compound_nr(page);
 	address = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
 	/* Check for address beyond vma (or wrapped through 0?) */

mm/page_vma_mapped.c +1 -9

index f7b331081791..fcd7b9ccfb1e 100644
@@ -181,15 +181,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
 		return true;
 	}
 
-	/*
-	 * Seek to next pte only makes sense for THP.
-	 * But more important than that optimization, is to filter out
-	 * any PageKsm page: whose page->index misleads vma_address()
-	 * and vma_address_end() to disaster.
-	 */
-	end = PageTransCompound(page) ?
-		vma_address_end(page, pvmw->vma) :
-		pvmw->address + PAGE_SIZE;
+	end = vma_address_end(page, pvmw->address, pvmw->vma);
 	if (pvmw->pte)
 		goto next_pte;
 restart:

mm/rmap.c +3 -5

index a531b64d53fa..5d5dc2a60a26 100644
@@ -946,7 +946,7 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
 	 */
 	mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_PAGE,
 				0, vma, vma->vm_mm, address,
-				vma_address_end(page, vma));
+				vma_address_end(page, address, vma));
 	mmu_notifier_invalidate_range_start(&range);
 
 	while (page_vma_mapped_walk(&pvmw)) {

@@ -1453,8 +1453,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 	 * Note that the page can not be free in this function as call of
 	 * try_to_unmap() must hold a reference on the page.
 	 */
-	range.end = PageKsm(page) ?
-			address + PAGE_SIZE : vma_address_end(page, vma);
+	range.end = vma_address_end(page, address, vma);
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
 				address, range.end);
 	if (PageHuge(page)) {

@@ -1757,8 +1756,7 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
 	 * Note that the page can not be free in this function as call of
 	 * try_to_unmap() must hold a reference on the page.
 	 */
-	range.end = PageKsm(page) ?
-			address + PAGE_SIZE : vma_address_end(page, vma);
+	range.end = vma_address_end(page, address, vma);
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
 				address, range.end);
 	if (PageHuge(page)) {