[PATCH 1/4] thp: clean up __collapse_huge_page_isolate

[PATCH 1/4] thp: clean up __collapse_huge_page_isolate

[Bob Liu]

There are duplicated place using release_pte_pages().
And release_all_pte_pages() can also be removed.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/huge_memory.c |   37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index a863af2..462d6ea 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1700,64 +1700,49 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte)
 	}
 }
 
-static void release_all_pte_pages(pte_t *pte)
-{
-	release_pte_pages(pte, pte + HPAGE_PMD_NR);
-}
-
 static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
 					unsigned long address,
 					pte_t *pte)
 {
 	struct page *page;
 	pte_t *_pte;
-	int referenced = 0, isolated = 0, none = 0;
+	int referenced = 0, isolated = 1, none = 0;
 	for (_pte = pte; _pte < pte+HPAGE_PMD_NR;
 	     _pte++, address += PAGE_SIZE) {
 		pte_t pteval = *_pte;
 		if (pte_none(pteval)) {
 			if (++none <= khugepaged_max_ptes_none)
 				continue;
-			else {
-				release_pte_pages(pte, _pte);
+			else
 				goto out;
-			}
 		}
-		if (!pte_present(pteval) || !pte_write(pteval)) {
-			release_pte_pages(pte, _pte);
+		if (!pte_present(pteval) || !pte_write(pteval))
 			goto out;
-		}
 		page = vm_normal_page(vma, address, pteval);
-		if (unlikely(!page)) {
-			release_pte_pages(pte, _pte);
+		if (unlikely(!page))
 			goto out;
-		}
+
 		VM_BUG_ON(PageCompound(page));
 		BUG_ON(!PageAnon(page));
 		VM_BUG_ON(!PageSwapBacked(page));
 
 		/* cannot use mapcount: can't collapse if there's a gup pin */
-		if (page_count(page) != 1) {
-			release_pte_pages(pte, _pte);
+		if (page_count(page) != 1)
 			goto out;
-		}
 		/*
 		 * We can do it before isolate_lru_page because the
 		 * page can't be freed from under us. NOTE: PG_lock
 		 * is needed to serialize against split_huge_page
 		 * when invoked from the VM.
 		 */
-		if (!trylock_page(page)) {
-			release_pte_pages(pte, _pte);
+		if (!trylock_page(page))
 			goto out;
-		}
 		/*
 		 * Isolate the page to avoid collapsing an hugepage
 		 * currently in use by the VM.
 		 */
 		if (isolate_lru_page(page)) {
 			unlock_page(page);
-			release_pte_pages(pte, _pte);
 			goto out;
 		}
 		/* 0 stands for page_is_file_cache(page) == false */
@@ -1770,11 +1755,11 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
 		    mmu_notifier_test_young(vma->vm_mm, address))
 			referenced = 1;
 	}
-	if (unlikely(!referenced))
-		release_all_pte_pages(pte);
-	else
-		isolated = 1;
+	if (unlikely(!referenced)) {
 out:
+		release_pte_pages(pte, _pte);
+		isolated = 0;
+	}
 	return isolated;
 }
 
-- 
1.7.9.5

[PATCH 2/4] thp: introduce hugepage_get_pmd()

[Bob Liu]

Introduce hugepage_get_pmd() to simple code.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/huge_memory.c |   68 ++++++++++++++++++++++--------------------------------
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 462d6ea..e575b29 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1115,6 +1115,25 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 	return ret;
 }
 
+static pmd_t *hugepage_get_pmd(struct mm_struct *mm, unsigned long address)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd = NULL;
+
+	pgd = pgd_offset(mm, address);
+	if (!pgd_present(*pgd))
+		goto out;
+
+	pud = pud_offset(pgd, address);
+	if (!pud_present(*pud))
+		goto out;
+
+	pmd = pmd_offset(pud, address);
+out:
+	return pmd;
+}
+
 /*
  * Returns 1 if a given pmd maps a stable (not under splitting) thp.
  * Returns -1 if it maps a thp under splitting. Returns 0 otherwise.
@@ -1145,22 +1164,14 @@ pmd_t *page_check_address_pmd(struct page *page,
 			      unsigned long address,
 			      enum page_check_address_pmd_flag flag)
 {
-	pgd_t *pgd;
-	pud_t *pud;
 	pmd_t *pmd, *ret = NULL;
 
 	if (address & ~HPAGE_PMD_MASK)
 		goto out;
 
-	pgd = pgd_offset(mm, address);
-	if (!pgd_present(*pgd))
-		goto out;
-
-	pud = pud_offset(pgd, address);
-	if (!pud_present(*pud))
+	pmd = hugepage_get_pmd(mm, address);
+	if (!pmd)
 		goto out;
-
-	pmd = pmd_offset(pud, address);
 	if (pmd_none(*pmd))
 		goto out;
 	if (pmd_page(*pmd) != page)
@@ -1908,8 +1919,6 @@ static void collapse_huge_page(struct mm_struct *mm,
 				   struct vm_area_struct *vma,
 				   int node)
 {
-	pgd_t *pgd;
-	pud_t *pud;
 	pmd_t *pmd, _pmd;
 	pte_t *pte;
 	pgtable_t pgtable;
@@ -1955,16 +1964,9 @@ static void collapse_huge_page(struct mm_struct *mm,
 		goto out;
 	VM_BUG_ON(vma->vm_flags & VM_NO_THP);
 
-	pgd = pgd_offset(mm, address);
-	if (!pgd_present(*pgd))
+	pmd = hugepage_get_pmd(mm, address);
+	if (!pmd)
 		goto out;
-
-	pud = pud_offset(pgd, address);
-	if (!pud_present(*pud))
-		goto out;
-
-	pmd = pmd_offset(pud, address);
-	/* pmd can't go away or become huge under us */
 	if (!pmd_present(*pmd) || pmd_trans_huge(*pmd))
 		goto out;
 
