[PATCH 1/3] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[PATCH 1/3] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Pingfan Liu]

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Jan Kara <jack@suse.cz>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/migrate.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8992741..c2ec614 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2230,7 +2230,6 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 		if (pte_none(pte)) {
 			mpfn = MIGRATE_PFN_MIGRATE;
 			migrate->cpages++;
-			pfn = 0;
 			goto next;
 		}
 
@@ -2255,7 +2254,6 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			if (is_zero_pfn(pfn)) {
 				mpfn = MIGRATE_PFN_MIGRATE;
 				migrate->cpages++;
-				pfn = 0;
 				goto next;
 			}
 			page = vm_normal_page(migrate->vma, addr, pte);
@@ -2265,10 +2263,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 
 		/* FIXME support THP */
 		if (!page || !page->mapping || PageTransCompound(page)) {
-			mpfn = pfn = 0;
+			mpfn = 0;
 			goto next;
 		}
-		pfn = page_to_pfn(page);
 
 		/*
 		 * By getting a reference on the page we pin it and that blocks
-- 
2.7.5

[PATCH 2/3] mm/migrate: see hole as invalid source page

[Pingfan Liu]

MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate.
As for hole, there is no valid pfn, not to mention migration.

Before this patch, hole has already relied on the following code to be
filtered out. Hence it is more reasonable to see hole as invalid source
page.
migrate_vma_prepare()
{
		struct page *page = migrate_pfn_to_page(migrate->src[i]);

		if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE))
		     \_ this condition
}

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Jan Kara <jack@suse.cz>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/migrate.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index c2ec614..832483f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2136,10 +2136,9 @@ static int migrate_vma_collect_hole(unsigned long start,
 	unsigned long addr;
 
 	for (addr = start & PAGE_MASK; addr < end; addr += PAGE_SIZE) {
-		migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE;
+		migrate->src[migrate->npages] = 0;
 		migrate->dst[migrate->npages] = 0;
 		migrate->npages++;
-		migrate->cpages++;
 	}
 
 	return 0;
@@ -2228,8 +2227,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 		pfn = pte_pfn(pte);
 
 		if (pte_none(pte)) {
-			mpfn = MIGRATE_PFN_MIGRATE;
-			migrate->cpages++;
+			mpfn = 0;
 			goto next;
 		}
 
-- 
2.7.5

[PATCH 3/3] mm/migrate: remove the duplicated code migrate_vma_collect_hole()

[Pingfan Liu]

After the previous patch which sees hole as invalid source,
migrate_vma_collect_hole() has the same code as migrate_vma_collect_skip().
Removing the duplicated code.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Jan Kara <jack@suse.cz>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/migrate.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 832483f..95e038d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2128,22 +2128,6 @@ struct migrate_vma {
 	unsigned long		end;
 };
 
-static int migrate_vma_collect_hole(unsigned long start,
-				    unsigned long end,
-				    struct mm_walk *walk)
-{
-	struct migrate_vma *migrate = walk->private;
-	unsigned long addr;
-
-	for (addr = start & PAGE_MASK; addr < end; addr += PAGE_SIZE) {
-		migrate->src[migrate->npages] = 0;
-		migrate->dst[migrate->npages] = 0;
-		migrate->npages++;
-	}
-
-	return 0;
-}
-
 static int migrate_vma_collect_skip(unsigned long start,
 				    unsigned long end,
 				    struct mm_walk *walk)
@@ -2173,7 +2157,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 
 again:
 	if (pmd_none(*pmdp))
-		return migrate_vma_collect_hole(start, end, walk);
+		return migrate_vma_collect_skip(start, end, walk);
 
 	if (pmd_trans_huge(*pmdp)) {
 		struct page *page;
@@ -2206,7 +2190,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 				return migrate_vma_collect_skip(start, end,
 								walk);
 			if (pmd_none(*pmdp))
-				return migrate_vma_collect_hole(start, end,
+				return migrate_vma_collect_skip(start, end,
 								walk);
 		}
 	}
@@ -2337,7 +2321,7 @@ static void migrate_vma_collect(struct migrate_vma *migrate)
 
 	mm_walk.pmd_entry = migrate_vma_collect_pmd;
 	mm_walk.pte_entry = NULL;
-	mm_walk.pte_hole = migrate_vma_collect_hole;
+	mm_walk.pte_hole = migrate_vma_collect_skip;
 	mm_walk.hugetlb_entry = NULL;
 	mm_walk.test_walk = NULL;
 	mm_walk.vma = migrate->vma;
-- 
2.7.5

Re: [PATCH 1/3] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Matthew Wilcox]

This needs something beyond the subject line.  Maybe ...

After these assignments, we either restart the loop with a fresh variable,
or we assign to the variable again without using the value we've assigned.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

>  			goto next;
>  		}
> -		pfn = page_to_pfn(page);

After you've done all this, as far as I can tell, the 'pfn' variable is
only used in one arm of the conditions, so it can be moved there.

ie something like:

-               unsigned long mpfn, pfn;
+               unsigned long mpfn;
...
-               pfn = pte_pfn(pte);
...
+                       unsigned long pfn = pte_pfn(pte);
+

Re: [PATCH 1/3] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Pingfan Liu]

On Tue, Aug 06, 2019 at 06:35:03AM -0700, Matthew Wilcox wrote:
> 
> This needs something beyond the subject line.  Maybe ...
> 
> After these assignments, we either restart the loop with a fresh variable,
> or we assign to the variable again without using the value we've assigned.
> 
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> >  			goto next;
> >  		}
> > -		pfn = page_to_pfn(page);
> 
> After you've done all this, as far as I can tell, the 'pfn' variable is
> only used in one arm of the conditions, so it can be moved there.
> 
> ie something like:
> 
> -               unsigned long mpfn, pfn;
> +               unsigned long mpfn;
> ...
> -               pfn = pte_pfn(pte);
> ...
> +                       unsigned long pfn = pte_pfn(pte);
> +
> 
This makes code better. Thank you for the suggestion. Will send v2 for
this patch.

Regards,
	Pingfan

[PATCHv2] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Pingfan Liu]

Clean up useless 'pfn' variable.

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Jan Kara <jack@suse.cz>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
To: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/migrate.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 8992741..d483a55 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2225,17 +2225,15 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 		pte_t pte;
 
 		pte = *ptep;
-		pfn = pte_pfn(pte);
 
 		if (pte_none(pte)) {
 			mpfn = MIGRATE_PFN_MIGRATE;
 			migrate->cpages++;
-			pfn = 0;
 			goto next;
 		}
 
 		if (!pte_present(pte)) {
-			mpfn = pfn = 0;
+			mpfn = 0;
 
 			/*
 			 * Only care about unaddressable device page special
@@ -2252,10 +2250,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 			if (is_write_device_private_entry(entry))
 				mpfn |= MIGRATE_PFN_WRITE;
 		} else {
+			pfn = pte_pfn(pte);
 			if (is_zero_pfn(pfn)) {
 				mpfn = MIGRATE_PFN_MIGRATE;
 				migrate->cpages++;
-				pfn = 0;
 				goto next;
 			}
 			page = vm_normal_page(migrate->vma, addr, pte);
@@ -2265,10 +2263,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 
 		/* FIXME support THP */
 		if (!page || !page->mapping || PageTransCompound(page)) {
-			mpfn = pfn = 0;
+			mpfn = 0;
 			goto next;
 		}
