On Sat, 06 Aug 2011 18:58:05 +0200
aarcange@xxxxxxxxxx wrote:
> From: Andrea Arcangeli <aarcange@xxxxxxxxxx>
>
> This adds THP support to mremap (decreases the number of split_huge_page
> called).
>
> ...
I have some nitpicking.
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1054,6 +1054,52 @@ int mincore_huge_pmd(struct vm_area_stru
> return ret;
> }
>
> +int move_huge_pmd(struct vm_area_struct *vma, struct vm_area_struct *new_vma,
> + unsigned long old_addr,
> + unsigned long new_addr, unsigned long old_end,
> + pmd_t *old_pmd, pmd_t *new_pmd)
> +{
> + int ret = 0;
> + pmd_t pmd;
> +
> + struct mm_struct *mm = vma->vm_mm;
> +
> + if ((old_addr & ~HPAGE_PMD_MASK) ||
> + (new_addr & ~HPAGE_PMD_MASK) ||
> + (old_addr + HPAGE_PMD_SIZE) > old_end ||
Can (old_addr + HPAGE_PMD_SIZE) wrap past zero?
> + new_vma->vm_flags & VM_NOHUGEPAGE)
This should be parenthesized, IMO, like the other sub-expressions.
> + goto out;
> +
> + /*
> + * The destination pmd shouldn't be established, free_pgtables()
> + * should have release it.
> + */
> + if (!pmd_none(*new_pmd)) {
> + WARN_ON(1);
We can use the WARN_ON return value trick here.
> + VM_BUG_ON(pmd_trans_huge(*new_pmd));
> + goto out;
> + }
> +
> + spin_lock(&mm->page_table_lock);
> + if (likely(pmd_trans_huge(*old_pmd))) {
> + if (pmd_trans_splitting(*old_pmd)) {
> + spin_unlock(&mm->page_table_lock);
> + wait_split_huge_page(vma->anon_vma, old_pmd);
> + ret = -1;
> + } else {
> + pmd = pmdp_get_and_clear(mm, old_addr, old_pmd);
> + VM_BUG_ON(!pmd_none(*new_pmd));
> + set_pmd_at(mm, new_addr, new_pmd, pmd);
> + spin_unlock(&mm->page_table_lock);
> + ret = 1;
> + }
> + } else
> + spin_unlock(&mm->page_table_lock);
If the "if" part has braces, it's conventional to also add them to the
"else" part.
> +out:
> + return ret;
> +}
> +
Result:
diff -puN mm/huge_memory.c~thp-mremap-support-and-tlb-optimization-fix mm/huge_memory.c
--- a/mm/huge_memory.c~thp-mremap-support-and-tlb-optimization-fix
+++ a/mm/huge_memory.c
@@ -1065,15 +1065,14 @@ int move_huge_pmd(struct vm_area_struct
if ((old_addr & ~HPAGE_PMD_MASK) ||
(new_addr & ~HPAGE_PMD_MASK) ||
(old_addr + HPAGE_PMD_SIZE) > old_end ||
- new_vma->vm_flags & VM_NOHUGEPAGE)
+ (new_vma->vm_flags & VM_NOHUGEPAGE))
goto out;
/*
* The destination pmd shouldn't be established, free_pgtables()
* should have release it.
*/
- if (!pmd_none(*new_pmd)) {
- WARN_ON(1);
+ if (!WARN_ON(pmd_none(*new_pmd))) {
VM_BUG_ON(pmd_trans_huge(*new_pmd));
goto out;
}
@@ -1091,9 +1090,9 @@ int move_huge_pmd(struct vm_area_struct
spin_unlock(&mm->page_table_lock);
ret = 1;
}
- } else
+ } else {
spin_unlock(&mm->page_table_lock);
-
+ }
out:
return ret;
}
--- a/mm/mremap.c~thp-mremap-support-and-tlb-optimization-fix
+++ a/mm/mremap.c
@@ -155,13 +155,13 @@ unsigned long move_page_tables(struct vm
if (err > 0) {
need_flush = true;
continue;
- } else if (!err)
+ } else if (!err) {
split_huge_page_pmd(vma->vm_mm, old_pmd);
+ }
VM_BUG_ON(pmd_trans_huge(*old_pmd));
}
if (pmd_none(*new_pmd) && __pte_alloc(new_vma->vm_mm, new_vma,
- new_pmd,
- new_addr))
+ new_pmd, new_addr))
break;
next = (new_addr + PMD_SIZE) & PMD_MASK;
if (extent > next - new_addr)
_
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>