[v4] mm/page_vma_mapped.c: Explicitly compare pfn for normal, hugetlbfs and THP page
diff mbox series

Message ID 1578737885-8890-1-git-send-email-lixinhai.lxh@gmail.com
State New
Headers show
Series
  • [v4] mm/page_vma_mapped.c: Explicitly compare pfn for normal, hugetlbfs and THP page
Related show

Commit Message

Li Xinhai Jan. 11, 2020, 10:18 a.m. UTC
When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
The current implementation apply comparison as
- normal 4K page: page_pfn <= pfn < page_pfn + 1
- hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
- THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
in pfn_in_hpage. For hugetlbfs page, it should be
page_pfn == pfn

Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
is not only for THP and explicitly compare for these cases.

Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Michal Hocko <mhocko@kernel.org>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/page_vma_mapped.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Andrew Morton Jan. 13, 2020, 10:38 p.m. UTC | #1
On Sat, 11 Jan 2020 10:18:05 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote:

> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
> The current implementation apply comparison as
> - normal 4K page: page_pfn <= pfn < page_pfn + 1
> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> in pfn_in_hpage. For hugetlbfs page, it should be
> page_pfn == pfn

Does this have any runtime effects?

> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
> is not only for THP and explicitly compare for these cases.
Michal Hocko Jan. 14, 2020, 9:25 a.m. UTC | #2
On Sat 11-01-20 10:18:05, Li Xinhai wrote:
> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
> The current implementation apply comparison as
> - normal 4K page: page_pfn <= pfn < page_pfn + 1
> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> in pfn_in_hpage. For hugetlbfs page, it should be
> page_pfn == pfn
> 
> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
> is not only for THP and explicitly compare for these cases.

Why is this important to do. I have asked and Mike had the same feeling
that the patch is missing any real justification. Why do we need this
change? It is great that you have dropped VM_BUG_ON btw.

> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  mm/page_vma_mapped.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index eff4b45..719c352 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -52,12 +52,16 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
>  	return true;
>  }
>  
> -static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
> +static inline bool pfn_is_match(struct page *page, unsigned long pfn)
>  {
> -	unsigned long hpage_pfn = page_to_pfn(hpage);
> +	unsigned long page_pfn = page_to_pfn(page);
> +
> +	/* normal page and hugetlbfs page */
> +	if (!PageTransCompound(page) || PageHuge(page))
> +		return page_pfn == pfn;
>  
>  	/* THP can be referenced by any subpage */
> -	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
> +	return pfn >= page_pfn && pfn - page_pfn < hpage_nr_pages(page);
>  }
>  
>  /**
> @@ -108,7 +112,7 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)
>  		pfn = pte_pfn(*pvmw->pte);
>  	}
>  
> -	return pfn_in_hpage(pvmw->page, pfn);
> +	return pfn_is_match(pvmw->page, pfn);
>  }
>  
>  /**
> -- 
> 1.8.3.1
Li Xinhai Jan. 14, 2020, 1:47 p.m. UTC | #3
On 2020-01-14 at 17:25 Michal Hocko wrote:
>On Sat 11-01-20 10:18:05, Li Xinhai wrote:
>> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
>> The current implementation apply comparison as
>> - normal 4K page: page_pfn <= pfn < page_pfn + 1
>> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
>> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
>> in pfn_in_hpage. For hugetlbfs page, it should be
>> page_pfn == pfn
>>
>> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
>> is not only for THP and explicitly compare for these cases.
>
>Why is this important to do. I have asked and Mike had the same feeling
>that the patch is missing any real justification. Why do we need this
>change? It is great that you have dropped VM_BUG_ON btw.
> 
I think it is important to make the code clear, as said, comparing hugetlbfs page
in range page_pfn <= pfn < page_pfn + HPAGE_PMD_NR is confusion.

>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> ---
>>  mm/page_vma_mapped.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>> index eff4b45..719c352 100644
>> --- a/mm/page_vma_mapped.c
>> +++ b/mm/page_vma_mapped.c
>> @@ -52,12 +52,16 @@ static bool map_pte(struct page_vma_mapped_walk *pvmw)
>>  return true;
>>  }
>> 
>> -static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
>> +static inline bool pfn_is_match(struct page *page, unsigned long pfn)
>>  {
>> -	unsigned long hpage_pfn = page_to_pfn(hpage);
>> +	unsigned long page_pfn = page_to_pfn(page);
>> +
>> +	/* normal page and hugetlbfs page */
>> +	if (!PageTransCompound(page) || PageHuge(page))
>> +	return page_pfn == pfn;
>> 
>>  /* THP can be referenced by any subpage */
>> -	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
>> +	return pfn >= page_pfn && pfn - page_pfn < hpage_nr_pages(page);
>>  }
>> 
>>  /**
>> @@ -108,7 +112,7 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)
>>  pfn = pte_pfn(*pvmw->pte);
>>  }
>> 
>> -	return pfn_in_hpage(pvmw->page, pfn);
>> +	return pfn_is_match(pvmw->page, pfn);
>>  }
>> 
>>  /**
>> --
>> 1.8.3.1
>
>--
>Michal Hocko
>SUSE Labs
Li Xinhai Jan. 14, 2020, 2:24 p.m. UTC | #4
On 2020-01-14 at 06:38 Andrew Morton wrote:
>On Sat, 11 Jan 2020 10:18:05 +0000 Li Xinhai <lixinhai.lxh@gmail.com> wrote:
>
>> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
>> The current implementation apply comparison as
>> - normal 4K page: page_pfn <= pfn < page_pfn + 1
>> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
>> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
>> in pfn_in_hpage. For hugetlbfs page, it should be
>> page_pfn == pfn
>
>Does this have any runtime effects? 
No impaction to current behavior, just make the code clear.

>
>> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
>> is not only for THP and explicitly compare for these cases.
Michal Hocko Jan. 15, 2020, 9:31 a.m. UTC | #5
On Tue 14-01-20 21:47:51, Li Xinhai wrote:
> On 2020-01-14 at 17:25 Michal Hocko wrote:
> >On Sat 11-01-20 10:18:05, Li Xinhai wrote:
> >> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
> >> The current implementation apply comparison as
> >> - normal 4K page: page_pfn <= pfn < page_pfn + 1
> >> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> >> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> >> in pfn_in_hpage. For hugetlbfs page, it should be
> >> page_pfn == pfn
> >>
> >> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
> >> is not only for THP and explicitly compare for these cases.
> >
> >Why is this important to do. I have asked and Mike had the same feeling
> >that the patch is missing any real justification. Why do we need this
> >change? It is great that you have dropped VM_BUG_ON btw.
> > 
> I think it is important to make the code clear, as said, comparing hugetlbfs page
> in range page_pfn <= pfn < page_pfn + HPAGE_PMD_NR is confusion.

I am sorry but I do not really see a big confusion here. It is probably
a matter of taste. If others consider this an improvement then I will
not stand in the way but I consider the justification insuficient for
merging.
Mike Kravetz Jan. 16, 2020, 12:40 a.m. UTC | #6
On 1/15/20 1:31 AM, Michal Hocko wrote:
> On Tue 14-01-20 21:47:51, Li Xinhai wrote:
>> On 2020-01-14 at 17:25 Michal Hocko wrote:
>>> On Sat 11-01-20 10:18:05, Li Xinhai wrote:
>>>> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
>>>> The current implementation apply comparison as
>>>> - normal 4K page: page_pfn <= pfn < page_pfn + 1
>>>> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
>>>> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
>>>> in pfn_in_hpage. For hugetlbfs page, it should be
>>>> page_pfn == pfn
>>>>
>>>> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
>>>> is not only for THP and explicitly compare for these cases.
>>>
>>> Why is this important to do. I have asked and Mike had the same feeling
>>> that the patch is missing any real justification. Why do we need this
>>> change? It is great that you have dropped VM_BUG_ON btw.
>>>
>> I think it is important to make the code clear, as said, comparing hugetlbfs page
>> in range page_pfn <= pfn < page_pfn + HPAGE_PMD_NR is confusion.
> 
> I am sorry but I do not really see a big confusion here. It is probably
> a matter of taste. If others consider this an improvement then I will
> not stand in the way but I consider the justification insuficient for
> merging.

Perhaps I am confused, but the patch does update the check done for
hugetlb pages.  IIUC, previously there was no distinction between THP
pages and hugetlb pages in the check.  It is valid to pass in a sub-
page of a THP page, but not that of a hugetlb page.

I do not believe it is possible for existing code to pass in a sub-page
of a hugetlb page.  And, no one has ever seen this as an issue.  This
is why I was questioning the need for the patch.

With all that said, the new code/check is better (more complete) than
the original.  It may not do anything for the current code base, but
it 'could' catch potential errors in future code.  Because of this, I
do consider the code an improvement.

Acked-by: Mike Kravetz <mike.kravetz@oracle.com>
Michal Hocko Jan. 16, 2020, 10:29 a.m. UTC | #7
On Wed 15-01-20 16:40:26, Mike Kravetz wrote:
> On 1/15/20 1:31 AM, Michal Hocko wrote:
> > On Tue 14-01-20 21:47:51, Li Xinhai wrote:
> >> On 2020-01-14 at 17:25 Michal Hocko wrote:
> >>> On Sat 11-01-20 10:18:05, Li Xinhai wrote:
> >>>> When check_pte, pfn of normal, hugetlbfs and THP page need be compared.
> >>>> The current implementation apply comparison as
> >>>> - normal 4K page: page_pfn <= pfn < page_pfn + 1
> >>>> - hugetlbfs page:  page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> >>>> - THP page: page_pfn <= pfn < page_pfn + HPAGE_PMD_NR
> >>>> in pfn_in_hpage. For hugetlbfs page, it should be
> >>>> page_pfn == pfn
> >>>>
> >>>> Now, change pfn_in_hpage to pfn_is_match to highlight that comparison
> >>>> is not only for THP and explicitly compare for these cases.
> >>>
> >>> Why is this important to do. I have asked and Mike had the same feeling
> >>> that the patch is missing any real justification. Why do we need this
> >>> change? It is great that you have dropped VM_BUG_ON btw.
> >>>
> >> I think it is important to make the code clear, as said, comparing hugetlbfs page
> >> in range page_pfn <= pfn < page_pfn + HPAGE_PMD_NR is confusion.
> > 
> > I am sorry but I do not really see a big confusion here. It is probably
> > a matter of taste. If others consider this an improvement then I will
> > not stand in the way but I consider the justification insuficient for
> > merging.
> 
> Perhaps I am confused, but the patch does update the check done for
> hugetlb pages.  IIUC, previously there was no distinction between THP
> pages and hugetlb pages in the check.  It is valid to pass in a sub-
> page of a THP page, but not that of a hugetlb page.
> 
> I do not believe it is possible for existing code to pass in a sub-page
> of a hugetlb page.  And, no one has ever seen this as an issue.  This
> is why I was questioning the need for the patch.

Exactly and that is the reason I fail the see a point. 

> With all that said, the new code/check is better (more complete) than
> the original.  It may not do anything for the current code base, but
> it 'could' catch potential errors in future code.  Because of this, I
> do consider the code an improvement.

It adds a branch for something that doesn't happen and also to a code
path which is quite internal to the MM AFAICS. That being said, if you
believe this is an improvement then I will not stand in the way. But
there are so many other places which could add checks that are not
exercised...

> Acked-by: Mike Kravetz <mike.kravetz@oracle.com>

Patch
diff mbox series

diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
index eff4b45..719c352 100644
--- a/mm/page_vma_mapped.c
+++ b/mm/page_vma_mapped.c
@@ -52,12 +52,16 @@  static bool map_pte(struct page_vma_mapped_walk *pvmw)
 	return true;
 }
 
-static inline bool pfn_in_hpage(struct page *hpage, unsigned long pfn)
+static inline bool pfn_is_match(struct page *page, unsigned long pfn)
 {
-	unsigned long hpage_pfn = page_to_pfn(hpage);
+	unsigned long page_pfn = page_to_pfn(page);
+
+	/* normal page and hugetlbfs page */
+	if (!PageTransCompound(page) || PageHuge(page))
+		return page_pfn == pfn;
 
 	/* THP can be referenced by any subpage */
-	return pfn >= hpage_pfn && pfn - hpage_pfn < hpage_nr_pages(hpage);
+	return pfn >= page_pfn && pfn - page_pfn < hpage_nr_pages(page);
 }
 
 /**
@@ -108,7 +112,7 @@  static bool check_pte(struct page_vma_mapped_walk *pvmw)
 		pfn = pte_pfn(*pvmw->pte);
 	}
 
-	return pfn_in_hpage(pvmw->page, pfn);
+	return pfn_is_match(pvmw->page, pfn);
 }
 
 /**