-		pfn = page_to_pfn(page);
 
 		/*
 		 * By getting a reference on the page we pin it and that blocks
-- 
2.7.5

Re: [PATCHv2] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Ralph Campbell]

On 8/7/19 1:41 AM, Pingfan Liu wrote:
> Clean up useless 'pfn' variable.
> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jan Kara <jack@suse.cz>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> To: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>   mm/migrate.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8992741..d483a55 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2225,17 +2225,15 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>   		pte_t pte;
>   
>   		pte = *ptep;
> -		pfn = pte_pfn(pte);
>   
>   		if (pte_none(pte)) {
>   			mpfn = MIGRATE_PFN_MIGRATE;
>   			migrate->cpages++;
> -			pfn = 0;
>   			goto next;
>   		}
>   
>   		if (!pte_present(pte)) {
> -			mpfn = pfn = 0;
> +			mpfn = 0;
>   
>   			/*
>   			 * Only care about unaddressable device page special
> @@ -2252,10 +2250,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>   			if (is_write_device_private_entry(entry))
>   				mpfn |= MIGRATE_PFN_WRITE;
>   		} else {
> +			pfn = pte_pfn(pte);
>   			if (is_zero_pfn(pfn)) {
>   				mpfn = MIGRATE_PFN_MIGRATE;
>   				migrate->cpages++;
> -				pfn = 0;
>   				goto next;
>   			}
>   			page = vm_normal_page(migrate->vma, addr, pte);
> @@ -2265,10 +2263,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>   
>   		/* FIXME support THP */
>   		if (!page || !page->mapping || PageTransCompound(page)) {
> -			mpfn = pfn = 0;
> +			mpfn = 0;
>   			goto next;
>   		}
> -		pfn = page_to_pfn(page);
>   
>   		/*
>   		 * By getting a reference on the page we pin it and that blocks
> 

Thanks, I was planning to do this too.
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>

Re: [PATCHv2] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Jerome Glisse]

On Wed, Aug 07, 2019 at 04:41:12PM +0800, Pingfan Liu wrote:
> Clean up useless 'pfn' variable.

NAK there is a bug see below:

> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jan Kara <jack@suse.cz>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> To: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/migrate.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8992741..d483a55 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2225,17 +2225,15 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  		pte_t pte;
>  
>  		pte = *ptep;
> -		pfn = pte_pfn(pte);
>  
>  		if (pte_none(pte)) {
>  			mpfn = MIGRATE_PFN_MIGRATE;
>  			migrate->cpages++;
> -			pfn = 0;
>  			goto next;
>  		}
>  
>  		if (!pte_present(pte)) {
> -			mpfn = pfn = 0;
> +			mpfn = 0;
>  
>  			/*
>  			 * Only care about unaddressable device page special
> @@ -2252,10 +2250,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  			if (is_write_device_private_entry(entry))
>  				mpfn |= MIGRATE_PFN_WRITE;
>  		} else {
> +			pfn = pte_pfn(pte);
>  			if (is_zero_pfn(pfn)) {
>  				mpfn = MIGRATE_PFN_MIGRATE;
>  				migrate->cpages++;
> -				pfn = 0;
>  				goto next;
>  			}
>  			page = vm_normal_page(migrate->vma, addr, pte);
> @@ -2265,10 +2263,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  
>  		/* FIXME support THP */
>  		if (!page || !page->mapping || PageTransCompound(page)) {
> -			mpfn = pfn = 0;
> +			mpfn = 0;
>  			goto next;
>  		}
> -		pfn = page_to_pfn(page);

