mm: improve insert_pages() error handling
Subsystems (2)
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
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
@@ -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
PATCH v2 2/3 mm/memory.c: simplify error handling in insert_pages()
In reply to Avi WeissInitialize 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
@@ -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
PATCH v2 3/3 mm: return -ENOMEM for page-table allocation failure in insert_pages()
In reply to Avi Weisspopulate_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
@@ -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
Re: [PATCH v2 2/3] mm/memory.c: simplify error handling in insert_pages()
In reply to Avi WeissQuoted 5 lines · Avi Weiss
Initialize error return status to zero and then set it as needed at each point of failure.
Assign
-ENOMEMexplicitly whenpte_alloc()fails as thepte_alloc()macro returns a boolean.
Subject should be "mm/memory: simplify" ...
Signature · 3 lines
Cheers, David
David Hildenbrand (Arm)
maintainerRe: [PATCH v2 1/3] mm/memory.c: rename walk_to_pmd() to populate_to_pmd()
In reply to Avi WeissQuoted 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
Re: [PATCH v2 3/3] mm: return -ENOMEM for page-table allocation failure in insert_pages()
In reply to Avi WeissQuoted 22 lines · Avi Weiss
populate_to_pmd()returns NULL only whenp4d_alloc(),pud_alloc(), orpmd_alloc()fails. These are page-table allocation failures, butinsert_pages()currently reports them as-EFAULT.Return
-ENOMEMinstead, consistent with the subsequentpte_alloc()failure and with the single-pageinsert_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-EFAULTreturn forpte_offset_map_lock(), which is not an allocation failure.Signed-off-by Avi Weiss <[email protected]>--- mm/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memory.c +0
-0
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
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 whenp4d_alloc(),pud_alloc(), orpmd_alloc()fails. These are page-table allocation failures, butinsert_pages()currently reports them as-EFAULT.Return
-ENOMEMinstead, consistent with the subsequentpte_alloc()failure and with the single-pageinsert_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-EFAULTreturn forpte_offset_map_lock(), which is not an allocation failure.Signed-off-by Avi Weiss <[email protected]>--- mm/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/memory.c +0
-0mm/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