[PATCH] mm/hugetlb: split out is_hugetlb_entry_migration_or_hwpoison

[PATCH] mm/hugetlb: split out is_hugetlb_entry_migration_or_hwpoison

[Wanpeng Li]

From: Wanpeng Li <liwp@linux.vnet.ibm.com>

Code was duplicated in two functions, clean it up.

Signed-off-by: Wanpeng Li <liwp.linux@gmail.com>
---
 mm/hugetlb.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index e198831..4f9ce3f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2282,30 +2282,28 @@ nomem:
 	return -ENOMEM;
 }
 
-static int is_hugetlb_entry_migration(pte_t pte)
+static int is_hugetlb_entry_migration_or_hwpoison(pte_t pte, bool migration)
 {
 	swp_entry_t swp;
 
 	if (huge_pte_none(pte) || pte_present(pte))
 		return 0;
 	swp = pte_to_swp_entry(pte);
-	if (non_swap_entry(swp) && is_migration_entry(swp))
+	if (non_swap_entry(pte) && ((migration && is_migration_entry(pte))
+				|| !migration && is_hwpoison_entry(pte)))
 		return 1;
 	else
 		return 0;
 }
 
-static int is_hugetlb_entry_hwpoisoned(pte_t pte)
+static int is_hugetlb_entry_migration(pte_t pte)
 {
-	swp_entry_t swp;
+	return is_hugetlb_entry_migration_or_hwpoison(pte, true);
+}
 
-	if (huge_pte_none(pte) || pte_present(pte))
-		return 0;
-	swp = pte_to_swp_entry(pte);
-	if (non_swap_entry(swp) && is_hwpoison_entry(swp))
-		return 1;
-	else
-		return 0;
+static int is_hugetlb_entry_hwpoisoned(pte_t pte)
+{
+	return is_hugetlb_entry_migration_or_hwpoison(pte, false);
 }
 
 void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start,
-- 
1.7.5.4

Re: [PATCH] mm/hugetlb: split out is_hugetlb_entry_migration_or_hwpoison

[David Rientjes]

On Mon, 9 Jul 2012, Wanpeng Li wrote:

> From: Wanpeng Li <liwp@linux.vnet.ibm.com>
> 
> Code was duplicated in two functions, clean it up.
> 
> Signed-off-by: Wanpeng Li <liwp.linux@gmail.com>
> ---
>  mm/hugetlb.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)

Nack, this makes the code more convoluted then the hugetlb code already is 
and savings two lines of code isn't worth it.

Re: [PATCH] mm/hugetlb: split out is_hugetlb_entry_migration_or_hwpoison

[Mel Gorman]

On Mon, Jul 09, 2012 at 06:12:41PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <liwp@linux.vnet.ibm.com>
> 
> Code was duplicated in two functions, clean it up.
> 
> Signed-off-by: Wanpeng Li <liwp.linux@gmail.com>

is_hugetlb_entry_migration() now returns true for hwpoisoned pages. In
this block

                if (unlikely(is_hugetlb_entry_migration(entry))) {
                        migration_entry_wait(mm, (pmd_t *)ptep, address);
                        return 0;

we now will call migration_entry_wait and return 0 to the fault handler
instead of VM_FAULT_HWPOISON_LARGE | VM_FAULT_SET_HINDEX(h - hstates).
By co-incidence this might work because migration of hugetlb happens for
poisoned pages but it would be just a co-incidence. Some other change in
the future such as better support for memory hotplug of regions backed
by hugetlbfs may break it again.

Superficially, this patch looks broken and the changelog contains
no motivation as to why this patch should be merged such as being a
pre-requisite for another fix or feature.  It just looks like churn for
the sake of churn. It might be just me but it feels like I'm seeing a lot
more of this style of patch recently on linux-mm and review bandwidth is
not infinite :(

Nak.

-- 
Mel Gorman
SUSE Labs