Skip to contentSkip to conversation

mm: improve insert_pages() error handling

PATCH Superseded Avi Weiss 3 participants 8 messages Aug 5 – Aug 25

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Avi Weiss

author
#0 lore raw

v2 cover mm: improve insert_pages() error handling

Improve insert_pages() error handling following review of the minor cleanup in v1.

Rename the helper that populates the upper page-table levels, set error codes at their points of failure, and return -ENOMEM when that population fails.

Thanks for the review. I'm new to Linux; most of my previous upstream work has been on NASA cFS and RTEMS.

I agree that initializing the return value to zero and assigning each relevant error code at its failure site is clearer and more idiomatic. The second (middle) patch implements that cleanup.

The pte_alloc() macro returns a boolean, so patch 2 tests it and assigns -ENOMEM explicitly rather than using its return value as an error code.

Changes in v2:

* rename walk_to_pmd() to populate_to_pmd()
* set error codes at their points of failure and rename ret to err
* split the cleanup and return-code change into separate patches
* drop the Fixes tag because this is a general cleanup

Avi Weiss (3):
mm/memory.c: rename walk_to_pmd() to populate_to_pmd() mm/memory.c: simplify error handling in insert_pages()
mm: return -ENOMEM for page-table allocation failure in insert_pages()

 mm/memory.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

base-commit: 848acc8ffe1b7cd5f1bf427b93069becfebc2c9d

Signature · 1 line
2.43.0
author
#1 lore raw

PATCH v2 1/3 mm/memory.c: rename walk_to_pmd() to populate_to_pmd()

