Hi all,
This series contains cleanups to remove unneeded variable, useless
BUG_ON and use helper to improve readability. Also we fix a potential
pgmap refcnt leak. More details can be found in the respective
changelogs. Thanks!
Miaohe Lin (5):
mm: gup: remove set but unused local variable major
mm: gup: remove unneed local variable orig_refs
mm: gup: remove useless BUG_ON in __get_user_pages()
mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()
mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()
mm/gup.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
--
2.23.0
Indeed, this BUG_ON couldn't catch anything useful. We are sure ret == 0
here because we would already bail out if ret != 0 and ret is untouched
till here.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/gup.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/gup.c b/mm/gup.c
index d6056dcab02c..d7e4507de6b1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1149,7 +1149,6 @@ static long __get_user_pages(struct mm_struct *mm,
* We must stop here.
*/
BUG_ON(gup_flags & FOLL_NOWAIT);
- BUG_ON(ret != 0);
goto out;
}
continue;
--
2.23.0
Remove unneed local variable orig_refs since refs is unchanged now.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/gup.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 430495cb1b91..d6056dcab02c 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -117,8 +117,6 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
if (flags & FOLL_GET)
return try_get_compound_head(page, refs);
else if (flags & FOLL_PIN) {
- int orig_refs = refs;
-
/*
* Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
* right zone, so fail and let the caller fall back to the slow
@@ -150,7 +148,7 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1));
mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_ACQUIRED,
- orig_refs);
+ refs);
return page;
}
--
2.23.0
When failed to try_grab_page, put_dev_pagemap() is missed. So pgmap
refcnt will leak in this case. Also we remove the check for pgmap
against NULL as it's also checked inside the put_dev_pagemap().
Signed-off-by: Miaohe Lin <[email protected]>
Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
---
mm/gup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index d7e4507de6b1..8c89e614d4aa 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2253,14 +2253,14 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
pages[*nr] = page;
if (unlikely(!try_grab_page(page, flags))) {
undo_dev_pagemap(nr, nr_start, flags, pages);
+ put_dev_pagemap(pgmap);
return 0;
}
(*nr)++;
pfn++;
} while (addr += PAGE_SIZE, addr != end);
- if (pgmap)
- put_dev_pagemap(pgmap);
+ put_dev_pagemap(pgmap);
return 1;
}
--
2.23.0
Since commit a2beb5f1efed ("mm: clean up the last pieces of page fault
accountings"), the local variable major is unused. Remove it.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/gup.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 77150624f77a..430495cb1b91 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1276,7 +1276,7 @@ int fixup_user_fault(struct mm_struct *mm,
bool *unlocked)
{
struct vm_area_struct *vma;
- vm_fault_t ret, major = 0;
+ vm_fault_t ret;
address = untagged_addr(address);
@@ -1296,7 +1296,6 @@ int fixup_user_fault(struct mm_struct *mm,
return -EINTR;
ret = handle_mm_fault(vma, address, fault_flags, NULL);
- major |= ret & VM_FAULT_MAJOR;
if (ret & VM_FAULT_ERROR) {
int err = vm_fault_to_errno(ret, 0);
--
2.23.0
Use helper PAGE_ALIGNED to check if address is aligned to PAGE_SIZE.
Minor readability improvement.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/gup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 8c89e614d4aa..802a3deb50cd 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1471,8 +1471,8 @@ long populate_vma_page_range(struct vm_area_struct *vma,
unsigned long nr_pages = (end - start) / PAGE_SIZE;
int gup_flags;
- VM_BUG_ON(start & ~PAGE_MASK);
- VM_BUG_ON(end & ~PAGE_MASK);
+ VM_BUG_ON(!PAGE_ALIGNED(start));
+ VM_BUG_ON(!PAGE_ALIGNED(end));
VM_BUG_ON_VMA(start < vma->vm_start, vma);
VM_BUG_ON_VMA(end > vma->vm_end, vma);
mmap_assert_locked(mm);
--
2.23.0
On Sat, 7 Aug 2021 17:36:19 +0800 Miaohe Lin <[email protected]> wrote:
> When failed to try_grab_page, put_dev_pagemap() is missed. So pgmap
> refcnt will leak in this case. Also we remove the check for pgmap
> against NULL as it's also checked inside the put_dev_pagemap().
>
> ...
>
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2253,14 +2253,14 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
> pages[*nr] = page;
> if (unlikely(!try_grab_page(page, flags))) {
> undo_dev_pagemap(nr, nr_start, flags, pages);
> + put_dev_pagemap(pgmap);
> return 0;
> }
> (*nr)++;
> pfn++;
> } while (addr += PAGE_SIZE, addr != end);
>
> - if (pgmap)
> - put_dev_pagemap(pgmap);
> + put_dev_pagemap(pgmap);
> return 1;
> }
We can simplify further, and remove the troublesome multiple return points?
--- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge-fix
+++ a/mm/gup.c
@@ -2247,14 +2247,13 @@ static int __gup_device_huge(unsigned lo
pgmap = get_dev_pagemap(pfn, pgmap);
if (unlikely(!pgmap)) {
undo_dev_pagemap(nr, nr_start, flags, pages);
- return 0;
+ break;
}
SetPageReferenced(page);
pages[*nr] = page;
if (unlikely(!try_grab_page(page, flags))) {
undo_dev_pagemap(nr, nr_start, flags, pages);
- put_dev_pagemap(pgmap);
- return 0;
+ break;
}
(*nr)++;
pfn++;
_
On Sat, 7 Aug 2021 11:41:12 -0700 Andrew Morton <[email protected]> wrote:
> We can simplify further, and remove the troublesome multiple return points?
>
oops.
--- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge-fix-fix
+++ a/mm/gup.c
@@ -2240,6 +2240,7 @@ static int __gup_device_huge(unsigned lo
{
int nr_start = *nr;
struct dev_pagemap *pgmap = NULL;
+ int ret = 1;
do {
struct page *page = pfn_to_page(pfn);
@@ -2247,12 +2248,14 @@ static int __gup_device_huge(unsigned lo
pgmap = get_dev_pagemap(pfn, pgmap);
if (unlikely(!pgmap)) {
undo_dev_pagemap(nr, nr_start, flags, pages);
+ ret = 0;
break;
}
SetPageReferenced(page);
pages[*nr] = page;
if (unlikely(!try_grab_page(page, flags))) {
undo_dev_pagemap(nr, nr_start, flags, pages);
+ ret = 0;
break;
}
(*nr)++;
@@ -2260,7 +2263,7 @@ static int __gup_device_huge(unsigned lo
} while (addr += PAGE_SIZE, addr != end);
put_dev_pagemap(pgmap);
- return 1;
+ return ret;
}
static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
Not sure if it's worth bothering, really...
On 8/7/21 2:36 AM, Miaohe Lin wrote:
> Hi all,
> This series contains cleanups to remove unneeded variable, useless
> BUG_ON and use helper to improve readability. Also we fix a potential
> pgmap refcnt leak. More details can be found in the respective
> changelogs. Thanks!
>
> Miaohe Lin (5):
> mm: gup: remove set but unused local variable major
> mm: gup: remove unneed local variable orig_refs
> mm: gup: remove useless BUG_ON in __get_user_pages()
> mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()
> mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()
>
> mm/gup.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
For the series:
Reviewed-by: John Hubbard <[email protected]>
thanks,
--
John Hubbard
NVIDIA
On 8/7/21 11:45 AM, Andrew Morton wrote:
> On Sat, 7 Aug 2021 11:41:12 -0700 Andrew Morton <[email protected]> wrote:
>
>> We can simplify further, and remove the troublesome multiple return points?
>>
>
> oops.
I sent a reviewed by to the "+" fixup email, but just realized that that did not
hit the main
mailing list. So:
For the end result of these stacked fixes to this file:
Reviewed-by: John Hubbard <[email protected]>
thanks,
--
John Hubbard
NVIDIA
>
> --- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge-fix-fix
> +++ a/mm/gup.c
> @@ -2240,6 +2240,7 @@ static int __gup_device_huge(unsigned lo
> {
> int nr_start = *nr;
> struct dev_pagemap *pgmap = NULL;
> + int ret = 1;
>
> do {
> struct page *page = pfn_to_page(pfn);
> @@ -2247,12 +2248,14 @@ static int __gup_device_huge(unsigned lo
> pgmap = get_dev_pagemap(pfn, pgmap);
> if (unlikely(!pgmap)) {
> undo_dev_pagemap(nr, nr_start, flags, pages);
> + ret = 0;
> break;
> }
> SetPageReferenced(page);
> pages[*nr] = page;
> if (unlikely(!try_grab_page(page, flags))) {
> undo_dev_pagemap(nr, nr_start, flags, pages);
> + ret = 0;
> break;
> }
> (*nr)++;
> @@ -2260,7 +2263,7 @@ static int __gup_device_huge(unsigned lo
> } while (addr += PAGE_SIZE, addr != end);
>
> put_dev_pagemap(pgmap);
> - return 1;
> + return ret;
> }
>
> static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
>
> Not sure if it's worth bothering, really...
>
On 07.08.21 11:36, Miaohe Lin wrote:
> Since commit a2beb5f1efed ("mm: clean up the last pieces of page fault
> accountings"), the local variable major is unused. Remove it.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/gup.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 77150624f77a..430495cb1b91 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1276,7 +1276,7 @@ int fixup_user_fault(struct mm_struct *mm,
> bool *unlocked)
> {
> struct vm_area_struct *vma;
> - vm_fault_t ret, major = 0;
> + vm_fault_t ret;
>
> address = untagged_addr(address);
>
> @@ -1296,7 +1296,6 @@ int fixup_user_fault(struct mm_struct *mm,
> return -EINTR;
>
> ret = handle_mm_fault(vma, address, fault_flags, NULL);
> - major |= ret & VM_FAULT_MAJOR;
> if (ret & VM_FAULT_ERROR) {
> int err = vm_fault_to_errno(ret, 0);
>
>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 07.08.21 11:36, Miaohe Lin wrote:
> Remove unneed local variable orig_refs since refs is unchanged now.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/gup.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 430495cb1b91..d6056dcab02c 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -117,8 +117,6 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
> if (flags & FOLL_GET)
> return try_get_compound_head(page, refs);
> else if (flags & FOLL_PIN) {
> - int orig_refs = refs;
> -
> /*
> * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
> * right zone, so fail and let the caller fall back to the slow
> @@ -150,7 +148,7 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
> page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1));
>
> mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_ACQUIRED,
> - orig_refs);
> + refs);
>
> return page;
> }
>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 07.08.21 11:36, Miaohe Lin wrote:
> Indeed, this BUG_ON couldn't catch anything useful. We are sure ret == 0
> here because we would already bail out if ret != 0 and ret is untouched
> till here.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/gup.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index d6056dcab02c..d7e4507de6b1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1149,7 +1149,6 @@ static long __get_user_pages(struct mm_struct *mm,
> * We must stop here.
> */
> BUG_ON(gup_flags & FOLL_NOWAIT);
> - BUG_ON(ret != 0);
> goto out;
> }
> continue;
>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 07.08.21 11:36, Miaohe Lin wrote:
> Use helper PAGE_ALIGNED to check if address is aligned to PAGE_SIZE.
> Minor readability improvement.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/gup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 8c89e614d4aa..802a3deb50cd 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1471,8 +1471,8 @@ long populate_vma_page_range(struct vm_area_struct *vma,
> unsigned long nr_pages = (end - start) / PAGE_SIZE;
> int gup_flags;
>
> - VM_BUG_ON(start & ~PAGE_MASK);
> - VM_BUG_ON(end & ~PAGE_MASK);
> + VM_BUG_ON(!PAGE_ALIGNED(start));
> + VM_BUG_ON(!PAGE_ALIGNED(end));
> VM_BUG_ON_VMA(start < vma->vm_start, vma);
> VM_BUG_ON_VMA(end > vma->vm_end, vma);
> mmap_assert_locked(mm);
>
Making it look more like faultin_vma_page_range(), nice :)
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On Sat, 7 Aug 2021 17:36:15 +0800
Miaohe Lin <[email protected]> wrote:
> Hi all,
> This series contains cleanups to remove unneeded variable, useless
> BUG_ON and use helper to improve readability. Also we fix a potential
> pgmap refcnt leak. More details can be found in the respective
> changelogs. Thanks!
>
> Miaohe Lin (5):
> mm: gup: remove set but unused local variable major
> mm: gup: remove unneed local variable orig_refs
> mm: gup: remove useless BUG_ON in __get_user_pages()
> mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()
> mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()
>
> mm/gup.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
Whole series (including the fixup for the return value):
Reviewed-by: Claudio Imbrenda <[email protected]>