You can not remove that one ! Otherwise it will break the device
private case.

Re: [PATCH 2/3] mm/migrate: see hole as invalid source page

[Jerome Glisse]

On Tue, Aug 06, 2019 at 04:00:10PM +0800, Pingfan Liu wrote:
> MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate.
> As for hole, there is no valid pfn, not to mention migration.
> 
> Before this patch, hole has already relied on the following code to be
> filtered out. Hence it is more reasonable to see hole as invalid source
> page.
> migrate_vma_prepare()
> {
> 		struct page *page = migrate_pfn_to_page(migrate->src[i]);
> 
> 		if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE))
> 		     \_ this condition
> }

NAK you break the API, MIGRATE_PFN_MIGRATE is use for 2 things,
first it allow the collection code to mark entry that can be
migrated, then it use by driver to allow driver to skip migration
for some entry (for whatever reason the driver might have), we
still need to keep the entry and not clear it so that we can
cleanup thing (ie remove migration pte entry).

> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jan Kara <jack@suse.cz>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> To: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/migrate.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index c2ec614..832483f 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2136,10 +2136,9 @@ static int migrate_vma_collect_hole(unsigned long start,
>  	unsigned long addr;
>  
>  	for (addr = start & PAGE_MASK; addr < end; addr += PAGE_SIZE) {
> -		migrate->src[migrate->npages] = MIGRATE_PFN_MIGRATE;
> +		migrate->src[migrate->npages] = 0;
>  		migrate->dst[migrate->npages] = 0;
>  		migrate->npages++;
> -		migrate->cpages++;
>  	}
>  
>  	return 0;
> @@ -2228,8 +2227,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  		pfn = pte_pfn(pte);
>  
>  		if (pte_none(pte)) {
> -			mpfn = MIGRATE_PFN_MIGRATE;
> -			migrate->cpages++;
> +			mpfn = 0;
>  			goto next;
>  		}
>  
> -- 
> 2.7.5
> 

Re: [PATCH 3/3] mm/migrate: remove the duplicated code migrate_vma_collect_hole()

[Jerome Glisse]

On Tue, Aug 06, 2019 at 04:00:11PM +0800, Pingfan Liu wrote:
> After the previous patch which sees hole as invalid source,
> migrate_vma_collect_hole() has the same code as migrate_vma_collect_skip().
> Removing the duplicated code.

NAK this one too given previous NAK.

> 
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jan Kara <jack@suse.cz>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> To: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/migrate.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 832483f..95e038d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2128,22 +2128,6 @@ struct migrate_vma {
>  	unsigned long		end;
>  };
>  
> -static int migrate_vma_collect_hole(unsigned long start,
> -				    unsigned long end,
> -				    struct mm_walk *walk)
> -{
> -	struct migrate_vma *migrate = walk->private;
> -	unsigned long addr;
> -
> -	for (addr = start & PAGE_MASK; addr < end; addr += PAGE_SIZE) {
> -		migrate->src[migrate->npages] = 0;
> -		migrate->dst[migrate->npages] = 0;
> -		migrate->npages++;
> -	}
> -
> -	return 0;
> -}
> -
>  static int migrate_vma_collect_skip(unsigned long start,
>  				    unsigned long end,
>  				    struct mm_walk *walk)
> @@ -2173,7 +2157,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  
>  again:
>  	if (pmd_none(*pmdp))
> -		return migrate_vma_collect_hole(start, end, walk);
> +		return migrate_vma_collect_skip(start, end, walk);
>  
>  	if (pmd_trans_huge(*pmdp)) {
>  		struct page *page;
> @@ -2206,7 +2190,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>  				return migrate_vma_collect_skip(start, end,
>  								walk);
>  			if (pmd_none(*pmdp))
> -				return migrate_vma_collect_hole(start, end,
> +				return migrate_vma_collect_skip(start, end,
>  								walk);
>  		}
>  	}
> @@ -2337,7 +2321,7 @@ static void migrate_vma_collect(struct migrate_vma *migrate)
>  
>  	mm_walk.pmd_entry = migrate_vma_collect_pmd;
>  	mm_walk.pte_entry = NULL;
> -	mm_walk.pte_hole = migrate_vma_collect_hole;
> +	mm_walk.pte_hole = migrate_vma_collect_skip;
>  	mm_walk.hugetlb_entry = NULL;
>  	mm_walk.test_walk = NULL;
>  	mm_walk.vma = migrate->vma;
> -- 
> 2.7.5
> 

Re: [PATCHv2] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Ralph Campbell]