@@ -2048,8 +2050,6 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 			       unsigned long address,
 			       struct page **hpage)
 {
-	pgd_t *pgd;
-	pud_t *pud;
 	pmd_t *pmd;
 	pte_t *pte, *_pte;
 	int ret = 0, referenced = 0, none = 0;
@@ -2060,15 +2060,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
 
 	VM_BUG_ON(address & ~HPAGE_PMD_MASK);
 
-	pgd = pgd_offset(mm, address);
-	if (!pgd_present(*pgd))
-		goto out;
-
-	pud = pud_offset(pgd, address);
-	if (!pud_present(*pud))
+	pmd = hugepage_get_pmd(mm, address);
+	if (!pmd)
 		goto out;
-
-	pmd = pmd_offset(pud, address);
 	if (!pmd_present(*pmd) || pmd_trans_huge(*pmd))
 		goto out;
 
@@ -2363,21 +2357,13 @@ void __split_huge_page_pmd(struct mm_struct *mm, pmd_t *pmd)
 static void split_huge_page_address(struct mm_struct *mm,
 				    unsigned long address)
 {
-	pgd_t *pgd;
-	pud_t *pud;
 	pmd_t *pmd;
 
 	VM_BUG_ON(!(address & ~HPAGE_PMD_MASK));
 
-	pgd = pgd_offset(mm, address);
-	if (!pgd_present(*pgd))
-		return;
-
-	pud = pud_offset(pgd, address);
-	if (!pud_present(*pud))
+	pmd = hugepage_get_pmd(mm, address);
+	if (!pmd)
 		return;
-
-	pmd = pmd_offset(pud, address);
 	if (!pmd_present(*pmd))
 		return;
 	/*
-- 
1.7.9.5

[PATCH 3/4] thp: introduce hugepage_vma_check()

[Bob Liu]

Multi place do the same check.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/huge_memory.c |   38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index e575b29..3588fec 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1913,6 +1913,20 @@ static struct page
 }
 #endif
 
+static bool hugepage_vma_check(struct vm_area_struct *vma)
+{
+	if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
+	    (vma->vm_flags & VM_NOHUGEPAGE))
+		return false;
+
+	if (!vma->anon_vma || vma->vm_ops)
+		return false;
+	if (is_vma_temporary_stack(vma))
+		return false;
+	VM_BUG_ON(vma->vm_flags & VM_NO_THP);
+	return true;
+}
+
 static void collapse_huge_page(struct mm_struct *mm,
 				   unsigned long address,
 				   struct page **hpage,
@@ -1953,17 +1967,8 @@ static void collapse_huge_page(struct mm_struct *mm,
 	hend = vma->vm_end & HPAGE_PMD_MASK;
 	if (address < hstart || address + HPAGE_PMD_SIZE > hend)
 		goto out;
-
-	if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
-	    (vma->vm_flags & VM_NOHUGEPAGE))
-		goto out;
-
-	if (!vma->anon_vma || vma->vm_ops)
-		goto out;
-	if (is_vma_temporary_stack(vma))
+	if (!hugepage_vma_check(vma))
 		goto out;
-	VM_BUG_ON(vma->vm_flags & VM_NO_THP);
-
 	pmd = hugepage_get_pmd(mm, address);
 	if (!pmd)
 		goto out;
@@ -2171,20 +2176,11 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
 			progress++;
 			break;
 		}
-
-		if ((!(vma->vm_flags & VM_HUGEPAGE) &&
-		     !khugepaged_always()) ||
-		    (vma->vm_flags & VM_NOHUGEPAGE)) {
-		skip:
+		if (!hugepage_vma_check(vma)) {
+skip:
 			progress++;
 			continue;
 		}
-		if (!vma->anon_vma || vma->vm_ops)
-			goto skip;
-		if (is_vma_temporary_stack(vma))
-			goto skip;
-		VM_BUG_ON(vma->vm_flags & VM_NO_THP);
-
 		hstart = (vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
 		hend = vma->vm_end & HPAGE_PMD_MASK;
 		if (hstart >= hend)
-- 
1.7.9.5

[PATCH 4/4] thp: cleanup: introduce mk_huge_pmd()

[Bob Liu]

Introduce mk_huge_pmd() to simple code

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/huge_memory.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 3588fec..9fd1312 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -605,6 +605,15 @@ static inline pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
 	return pmd;
 }
 
+static inline pmd_t mk_huge_pmd(struct page *page, struct vm_area_struct *vma)
+{
+	pmd_t entry;
+	entry = mk_pmd(page, vma->vm_page_prot);
+	entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
+	entry = pmd_mkhuge(entry);
+	return entry;
+}
+
 static int __do_huge_pmd_anonymous_page(struct mm_struct *mm,
 					struct vm_area_struct *vma,
 					unsigned long haddr, pmd_t *pmd,
@@ -628,9 +637,7 @@ static int __do_huge_pmd_anonymous_page(struct mm_struct *mm,
 		pte_free(mm, pgtable);
 	} else {
 		pmd_t entry;
-		entry = mk_pmd(page, vma->vm_page_prot);
-		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
-		entry = pmd_mkhuge(entry);
+		entry = mk_huge_pmd(page, vma);
 		/*
 		 * The spinlocking to take the lru_lock inside
 		 * page_add_new_anon_rmap() acts as a full memory
@@ -950,9 +957,7 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	} else {
 		pmd_t entry;
 		VM_BUG_ON(!PageHead(page));
-		entry = mk_pmd(new_page, vma->vm_page_prot);
-		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
-		entry = pmd_mkhuge(entry);
+		entry = mk_huge_pmd(new_page, vma);
 		pmdp_clear_flush(vma, haddr, pmd);
 		page_add_new_anon_rmap(new_page, vma, haddr);
 		set_pmd_at(mm, haddr, pmd, entry);
@@ -2019,9 +2024,7 @@ static void collapse_huge_page(struct mm_struct *mm,
 	__SetPageUptodate(new_page);
 	pgtable = pmd_pgtable(_pmd);
 
-	_pmd = mk_pmd(new_page, vma->vm_page_prot);
-	_pmd = maybe_pmd_mkwrite(pmd_mkdirty(_pmd), vma);
-	_pmd = pmd_mkhuge(_pmd);
+	_pmd = mk_huge_pmd(new_page, vma);
 
 	/*
 	 * spin_lock() below is not the equivalent of smp_wmb(), so
-- 
1.7.9.5

Re: [PATCH 1/4] thp: clean up __collapse_huge_page_isolate

[Bob Liu]

Sorry, should be to linux-mm instead of linux-kernel.
It's strange linux-mm not in the result by using
./scripts/get_maintainer.pl -f ./mm/huge_memory.c

On Thu, Oct 18, 2012 at 6:12 PM, Bob Liu <lliubbo@gmail.com> wrote:
> There are duplicated place using release_pte_pages().
> And release_all_pte_pages() can also be removed.
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/huge_memory.c |   37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a863af2..462d6ea 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1700,64 +1700,49 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte)
>         }
>  }
>
> -static void release_all_pte_pages(pte_t *pte)
> -{
> -       release_pte_pages(pte, pte + HPAGE_PMD_NR);
> -}
> -
>  static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>                                         unsigned long address,
>                                         pte_t *pte)
>  {
>         struct page *page;
>         pte_t *_pte;
> -       int referenced = 0, isolated = 0, none = 0;
> +       int referenced = 0, isolated = 1, none = 0;
>         for (_pte = pte; _pte < pte+HPAGE_PMD_NR;
>              _pte++, address += PAGE_SIZE) {
>                 pte_t pteval = *_pte;
>                 if (pte_none(pteval)) {
>                         if (++none <= khugepaged_max_ptes_none)
>                                 continue;
> -                       else {
> -                               release_pte_pages(pte, _pte);
> +                       else
>                                 goto out;
> -                       }
>                 }
> -               if (!pte_present(pteval) || !pte_write(pteval)) {
> -                       release_pte_pages(pte, _pte);
> +               if (!pte_present(pteval) || !pte_write(pteval))
>                         goto out;
> -               }
>                 page = vm_normal_page(vma, address, pteval);
> -               if (unlikely(!page)) {
> -                       release_pte_pages(pte, _pte);
> +               if (unlikely(!page))
>                         goto out;
> -               }
> +
>                 VM_BUG_ON(PageCompound(page));
>                 BUG_ON(!PageAnon(page));
>                 VM_BUG_ON(!PageSwapBacked(page));
>
>                 /* cannot use mapcount: can't collapse if there's a gup pin */
> -               if (page_count(page) != 1) {
> -                       release_pte_pages(pte, _pte);
> +               if (page_count(page) != 1)
>                         goto out;
> -               }
>                 /*
>                  * We can do it before isolate_lru_page because the
>                  * page can't be freed from under us. NOTE: PG_lock
>                  * is needed to serialize against split_huge_page
>                  * when invoked from the VM.
>                  */
> -               if (!trylock_page(page)) {
> -                       release_pte_pages(pte, _pte);
> +               if (!trylock_page(page))
>                         goto out;
> -               }
>                 /*
>                  * Isolate the page to avoid collapsing an hugepage
>                  * currently in use by the VM.
>                  */
>                 if (isolate_lru_page(page)) {
>                         unlock_page(page);
> -                       release_pte_pages(pte, _pte);
>                         goto out;
>                 }
>                 /* 0 stands for page_is_file_cache(page) == false */
> @@ -1770,11 +1755,11 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>                     mmu_notifier_test_young(vma->vm_mm, address))
>                         referenced = 1;
>         }
> -       if (unlikely(!referenced))
> -               release_all_pte_pages(pte);
> -       else
> -               isolated = 1;
> +       if (unlikely(!referenced)) {
>  out:
> +               release_pte_pages(pte, _pte);
> +               isolated = 0;
> +       }
>         return isolated;
>  }
>
> --
> 1.7.9.5
>
>