This helper allocates any missing page-table levels (it doesn't just 'walk'. Rename for clarity.

---
 mm/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
mm/memory.c +3 -3
index ff338c2abe92..8658feba8be9 100644
@@ -2268,7 +2268,7 @@ void zap_special_vma_range(struct vm_area_struct *vma, unsigned long address,
 }
 EXPORT_SYMBOL_GPL(zap_special_vma_range);
 
-static pmd_t *walk_to_pmd(struct mm_struct *mm, unsigned long addr)
+static pmd_t *populate_to_pmd(struct mm_struct *mm, unsigned long addr)
 {
 	pgd_t *pgd;
 	p4d_t *p4d;
@@ -2293,7 +2293,7 @@ static pmd_t *walk_to_pmd(struct mm_struct *mm, unsigned long addr)
 pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
 		      spinlock_t **ptl)
 {
-	pmd_t *pmd = walk_to_pmd(mm, addr);
+	pmd_t *pmd = populate_to_pmd(mm, addr);
 
 	if (!pmd)
 		return NULL;
@@ -2437,7 +2437,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 	int ret;
 more:
 	ret = -EFAULT;
-	pmd = walk_to_pmd(mm, addr);
+	pmd = populate_to_pmd(mm, addr);
 	if (!pmd)
 		goto out;
 
Signature · 1 line
2.43.0
author
#3 lore raw

PATCH v2 2/3 mm/memory.c: simplify error handling in insert_pages()

In reply to Avi Weiss

Initialize error return status to zero and then set it as needed at each point of failure.

Assign -ENOMEM explicitly when pte_alloc() fails as the pte_alloc() macro returns a boolean.

---
 mm/memory.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
mm/memory.c +11 -11
index 8658feba8be9..11bb4fb98761 100644
@@ -2434,20 +2434,22 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 	unsigned long curr_page_idx = 0;
 	unsigned long remaining_pages_total = *num;
 	unsigned long pages_to_write_in_pmd;
-	int ret;
+	int err = 0;
 more:
-	ret = -EFAULT;
 	pmd = populate_to_pmd(mm, addr);
-	if (!pmd)
+	if (!pmd) {
+		err = -EFAULT;
 		goto out;
+	}
 
 	pages_to_write_in_pmd = min_t(unsigned long,
 		remaining_pages_total, PTRS_PER_PTE - pte_index(addr));
 
 	/* Allocate the PTE if necessary; takes PMD lock once only. */
-	ret = -ENOMEM;
-	if (pte_alloc(mm, pmd))
+	if (pte_alloc(mm, pmd)) {
+		err = -ENOMEM;
 		goto out;
+	}
 
 	while (pages_to_write_in_pmd) {
 		int pte_idx = 0;
@@ -2455,15 +2457,14 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 
 		start_pte = pte_offset_map_lock(mm, pmd, addr, &pte_lock);
 		if (!start_pte) {
-			ret = -EFAULT;
+			err = -EFAULT;
 			goto out;
 		}
 		for (pte = start_pte; pte_idx < batch_size; ++pte, ++pte_idx) {
-			int err = insert_page_in_batch_locked(vma, pte,
-				addr, pages[curr_page_idx], prot);
+			err = insert_page_in_batch_locked(vma, pte, addr,
+							  pages[curr_page_idx], prot);
 			if (unlikely(err)) {
 				pte_unmap_unlock(start_pte, pte_lock);
-				ret = err;
 				remaining_pages_total -= pte_idx;
 				goto out;
 			}
@@ -2476,10 +2477,9 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 	}
 	if (remaining_pages_total)
 		goto more;
-	ret = 0;
 out:
 	*num = remaining_pages_total;
-	return ret;
+	return err;
 }
 
 /**
Signature · 1 line
2.43.0
author
#5 lore raw

PATCH v2 3/3 mm: return -ENOMEM for page-table allocation failure in insert_pages()

In reply to Avi Weiss

populate_to_pmd() returns NULL only when p4d_alloc(), pud_alloc(), or pmd_alloc() fails. These are page-table allocation failures, but insert_pages() currently reports them as -EFAULT.

Return -ENOMEM instead, consistent with the subsequent pte_alloc() failure and with the single-page insert_page() path, which reports failure of the same page-table allocation chain as -ENOMEM.

Address and range validation failures in vm_insert_pages() continue to return -EFAULT. Keep the later -EFAULT return for pte_offset_map_lock(), which is not an allocation failure.

---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memory.c +1 -1
index 11bb4fb98761..16093479d341 100644
@@ -2438,7 +2438,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 more:
 	pmd = populate_to_pmd(mm, addr);
 	if (!pmd) {
-		err = -EFAULT;
+		err = -ENOMEM;
 		goto out;
 	}
 
Signature · 1 line
2.43.0
maintainer Acked-by on v2 2/3
#4 lore raw

Re: [PATCH v2 2/3] mm/memory.c: simplify error handling in insert_pages()

In reply to Avi Weiss
Quoted 5 lines · Avi Weiss

Initialize error return status to zero and then set it as needed at each point of failure.

Assign -ENOMEM explicitly when pte_alloc() fails as the pte_alloc() macro returns a boolean.

Subject should be "mm/memory: simplify" ...

Signature · 3 lines
Cheers,

David
#2 lore raw

Re: [PATCH v2 1/3] mm/memory.c: rename walk_to_pmd() to populate_to_pmd()

In reply to Avi Weiss
Quoted 2 lines · Avi Weiss

This helper allocates any missing page-table levels (it doesn't just 'walk'. Rename for clarity.

I don't think that's any clearer. The point it that population is just a side-effect of performing the walk.

The main job of this function is to return a pmd_t pointer.

Signature · 3 lines
Cheers,

David
maintainer Acked-by on v2 3/3
#6 lore raw

Re: [PATCH v2 3/3] mm: return -ENOMEM for page-table allocation failure in insert_pages()

In reply to Avi Weiss
Quoted 22 lines · Avi Weiss

populate_to_pmd() returns NULL only when p4d_alloc(), pud_alloc(), or pmd_alloc() fails. These are page-table allocation failures, but insert_pages() currently reports them as -EFAULT.

Return -ENOMEM instead, consistent with the subsequent pte_alloc() failure and with the single-page insert_page() path, which reports failure of the same page-table allocation chain as -ENOMEM.

Address and range validation failures in vm_insert_pages() continue to return -EFAULT. Keep the later -EFAULT return for pte_offset_map_lock(), which is not an allocation failure.

---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memory.c +0 -0
index 11bb4fb98761..16093479d341 100644

mm/memory.c static int insert_pages(struct vm_area_struct *vma, unsigned long addr, · line 2444

Quoted 8 lines
mm/memory.c +1 -1
@@ -2438,7 +2438,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 more:
 	pmd = populate_to_pmd(mm, addr);
 	if (!pmd) {
-		err = -EFAULT;
+		err = -ENOMEM;
 		goto out;
 	}
 

For this patch:

Signature · 3 lines
Cheers,

David
reviewer Reviewed-by
#7 lore raw

Re: [PATCH v2 3/3] mm: return -ENOMEM for page-table allocation failure in insert_pages()

mm/memory: please let's be consistent on all of the patches in the series.

Quoted 34 lines · David Hildenbrand (Arm)
Quoted 22 lines · Avi Weiss

populate_to_pmd() returns NULL only when p4d_alloc(), pud_alloc(), or pmd_alloc() fails. These are page-table allocation failures, but insert_pages() currently reports them as -EFAULT.

Return -ENOMEM instead, consistent with the subsequent pte_alloc() failure and with the single-page insert_page() path, which reports failure of the same page-table allocation chain as -ENOMEM.

Address and range validation failures in vm_insert_pages() continue to return -EFAULT. Keep the later -EFAULT return for pte_offset_map_lock(), which is not an allocation failure.

---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memory.c +0 -0
index 11bb4fb98761..16093479d341 100644

mm/memory.c static int insert_pages(struct vm_area_struct *vma, unsigned long addr, · line 2444

Quoted 8 lines
mm/memory.c +1 -1
@@ -2438,7 +2438,7 @@ static int insert_pages(struct vm_area_struct *vma, unsigned long addr,
 more:
 	pmd = populate_to_pmd(mm, addr);
 	if (!pmd) {
-		err = -EFAULT;
+		err = -ENOMEM;
 		goto out;
 	}
 

For this patch:

Quoted 6 lines

--
Cheers,

David

LGTM too so:

--
Cheers, Lorenzo