On 8/15/19 10:19 AM, Jerome Glisse wrote:
> On Wed, Aug 07, 2019 at 04:41:12PM +0800, Pingfan Liu wrote:
>> Clean up useless 'pfn' variable.
> 
> NAK there is a bug see below:
> 
>>
>> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> To: linux-mm@kvack.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   mm/migrate.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 8992741..d483a55 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -2225,17 +2225,15 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>>   		pte_t pte;
>>   
>>   		pte = *ptep;
>> -		pfn = pte_pfn(pte);
>>   
>>   		if (pte_none(pte)) {
>>   			mpfn = MIGRATE_PFN_MIGRATE;
>>   			migrate->cpages++;
>> -			pfn = 0;
>>   			goto next;
>>   		}
>>   
>>   		if (!pte_present(pte)) {
>> -			mpfn = pfn = 0;
>> +			mpfn = 0;
>>   
>>   			/*
>>   			 * Only care about unaddressable device page special
>> @@ -2252,10 +2250,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>>   			if (is_write_device_private_entry(entry))
>>   				mpfn |= MIGRATE_PFN_WRITE;
>>   		} else {
>> +			pfn = pte_pfn(pte);
>>   			if (is_zero_pfn(pfn)) {
>>   				mpfn = MIGRATE_PFN_MIGRATE;
>>   				migrate->cpages++;
>> -				pfn = 0;
>>   				goto next;
>>   			}
>>   			page = vm_normal_page(migrate->vma, addr, pte);
>> @@ -2265,10 +2263,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
>>   
>>   		/* FIXME support THP */
>>   		if (!page || !page->mapping || PageTransCompound(page)) {
>> -			mpfn = pfn = 0;
>> +			mpfn = 0;
>>   			goto next;
>>   		}
>> -		pfn = page_to_pfn(page);
> 
> You can not remove that one ! Otherwise it will break the device
> private case.
> 

I don't understand. The only use of "pfn" I see is in the "else"
clause above where it is set just before using it.

Re: [PATCHv2] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Jerome Glisse]