-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] thp: introduce hugepage_get_pmd()

[Bob Liu]

On Thu, Oct 18, 2012 at 6:12 PM, Bob Liu <lliubbo@gmail.com> wrote:
> Introduce hugepage_get_pmd() to simple code.
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/huge_memory.c |   68 ++++++++++++++++++++++--------------------------------
>  1 file changed, 27 insertions(+), 41 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 462d6ea..e575b29 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1115,6 +1115,25 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>         return ret;
>  }
>
> +static pmd_t *hugepage_get_pmd(struct mm_struct *mm, unsigned long address)
> +{
> +       pgd_t *pgd;
> +       pud_t *pud;
> +       pmd_t *pmd = NULL;
> +
> +       pgd = pgd_offset(mm, address);
> +       if (!pgd_present(*pgd))
> +               goto out;
> +
> +       pud = pud_offset(pgd, address);
> +       if (!pud_present(*pud))
> +               goto out;
> +
> +       pmd = pmd_offset(pud, address);
> +out:
> +       return pmd;
> +}
> +
>  /*
>   * Returns 1 if a given pmd maps a stable (not under splitting) thp.
>   * Returns -1 if it maps a thp under splitting. Returns 0 otherwise.
> @@ -1145,22 +1164,14 @@ pmd_t *page_check_address_pmd(struct page *page,
>                               unsigned long address,
>                               enum page_check_address_pmd_flag flag)
>  {
> -       pgd_t *pgd;
> -       pud_t *pud;
>         pmd_t *pmd, *ret = NULL;
>
>         if (address & ~HPAGE_PMD_MASK)
>                 goto out;
>
> -       pgd = pgd_offset(mm, address);
> -       if (!pgd_present(*pgd))
> -               goto out;
> -
> -       pud = pud_offset(pgd, address);
> -       if (!pud_present(*pud))
> +       pmd = hugepage_get_pmd(mm, address);
> +       if (!pmd)
>                 goto out;
> -
> -       pmd = pmd_offset(pud, address);
>         if (pmd_none(*pmd))
>                 goto out;
>         if (pmd_page(*pmd) != page)
> @@ -1908,8 +1919,6 @@ static void collapse_huge_page(struct mm_struct *mm,
>                                    struct vm_area_struct *vma,
>                                    int node)
>  {
> -       pgd_t *pgd;
> -       pud_t *pud;
>         pmd_t *pmd, _pmd;
>         pte_t *pte;
>         pgtable_t pgtable;
> @@ -1955,16 +1964,9 @@ static void collapse_huge_page(struct mm_struct *mm,
>                 goto out;
>         VM_BUG_ON(vma->vm_flags & VM_NO_THP);
>
> -       pgd = pgd_offset(mm, address);
> -       if (!pgd_present(*pgd))
> +       pmd = hugepage_get_pmd(mm, address);
> +       if (!pmd)
>                 goto out;
> -
> -       pud = pud_offset(pgd, address);
> -       if (!pud_present(*pud))
> -               goto out;
> -
> -       pmd = pmd_offset(pud, address);
> -       /* pmd can't go away or become huge under us */
>         if (!pmd_present(*pmd) || pmd_trans_huge(*pmd))
>                 goto out;
>
> @@ -2048,8 +2050,6 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
>                                unsigned long address,
>                                struct page **hpage)
>  {
> -       pgd_t *pgd;
> -       pud_t *pud;
>         pmd_t *pmd;
>         pte_t *pte, *_pte;
>         int ret = 0, referenced = 0, none = 0;
> @@ -2060,15 +2060,9 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
>
>         VM_BUG_ON(address & ~HPAGE_PMD_MASK);
>
> -       pgd = pgd_offset(mm, address);
> -       if (!pgd_present(*pgd))
> -               goto out;
> -
> -       pud = pud_offset(pgd, address);
> -       if (!pud_present(*pud))
> +       pmd = hugepage_get_pmd(mm, address);
> +       if (!pmd)
>                 goto out;
> -
> -       pmd = pmd_offset(pud, address);
>         if (!pmd_present(*pmd) || pmd_trans_huge(*pmd))
>                 goto out;
>
> @@ -2363,21 +2357,13 @@ void __split_huge_page_pmd(struct mm_struct *mm, pmd_t *pmd)
>  static void split_huge_page_address(struct mm_struct *mm,
>                                     unsigned long address)
>  {
> -       pgd_t *pgd;
> -       pud_t *pud;
>         pmd_t *pmd;
>
>         VM_BUG_ON(!(address & ~HPAGE_PMD_MASK));
>
> -       pgd = pgd_offset(mm, address);
> -       if (!pgd_present(*pgd))
> -               return;
> -
> -       pud = pud_offset(pgd, address);
> -       if (!pud_present(*pud))
> +       pmd = hugepage_get_pmd(mm, address);
> +       if (!pmd)
>                 return;
> -
> -       pmd = pmd_offset(pud, address);
>         if (!pmd_present(*pmd))
>                 return;
>         /*
> --
> 1.7.9.5
>
>



-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 3/4] thp: introduce hugepage_vma_check()

[Bob Liu]

On Thu, Oct 18, 2012 at 6:12 PM, Bob Liu <lliubbo@gmail.com> wrote:
> Multi place do the same check.
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/huge_memory.c |   38 +++++++++++++++++---------------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index e575b29..3588fec 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1913,6 +1913,20 @@ static struct page
>  }
>  #endif
>
> +static bool hugepage_vma_check(struct vm_area_struct *vma)
> +{
> +       if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
> +           (vma->vm_flags & VM_NOHUGEPAGE))
> +               return false;
> +
> +       if (!vma->anon_vma || vma->vm_ops)
> +               return false;
> +       if (is_vma_temporary_stack(vma))
> +               return false;
> +       VM_BUG_ON(vma->vm_flags & VM_NO_THP);
> +       return true;
> +}
> +
>  static void collapse_huge_page(struct mm_struct *mm,
>                                    unsigned long address,
>                                    struct page **hpage,
> @@ -1953,17 +1967,8 @@ static void collapse_huge_page(struct mm_struct *mm,
>         hend = vma->vm_end & HPAGE_PMD_MASK;
>         if (address < hstart || address + HPAGE_PMD_SIZE > hend)
>                 goto out;
> -
> -       if ((!(vma->vm_flags & VM_HUGEPAGE) && !khugepaged_always()) ||
> -           (vma->vm_flags & VM_NOHUGEPAGE))
> -               goto out;
> -
> -       if (!vma->anon_vma || vma->vm_ops)
> -               goto out;
> -       if (is_vma_temporary_stack(vma))
> +       if (!hugepage_vma_check(vma))
>                 goto out;
> -       VM_BUG_ON(vma->vm_flags & VM_NO_THP);
> -
>         pmd = hugepage_get_pmd(mm, address);
>         if (!pmd)
>                 goto out;
> @@ -2171,20 +2176,11 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
>                         progress++;
>                         break;
>                 }
> -
> -               if ((!(vma->vm_flags & VM_HUGEPAGE) &&
> -                    !khugepaged_always()) ||
> -                   (vma->vm_flags & VM_NOHUGEPAGE)) {
> -               skip:
> +               if (!hugepage_vma_check(vma)) {
> +skip:
>                         progress++;
>                         continue;
>                 }
> -               if (!vma->anon_vma || vma->vm_ops)
> -                       goto skip;
> -               if (is_vma_temporary_stack(vma))
> -                       goto skip;
> -               VM_BUG_ON(vma->vm_flags & VM_NO_THP);
> -
>                 hstart = (vma->vm_start + ~HPAGE_PMD_MASK) & HPAGE_PMD_MASK;
>                 hend = vma->vm_end & HPAGE_PMD_MASK;
>                 if (hstart >= hend)
> --
> 1.7.9.5
>
>



-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 4/4] thp: cleanup: introduce mk_huge_pmd()

[Bob Liu]

On Thu, Oct 18, 2012 at 6:12 PM, Bob Liu <lliubbo@gmail.com> wrote:
> Introduce mk_huge_pmd() to simple code
>
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/huge_memory.c |   21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 3588fec..9fd1312 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -605,6 +605,15 @@ static inline pmd_t maybe_pmd_mkwrite(pmd_t pmd, struct vm_area_struct *vma)
>         return pmd;
>  }
>
> +static inline pmd_t mk_huge_pmd(struct page *page, struct vm_area_struct *vma)
> +{
> +       pmd_t entry;
> +       entry = mk_pmd(page, vma->vm_page_prot);
> +       entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> +       entry = pmd_mkhuge(entry);
> +       return entry;
> +}
> +
>  static int __do_huge_pmd_anonymous_page(struct mm_struct *mm,
>                                         struct vm_area_struct *vma,
>                                         unsigned long haddr, pmd_t *pmd,
> @@ -628,9 +637,7 @@ static int __do_huge_pmd_anonymous_page(struct mm_struct *mm,
>                 pte_free(mm, pgtable);
>         } else {
>                 pmd_t entry;
> -               entry = mk_pmd(page, vma->vm_page_prot);
> -               entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> -               entry = pmd_mkhuge(entry);
> +               entry = mk_huge_pmd(page, vma);
>                 /*
>                  * The spinlocking to take the lru_lock inside
>                  * page_add_new_anon_rmap() acts as a full memory
> @@ -950,9 +957,7 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
>         } else {
>                 pmd_t entry;
>                 VM_BUG_ON(!PageHead(page));
> -               entry = mk_pmd(new_page, vma->vm_page_prot);
> -               entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> -               entry = pmd_mkhuge(entry);
> +               entry = mk_huge_pmd(new_page, vma);
>                 pmdp_clear_flush(vma, haddr, pmd);
>                 page_add_new_anon_rmap(new_page, vma, haddr);
>                 set_pmd_at(mm, haddr, pmd, entry);
> @@ -2019,9 +2024,7 @@ static void collapse_huge_page(struct mm_struct *mm,
>         __SetPageUptodate(new_page);
>         pgtable = pmd_pgtable(_pmd);
>
> -       _pmd = mk_pmd(new_page, vma->vm_page_prot);
> -       _pmd = maybe_pmd_mkwrite(pmd_mkdirty(_pmd), vma);
> -       _pmd = pmd_mkhuge(_pmd);
> +       _pmd = mk_huge_pmd(new_page, vma);
>
>         /*
>          * spin_lock() below is not the equivalent of smp_wmb(), so
> --
> 1.7.9.5
>
>



-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] thp: clean up __collapse_huge_page_isolate

[David Rientjes]

On Thu, 18 Oct 2012, Bob Liu wrote:

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index a863af2..462d6ea 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1700,64 +1700,49 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte)
>  	}
>  }
>  
> -static void release_all_pte_pages(pte_t *pte)
> -{
> -	release_pte_pages(pte, pte + HPAGE_PMD_NR);
> -}
> -
>  static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>  					unsigned long address,
>  					pte_t *pte)
>  {
>  	struct page *page;
>  	pte_t *_pte;
> -	int referenced = 0, isolated = 0, none = 0;
> +	int referenced = 0, isolated = 1, none = 0;
>  	for (_pte = pte; _pte < pte+HPAGE_PMD_NR;
>  	     _pte++, address += PAGE_SIZE) {
>  		pte_t pteval = *_pte;
>  		if (pte_none(pteval)) {
>  			if (++none <= khugepaged_max_ptes_none)
>  				continue;
> -			else {
> -				release_pte_pages(pte, _pte);
> +			else
>  				goto out;
> -			}
>  		}
> -		if (!pte_present(pteval) || !pte_write(pteval)) {
> -			release_pte_pages(pte, _pte);
> +		if (!pte_present(pteval) || !pte_write(pteval))
>  			goto out;
> -		}
>  		page = vm_normal_page(vma, address, pteval);
> -		if (unlikely(!page)) {
> -			release_pte_pages(pte, _pte);
> +		if (unlikely(!page))
>  			goto out;
> -		}
> +
>  		VM_BUG_ON(PageCompound(page));
>  		BUG_ON(!PageAnon(page));
>  		VM_BUG_ON(!PageSwapBacked(page));
>  
>  		/* cannot use mapcount: can't collapse if there's a gup pin */
> -		if (page_count(page) != 1) {
> -			release_pte_pages(pte, _pte);
> +		if (page_count(page) != 1)
>  			goto out;
> -		}
>  		/*
>  		 * We can do it before isolate_lru_page because the
>  		 * page can't be freed from under us. NOTE: PG_lock
>  		 * is needed to serialize against split_huge_page
>  		 * when invoked from the VM.
>  		 */
> -		if (!trylock_page(page)) {
> -			release_pte_pages(pte, _pte);
> +		if (!trylock_page(page))
>  			goto out;
> -		}
>  		/*
>  		 * Isolate the page to avoid collapsing an hugepage
>  		 * currently in use by the VM.
>  		 */
>  		if (isolate_lru_page(page)) {
>  			unlock_page(page);
> -			release_pte_pages(pte, _pte);
>  			goto out;
>  		}
>  		/* 0 stands for page_is_file_cache(page) == false */
> @@ -1770,11 +1755,11 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>  		    mmu_notifier_test_young(vma->vm_mm, address))
>  			referenced = 1;
>  	}
> -	if (unlikely(!referenced))
> -		release_all_pte_pages(pte);
> -	else
> -		isolated = 1;
> +	if (unlikely(!referenced)) {
>  out:

Labels inside of conditionals are never good if they can be avoided and in 
this case you can avoid it by doing

		if (likely(referenced))
			return 1;
	out:
		...

> +		release_pte_pages(pte, _pte);
> +		isolated = 0;
> +	}
>  	return isolated;
>  }
>  

