[patch v2] hugetlb: correct page offset index for sharing pmd

[patch v2] hugetlb: correct page offset index for sharing pmd

[Hillf Danton]

The computation of page offset index is incorrect to be used in scanning
prio tree, as huge page offset is required, and is fixed with well
defined routine.

Changes from v1
	o s/linear_page_index/linear_hugepage_index/ for clearer code
	o hp_idx variable added for less change


Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/arch/x86/mm/hugetlbpage.c	Fri Aug  3 20:34:58 2012
+++ b/arch/x86/mm/hugetlbpage.c	Fri Aug  3 20:40:16 2012
@@ -62,6 +62,7 @@ static void huge_pmd_share(struct mm_str
 {
 	struct vm_area_struct *vma = find_vma(mm, addr);
 	struct address_space *mapping = vma->vm_file->f_mapping;
+	pgoff_t hp_idx;
 	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
 			vma->vm_pgoff;
 	struct prio_tree_iter iter;
@@ -72,8 +73,10 @@ static void huge_pmd_share(struct mm_str
 	if (!vma_shareable(vma, addr))
 		return;

+	hp_idx = linear_hugepage_index(vma, addr);
+
 	mutex_lock(&mapping->i_mmap_mutex);
-	vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, idx, idx) {
+	vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, hp_idx, hp_idx) {
 		if (svma == vma)
 			continue;

--

Re: [patch v2] hugetlb: correct page offset index for sharing pmd

[Michal Hocko]

On Sat 04-08-12 14:08:31, Hillf Danton wrote:
> The computation of page offset index is incorrect to be used in scanning
> prio tree, as huge page offset is required, and is fixed with well
> defined routine.
> 
> Changes from v1
> 	o s/linear_page_index/linear_hugepage_index/ for clearer code
> 	o hp_idx variable added for less change
> 
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/arch/x86/mm/hugetlbpage.c	Fri Aug  3 20:34:58 2012
> +++ b/arch/x86/mm/hugetlbpage.c	Fri Aug  3 20:40:16 2012
> @@ -62,6 +62,7 @@ static void huge_pmd_share(struct mm_str
>  {
>  	struct vm_area_struct *vma = find_vma(mm, addr);
>  	struct address_space *mapping = vma->vm_file->f_mapping;
> +	pgoff_t hp_idx;
>  	pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
>  			vma->vm_pgoff;

So we have two indexes now. That is just plain ugly!

>  	struct prio_tree_iter iter;
> @@ -72,8 +73,10 @@ static void huge_pmd_share(struct mm_str
>  	if (!vma_shareable(vma, addr))
>  		return;
> 
> +	hp_idx = linear_hugepage_index(vma, addr);
> +
>  	mutex_lock(&mapping->i_mmap_mutex);
> -	vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, idx, idx) {
> +	vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, hp_idx, hp_idx) {
>  		if (svma == vma)
>  			continue;
> 
> --

-- 
Michal Hocko
SUSE Labs

Re: [patch v2] hugetlb: correct page offset index for sharing pmd

[Hillf Danton]

On Mon, Aug 6, 2012 at 9:24 PM, Michal Hocko <mhocko@suse.cz> wrote:
> On Sat 04-08-12 14:08:31, Hillf Danton wrote:
>> The computation of page offset index is incorrect to be used in scanning
>> prio tree, as huge page offset is required, and is fixed with well
>> defined routine.
>>
>> Changes from v1
>>       o s/linear_page_index/linear_hugepage_index/ for clearer code
>>       o hp_idx variable added for less change
>>
>>
>> Signed-off-by: Hillf Danton <dhillf@gmail.com>
>> ---
>>
>> --- a/arch/x86/mm/hugetlbpage.c       Fri Aug  3 20:34:58 2012
>> +++ b/arch/x86/mm/hugetlbpage.c       Fri Aug  3 20:40:16 2012
>> @@ -62,6 +62,7 @@ static void huge_pmd_share(struct mm_str
>>  {
>>       struct vm_area_struct *vma = find_vma(mm, addr);
>>       struct address_space *mapping = vma->vm_file->f_mapping;
>> +     pgoff_t hp_idx;
>>       pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
>>                       vma->vm_pgoff;
>
> So we have two indexes now. That is just plain ugly!
>

Two indexes result in less code change here and no change
in page_table_shareable. Plus linear_hugepage_index tells
clearly readers that hp_idx and idx are different.

Anyway I have no strong opinion to keep
page_table_shareable unchanged, but prefer less changes.

Thanks,
              Hillf

>>       struct prio_tree_iter iter;
>> @@ -72,8 +73,10 @@ static void huge_pmd_share(struct mm_str
>>       if (!vma_shareable(vma, addr))
>>               return;
>>
>> +     hp_idx = linear_hugepage_index(vma, addr);
>> +
>>       mutex_lock(&mapping->i_mmap_mutex);
>> -     vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, idx, idx) {
>> +     vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, hp_idx, hp_idx) {
>>               if (svma == vma)
>>                       continue;
>>
>> --
>
> --
> Michal Hocko
> SUSE Labs

Re: [patch v2] hugetlb: correct page offset index for sharing pmd

[Michal Hocko]

On Mon 06-08-12 21:37:45, Hillf Danton wrote:
> On Mon, Aug 6, 2012 at 9:24 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Sat 04-08-12 14:08:31, Hillf Danton wrote:
> >> The computation of page offset index is incorrect to be used in scanning
> >> prio tree, as huge page offset is required, and is fixed with well
> >> defined routine.
> >>
> >> Changes from v1
> >>       o s/linear_page_index/linear_hugepage_index/ for clearer code
> >>       o hp_idx variable added for less change
> >>
> >>
> >> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> >> ---
> >>
> >> --- a/arch/x86/mm/hugetlbpage.c       Fri Aug  3 20:34:58 2012
> >> +++ b/arch/x86/mm/hugetlbpage.c       Fri Aug  3 20:40:16 2012
> >> @@ -62,6 +62,7 @@ static void huge_pmd_share(struct mm_str
> >>  {
> >>       struct vm_area_struct *vma = find_vma(mm, addr);
> >>       struct address_space *mapping = vma->vm_file->f_mapping;
> >> +     pgoff_t hp_idx;
> >>       pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
> >>                       vma->vm_pgoff;
> >
> > So we have two indexes now. That is just plain ugly!
> >
> 
> Two indexes result in less code change here and no change
> in page_table_shareable. Plus linear_hugepage_index tells
> clearly readers that hp_idx and idx are different.

Why do you think they are different? It's the very same thing AFAICS.
It's just that page_table_shareable fix the index silently by saddr &
PUD_MASK.
-- 
Michal Hocko
SUSE Labs

Re: [patch v2] hugetlb: correct page offset index for sharing pmd

[Hillf Danton]

On Mon, Aug 6, 2012 at 11:54 PM, Michal Hocko <mhocko@suse.cz> wrote:
> It's just that page_table_shareable fix the index silently by saddr &
> PUD_MASK.

Follow no up, and see no wrong in page_table_shareable frankly.

Hillf

Re: [patch v2] hugetlb: correct page offset index for sharing pmd

[Andrew Morton]

On Mon, 6 Aug 2012 21:37:45 +0800
Hillf Danton <dhillf@gmail.com> wrote:

> On Mon, Aug 6, 2012 at 9:24 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Sat 04-08-12 14:08:31, Hillf Danton wrote:
> >> The computation of page offset index is incorrect to be used in scanning
> >> prio tree, as huge page offset is required, and is fixed with well
> >> defined routine.
> >>
> >> Changes from v1
> >>       o s/linear_page_index/linear_hugepage_index/ for clearer code
> >>       o hp_idx variable added for less change
> >>
> >>
> >> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> >> ---
> >>
> >> --- a/arch/x86/mm/hugetlbpage.c       Fri Aug  3 20:34:58 2012
> >> +++ b/arch/x86/mm/hugetlbpage.c       Fri Aug  3 20:40:16 2012
> >> @@ -62,6 +62,7 @@ static void huge_pmd_share(struct mm_str
> >>  {
> >>       struct vm_area_struct *vma = find_vma(mm, addr);
> >>       struct address_space *mapping = vma->vm_file->f_mapping;
> >> +     pgoff_t hp_idx;
> >>       pgoff_t idx = ((addr - vma->vm_start) >> PAGE_SHIFT) +
> >>                       vma->vm_pgoff;
> >
> > So we have two indexes now. That is just plain ugly!
> >
> 
> Two indexes result in less code change here and no change
> in page_table_shareable. Plus linear_hugepage_index tells
> clearly readers that hp_idx and idx are different.
> 
> Anyway I have no strong opinion to keep
> page_table_shareable unchanged, but prefer less changes.

Don't be too concerned about the size of a change - it's the end result
which matters.  If a larger patch results in a better end result, then
do the larger patch.

Also, please add some details to the changelog: the patch is fixing a
bug but we aren't told about the end-user-visible effects of that bug. 
This is important information.

Re: [patch v2] hugetlb: correct page offset index for sharing pmd

[Hillf Danton]

On Thu, Aug 16, 2012 at 7:03 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:

> Don't be too concerned about the size of a change - it's the end result
> which matters.  If a larger patch results in a better end result, then
> do the larger patch.

Hi Andrew,

This work was triggered by the fact that huge_pmd_share mismatches
unmap_ref_private. But it does match hugetlb_vmtruncate_list.

Plus RADIX_INDEX and HEAP_INDEX are defined, and used when inserting
vma into prio tree.
===
/*
 * The following macros are used for implementing prio_tree for i_mmap
 */

#define RADIX_INDEX(vma)  ((vma)->vm_pgoff)
#define VMA_SIZE(vma)	  (((vma)->vm_end - (vma)->vm_start) >> PAGE_SHIFT)
/* avoid overflow */
#define HEAP_INDEX(vma)	  ((vma)->vm_pgoff + (VMA_SIZE(vma) - 1))
===

Thus it is incorrect to use huge pgoff in searching vma in prio tree, and
I have to withdraw this work.

Thanks,
		Hillf