On Thu, Aug 15, 2019 at 12:23:44PM -0700, Ralph Campbell wrote:
> 
> On 8/15/19 10:19 AM, Jerome Glisse wrote:
> > On Wed, Aug 07, 2019 at 04:41:12PM +0800, Pingfan Liu wrote:
> > > Clean up useless 'pfn' variable.
> > 
> > NAK there is a bug see below:
> > 
> > > 
> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Mel Gorman <mgorman@techsingularity.net>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > Cc: Michal Hocko <mhocko@suse.com>
> > > Cc: Mike Kravetz <mike.kravetz@oracle.com>
> > > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > > Cc: Matthew Wilcox <willy@infradead.org>
> > > To: linux-mm@kvack.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >   mm/migrate.c | 9 +++------
> > >   1 file changed, 3 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > index 8992741..d483a55 100644
> > > --- a/mm/migrate.c
> > > +++ b/mm/migrate.c
> > > @@ -2225,17 +2225,15 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> > >   		pte_t pte;
> > >   		pte = *ptep;
> > > -		pfn = pte_pfn(pte);
> > >   		if (pte_none(pte)) {
> > >   			mpfn = MIGRATE_PFN_MIGRATE;
> > >   			migrate->cpages++;
> > > -			pfn = 0;
> > >   			goto next;
> > >   		}
> > >   		if (!pte_present(pte)) {
> > > -			mpfn = pfn = 0;
> > > +			mpfn = 0;
> > >   			/*
> > >   			 * Only care about unaddressable device page special
> > > @@ -2252,10 +2250,10 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> > >   			if (is_write_device_private_entry(entry))
> > >   				mpfn |= MIGRATE_PFN_WRITE;
> > >   		} else {
> > > +			pfn = pte_pfn(pte);
> > >   			if (is_zero_pfn(pfn)) {
> > >   				mpfn = MIGRATE_PFN_MIGRATE;
> > >   				migrate->cpages++;
> > > -				pfn = 0;
> > >   				goto next;
> > >   			}
> > >   			page = vm_normal_page(migrate->vma, addr, pte);
> > > @@ -2265,10 +2263,9 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
> > >   		/* FIXME support THP */
> > >   		if (!page || !page->mapping || PageTransCompound(page)) {
> > > -			mpfn = pfn = 0;
> > > +			mpfn = 0;
> > >   			goto next;
> > >   		}
> > > -		pfn = page_to_pfn(page);
> > 
> > You can not remove that one ! Otherwise it will break the device
> > private case.
> > 
> 
> I don't understand. The only use of "pfn" I see is in the "else"
> clause above where it is set just before using it.

Ok i managed to confuse myself with mpfn and probably with old
version of the code. Sorry for reading too quickly. Can we move
unsigned long pfn; into the else { branch so that there is no
more confusion to its scope.

Cheers,
Jérôme

Re: [PATCHv2] mm/migrate: clean up useless code in migrate_vma_collect_pmd()

[Pingfan Liu]

On Thu, Aug 15, 2019 at 03:40:21PM -0400, Jerome Glisse wrote:
> On Thu, Aug 15, 2019 at 12:23:44PM -0700, Ralph Campbell wrote:
> [...]
> > 
> > I don't understand. The only use of "pfn" I see is in the "else"
> > clause above where it is set just before using it.
> 
> Ok i managed to confuse myself with mpfn and probably with old
> version of the code. Sorry for reading too quickly. Can we move
> unsigned long pfn; into the else { branch so that there is no
> more confusion to its scope.
Looks better, I will update v3.

Thanks,
	Pingfan

Re: [PATCH 2/3] mm/migrate: see hole as invalid source page

[Pingfan Liu]

On Thu, Aug 15, 2019 at 01:22:22PM -0400, Jerome Glisse wrote:
> On Tue, Aug 06, 2019 at 04:00:10PM +0800, Pingfan Liu wrote:
> > MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate.
> > As for hole, there is no valid pfn, not to mention migration.
> > 
> > Before this patch, hole has already relied on the following code to be
> > filtered out. Hence it is more reasonable to see hole as invalid source
> > page.
> > migrate_vma_prepare()
> > {
> > 		struct page *page = migrate_pfn_to_page(migrate->src[i]);
> > 
> > 		if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE))
> > 		     \_ this condition
> > }
> 
> NAK you break the API, MIGRATE_PFN_MIGRATE is use for 2 things,
> first it allow the collection code to mark entry that can be
> migrated, then it use by driver to allow driver to skip migration
> for some entry (for whatever reason the driver might have), we
> still need to keep the entry and not clear it so that we can
> cleanup thing (ie remove migration pte entry).
Thanks for your kindly review.

I read the code again. Maybe I miss something. But as my understanding,
for hole, there is no pte.
As the current code migrate_vma_collect_pmd()
{
	if (pmd_none(*pmdp))
		return migrate_vma_collect_hole(start, end, walk);
...
	make_migration_entry()
}

We do not install migration entry for hole, then no need to remove
migration pte entry.

And on the driver side, there is way to migrate a hole. The driver just
skip it by
drivers/gpu/drm/nouveau/nouveau_dmem.c: if (!spage || !(src_pfns[i] & MIGRATE_PFN_MIGRATE))
                                             ^^^^
Finally, in migrate_vma_finalize(), for a hole,
		if (!page) {
			if (newpage) {
				unlock_page(newpage);
				put_page(newpage);
			}
			continue;
		}
And we do not rely on remove_migration_ptes(page, newpage, false); to
restore the orignal pte (and it is impossible).

Thanks,
	Pingfan

Re: [PATCH 2/3] mm/migrate: see hole as invalid source page

[Pingfan Liu]

On Fri, Aug 16, 2019 at 11:02:22PM +0800, Pingfan Liu wrote:
> On Thu, Aug 15, 2019 at 01:22:22PM -0400, Jerome Glisse wrote:
> > On Tue, Aug 06, 2019 at 04:00:10PM +0800, Pingfan Liu wrote:
> > > MIGRATE_PFN_MIGRATE marks a valid pfn, further more, suitable to migrate.
> > > As for hole, there is no valid pfn, not to mention migration.
> > > 
> > > Before this patch, hole has already relied on the following code to be
> > > filtered out. Hence it is more reasonable to see hole as invalid source
> > > page.
> > > migrate_vma_prepare()
> > > {
> > > 		struct page *page = migrate_pfn_to_page(migrate->src[i]);
> > > 
> > > 		if (!page || (migrate->src[i] & MIGRATE_PFN_MIGRATE))
> > > 		     \_ this condition
> > > }
> > 
> > NAK you break the API, MIGRATE_PFN_MIGRATE is use for 2 things,
> > first it allow the collection code to mark entry that can be
> > migrated, then it use by driver to allow driver to skip migration
> > for some entry (for whatever reason the driver might have), we
> > still need to keep the entry and not clear it so that we can
> > cleanup thing (ie remove migration pte entry).
> Thanks for your kindly review.
> 
> I read the code again. Maybe I miss something. But as my understanding,
> for hole, there is no pte.
> As the current code migrate_vma_collect_pmd()
> {
> 	if (pmd_none(*pmdp))
> 		return migrate_vma_collect_hole(start, end, walk);
> ...
> 	make_migration_entry()
> }
> 
> We do not install migration entry for hole, then no need to remove
> migration pte entry.
> 
> And on the driver side, there is way to migrate a hole. The driver just
> skip it by
> drivers/gpu/drm/nouveau/nouveau_dmem.c: if (!spage || !(src_pfns[i] & MIGRATE_PFN_MIGRATE))
>                                              ^^^^
> Finally, in migrate_vma_finalize(), for a hole,
> 		if (!page) {
> 			if (newpage) {
> 				unlock_page(newpage);
> 				put_page(newpage);
> 			}
> 			continue;
> 		}
> And we do not rely on remove_migration_ptes(page, newpage, false); to
> restore the orignal pte (and it is impossible).
> 
Hello, do you have any comment?

I think most of important, hole does not use the 'MIGRATE_PFN_MIGRATE'
API. Hole has not pte, and there is no way to 'remove migration pte
entry'. Further more, we can know the failure on the source side, no
need to defer it to driver side.

By this way, [3/3] can unify the code.

Thanks,
	Pingfan