Re: [PATCH 2/4] thp: introduce hugepage_get_pmd()

[David Rientjes]

On Thu, 18 Oct 2012, Bob Liu wrote:

> Introduce hugepage_get_pmd() to simple code.
> 

I don't see this as simple just because you're removing more lines, I find 
pagetable walks to be harder to read when split up like this and the "get" 
part implies you're grabbing a reference on the pmd, which you're not.

Re: [PATCH 3/4] thp: introduce hugepage_vma_check()

[David Rientjes]

On Thu, 18 Oct 2012, Bob Liu wrote:

> Multi place do the same check.
> 

This only succeeds in removing four lines and makes the code more 
difficult to follow, nack.

Re: [PATCH 3/4] thp: introduce hugepage_vma_check()

[Andrew Morton]

On Thu, 18 Oct 2012 13:16:04 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> On Thu, 18 Oct 2012, Bob Liu wrote:
> 
> > Multi place do the same check.
> > 
> 
> This only succeeds in removing four lines and makes the code more 
> difficult to follow

I like it.  It takes a bunch of code sequences which may or may not be the
same and removes all doubt, while making the call sites clearer.

Re: [PATCH 1/4] thp: clean up __collapse_huge_page_isolate

[Ni zhan Chen]

On 10/18/2012 06:20 PM, Bob Liu wrote:
> Sorry, should be to linux-mm instead of linux-kernel.
> It's strange linux-mm not in the result by using
> ./scripts/get_maintainer.pl -f ./mm/huge_memory.c
>
> On Thu, Oct 18, 2012 at 6:12 PM, Bob Liu <lliubbo@gmail.com> wrote:
>> There are duplicated place using release_pte_pages().
>> And release_all_pte_pages() can also be removed.

this cleanup looks reasonable to me.

>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>   mm/huge_memory.c |   37 +++++++++++--------------------------
>>   1 file changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index a863af2..462d6ea 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1700,64 +1700,49 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte)
>>          }
>>   }
>>
>> -static void release_all_pte_pages(pte_t *pte)
>> -{
>> -       release_pte_pages(pte, pte + HPAGE_PMD_NR);
>> -}
>> -
>>   static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>                                          unsigned long address,
>>                                          pte_t *pte)
>>   {
>>          struct page *page;
>>          pte_t *_pte;
>> -       int referenced = 0, isolated = 0, none = 0;
>> +       int referenced = 0, isolated = 1, none = 0;
>>          for (_pte = pte; _pte < pte+HPAGE_PMD_NR;
>>               _pte++, address += PAGE_SIZE) {
>>                  pte_t pteval = *_pte;
>>                  if (pte_none(pteval)) {
>>                          if (++none <= khugepaged_max_ptes_none)
>>                                  continue;
>> -                       else {
>> -                               release_pte_pages(pte, _pte);
>> +                       else
>>                                  goto out;
>> -                       }
>>                  }
>> -               if (!pte_present(pteval) || !pte_write(pteval)) {
>> -                       release_pte_pages(pte, _pte);
>> +               if (!pte_present(pteval) || !pte_write(pteval))
>>                          goto out;
>> -               }
>>                  page = vm_normal_page(vma, address, pteval);
>> -               if (unlikely(!page)) {
>> -                       release_pte_pages(pte, _pte);
>> +               if (unlikely(!page))
>>                          goto out;
>> -               }
>> +
>>                  VM_BUG_ON(PageCompound(page));
>>                  BUG_ON(!PageAnon(page));
>>                  VM_BUG_ON(!PageSwapBacked(page));
>>
>>                  /* cannot use mapcount: can't collapse if there's a gup pin */
>> -               if (page_count(page) != 1) {
>> -                       release_pte_pages(pte, _pte);
>> +               if (page_count(page) != 1)
>>                          goto out;
>> -               }
>>                  /*
>>                   * We can do it before isolate_lru_page because the
>>                   * page can't be freed from under us. NOTE: PG_lock
>>                   * is needed to serialize against split_huge_page
>>                   * when invoked from the VM.
>>                   */
>> -               if (!trylock_page(page)) {
>> -                       release_pte_pages(pte, _pte);
>> +               if (!trylock_page(page))
>>                          goto out;
>> -               }
>>                  /*
>>                   * Isolate the page to avoid collapsing an hugepage
>>                   * currently in use by the VM.
>>                   */
>>                  if (isolate_lru_page(page)) {
>>                          unlock_page(page);
>> -                       release_pte_pages(pte, _pte);
>>                          goto out;
>>                  }
>>                  /* 0 stands for page_is_file_cache(page) == false */
>> @@ -1770,11 +1755,11 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>                      mmu_notifier_test_young(vma->vm_mm, address))
>>                          referenced = 1;
>>          }
>> -       if (unlikely(!referenced))
>> -               release_all_pte_pages(pte);
>> -       else
>> -               isolated = 1;
>> +       if (unlikely(!referenced)) {
>>   out:
>> +               release_pte_pages(pte, _pte);
>> +               isolated = 0;
>> +       }
>>          return isolated;
>>   }
>>
>> --
>> 1.7.9.5
>>
>>
>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/4] thp: clean up __collapse_huge_page_isolate

[Bob Liu]

On Fri, Oct 19, 2012 at 4:13 AM, David Rientjes <rientjes@google.com> wrote:
> On Thu, 18 Oct 2012, Bob Liu wrote:
>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index a863af2..462d6ea 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1700,64 +1700,49 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte)
>>       }
>>  }
>>
>> -static void release_all_pte_pages(pte_t *pte)
>> -{
>> -     release_pte_pages(pte, pte + HPAGE_PMD_NR);
>> -}
>> -
>>  static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>                                       unsigned long address,
>>                                       pte_t *pte)
>>  {
>>       struct page *page;
>>       pte_t *_pte;
>> -     int referenced = 0, isolated = 0, none = 0;
>> +     int referenced = 0, isolated = 1, none = 0;
>>       for (_pte = pte; _pte < pte+HPAGE_PMD_NR;
>>            _pte++, address += PAGE_SIZE) {
>>               pte_t pteval = *_pte;
>>               if (pte_none(pteval)) {
>>                       if (++none <= khugepaged_max_ptes_none)
>>                               continue;
>> -                     else {
>> -                             release_pte_pages(pte, _pte);
>> +                     else
>>                               goto out;
>> -                     }
>>               }
>> -             if (!pte_present(pteval) || !pte_write(pteval)) {
>> -                     release_pte_pages(pte, _pte);
>> +             if (!pte_present(pteval) || !pte_write(pteval))
>>                       goto out;
>> -             }
>>               page = vm_normal_page(vma, address, pteval);
>> -             if (unlikely(!page)) {
>> -                     release_pte_pages(pte, _pte);
>> +             if (unlikely(!page))
>>                       goto out;
>> -             }
>> +
>>               VM_BUG_ON(PageCompound(page));
>>               BUG_ON(!PageAnon(page));
>>               VM_BUG_ON(!PageSwapBacked(page));
>>
>>               /* cannot use mapcount: can't collapse if there's a gup pin */
>> -             if (page_count(page) != 1) {
>> -                     release_pte_pages(pte, _pte);
>> +             if (page_count(page) != 1)
>>                       goto out;
>> -             }
>>               /*
>>                * We can do it before isolate_lru_page because the
>>                * page can't be freed from under us. NOTE: PG_lock
>>                * is needed to serialize against split_huge_page
>>                * when invoked from the VM.
>>                */
>> -             if (!trylock_page(page)) {
>> -                     release_pte_pages(pte, _pte);
>> +             if (!trylock_page(page))
>>                       goto out;
>> -             }
>>               /*
>>                * Isolate the page to avoid collapsing an hugepage
>>                * currently in use by the VM.
>>                */
>>               if (isolate_lru_page(page)) {
>>                       unlock_page(page);
>> -                     release_pte_pages(pte, _pte);
>>                       goto out;
>>               }
>>               /* 0 stands for page_is_file_cache(page) == false */
>> @@ -1770,11 +1755,11 @@ static int __collapse_huge_page_isolate(struct vm_area_struct *vma,
>>                   mmu_notifier_test_young(vma->vm_mm, address))
>>                       referenced = 1;
>>       }
>> -     if (unlikely(!referenced))
>> -             release_all_pte_pages(pte);
>> -     else
>> -             isolated = 1;
>> +     if (unlikely(!referenced)) {
>>  out:
>
> Labels inside of conditionals are never good if they can be avoided and in
> this case you can avoid it by doing
>
>                 if (likely(referenced))
>                         return 1;
>         out:
>                 ...
>

Will be updated, thanks.

>> +             release_pte_pages(pte, _pte);
>> +             isolated = 0;
>> +     }
>>       return isolated;
>>  }
>>

-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/4] thp: introduce hugepage_get_pmd()

[Bob Liu]

On Fri, Oct 19, 2012 at 4:15 AM, David Rientjes <rientjes@google.com> wrote:
> On Thu, 18 Oct 2012, Bob Liu wrote:
>
>> Introduce hugepage_get_pmd() to simple code.
>>
>
> I don't see this as simple just because you're removing more lines, I find
> pagetable walks to be harder to read when split up like this and the "get"
> part implies you're grabbing a reference on the pmd, which you're not.

There are really too much place(at least four) do pagetable walks in thp.

My original idea was putting them into one pagewalk fucntion together,
that would be
clearer and more readable.

But there are several different pmd checks, no better way found to
collect them currently.
i'll keep on working on that.

Thanks.
-- 
Regards,
--Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>