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

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

[Hillf Danton]

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

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
@@ -72,12 +72,15 @@ static void huge_pmd_share(struct mm_str
 	if (!vma_shareable(vma, addr))
 		return;

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

-		saddr = page_table_shareable(svma, vma, addr, idx);
+		saddr = page_table_shareable(svma, vma, addr,
+						idx * (PMD_SIZE/PAGE_SIZE));
 		if (saddr) {
 			spte = huge_pte_offset(svma->vm_mm, saddr);
 			if (spte) {
--

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

[Michal Hocko]

On Fri 03-08-12 20:56:45, Hillf Danton wrote:
> The computation of page offset index is open coded, and incorrect, to
> be used in scanning prio tree, as huge page offset is required, and is
> fixed with the well defined routine.

I guess that nobody reported this because if someone really wants to
share he will use aligned address for mmap/shmat and so the index is 0.
Anyway it is worth fixing. Thanks for pointing out!

> 
> 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
> @@ -72,12 +72,15 @@ static void huge_pmd_share(struct mm_str
>  	if (!vma_shareable(vma, addr))
>  		return;
> 
> +	idx = linear_page_index(vma, addr);
> +

You can use linear_hugepage_index directly and remove the idx
initialization as well.

>  	mutex_lock(&mapping->i_mmap_mutex);
>  	vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, idx, idx) {
>  		if (svma == vma)
>  			continue;
> 
> -		saddr = page_table_shareable(svma, vma, addr, idx);
> +		saddr = page_table_shareable(svma, vma, addr,
> +						idx * (PMD_SIZE/PAGE_SIZE));

Why not just fixing page_table_shareable as well rather than playing
tricks like this?

>  		if (saddr) {
>  			spte = huge_pte_offset(svma->vm_mm, saddr);
>  			if (spte) {
> --

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Fri 03-08-12 15:32:35, Michal Hocko wrote:
> On Fri 03-08-12 20:56:45, Hillf Danton wrote:
> > The computation of page offset index is open coded, and incorrect, to
> > be used in scanning prio tree, as huge page offset is required, and is
> > fixed with the well defined routine.
> 
> I guess that nobody reported this because if someone really wants to
> share he will use aligned address for mmap/shmat and so the index is 0.
> Anyway it is worth fixing. Thanks for pointing out!

I have looked at the code again and I don't think there is any problem
at all. vma_prio_tree_foreach understands page units so it will find
appropriate svmas.
Or am I missing something?

> 
> > 
> > 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
> > @@ -72,12 +72,15 @@ static void huge_pmd_share(struct mm_str
> >  	if (!vma_shareable(vma, addr))
> >  		return;
> > 
> > +	idx = linear_page_index(vma, addr);
> > +
> 
> You can use linear_hugepage_index directly and remove the idx
> initialization as well.
> 
> >  	mutex_lock(&mapping->i_mmap_mutex);
> >  	vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, idx, idx) {
> >  		if (svma == vma)
> >  			continue;
> > 
> > -		saddr = page_table_shareable(svma, vma, addr, idx);
> > +		saddr = page_table_shareable(svma, vma, addr,
> > +						idx * (PMD_SIZE/PAGE_SIZE));
> 
> Why not just fixing page_table_shareable as well rather than playing
> tricks like this?
> 
> >  		if (saddr) {
> >  			spte = huge_pte_offset(svma->vm_mm, saddr);
> >  			if (spte) {
> > --
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

[Hillf Danton]

On Fri, Aug 10, 2012 at 5:48 PM, Michal Hocko <mhocko@suse.cz> wrote:
> On Fri 03-08-12 15:32:35, Michal Hocko wrote:
>> On Fri 03-08-12 20:56:45, Hillf Danton wrote:
>> > The computation of page offset index is open coded, and incorrect, to
>> > be used in scanning prio tree, as huge page offset is required, and is
>> > fixed with the well defined routine.
>>
>> I guess that nobody reported this because if someone really wants to
>> share he will use aligned address for mmap/shmat and so the index is 0.
>> Anyway it is worth fixing. Thanks for pointing out!
>
> I have looked at the code again and I don't think there is any problem
> at all. vma_prio_tree_foreach understands page units so it will find
> appropriate svmas.
> Or am I missing something?

Well, what if another case of vma_prio_tree_foreach used by hugetlb
is correct?

Hillf

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

[Michal Hocko]

On Fri 10-08-12 20:07:12, Hillf Danton wrote:
> On Fri, Aug 10, 2012 at 5:48 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Fri 03-08-12 15:32:35, Michal Hocko wrote:
> >> On Fri 03-08-12 20:56:45, Hillf Danton wrote:
> >> > The computation of page offset index is open coded, and incorrect, to
> >> > be used in scanning prio tree, as huge page offset is required, and is
> >> > fixed with the well defined routine.
> >>
> >> I guess that nobody reported this because if someone really wants to
> >> share he will use aligned address for mmap/shmat and so the index is 0.
> >> Anyway it is worth fixing. Thanks for pointing out!
> >
> > I have looked at the code again and I don't think there is any problem
> > at all. vma_prio_tree_foreach understands page units so it will find
> > appropriate svmas.
> > Or am I missing something?
> 
> Well, what if another case of vma_prio_tree_foreach used by hugetlb
> is correct?

I guess you mean unmap_ref_private and that has been changed by you
(0c176d5 mm: hugetlb: fix pgoff computation when unmapping page from
vma)...  I was wrong at that time when giving my Reviewed-by. The patch
didn't break anything because you still find all relevant vmas because
vma_hugecache_offset just provides a smaller index which is still within
boundaries.
I think that 0c176d52 should be reverted because we do not have to refer
to the head page in this case and as we can see it causes confusion.

> 
> Hillf

-- 
Michal Hocko
SUSE Labs

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

[Hillf Danton]

On Fri, Aug 10, 2012 at 8:27 PM, Michal Hocko <mhocko@suse.cz> wrote:
>
> I guess you mean unmap_ref_private and that has been changed by you
> (0c176d5 mm: hugetlb: fix pgoff computation when unmapping page from
> vma)...  I was wrong at that time when giving my Reviewed-by. The patch
> didn't break anything because you still find all relevant vmas because
> vma_hugecache_offset just provides a smaller index which is still within
> boundaries.

No, as shown by the log message of 0c176d52b,  that fix was
triggered by  (vma->vm_pgoff >> PAGE_SHIFT), thus I dont see
what you really want to revert.


> I think that 0c176d52 should be reverted because we do not have to refer
> to the head page in this case and as we can see it causes confusion.
>

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

[Michal Hocko]

On Fri 10-08-12 20:37:20, Hillf Danton wrote:
> On Fri, Aug 10, 2012 at 8:27 PM, Michal Hocko <mhocko@suse.cz> wrote:
> >
> > I guess you mean unmap_ref_private and that has been changed by you
> > (0c176d5 mm: hugetlb: fix pgoff computation when unmapping page from
> > vma)...  I was wrong at that time when giving my Reviewed-by. The patch
> > didn't break anything because you still find all relevant vmas because
> > vma_hugecache_offset just provides a smaller index which is still within
> > boundaries.
> 
> No, as shown by the log message of 0c176d52b,  that fix was
> triggered by  (vma->vm_pgoff >> PAGE_SHIFT), thus I dont see
> what you really want to revert.

fix for that would be a part of the revert of course.
 
> > I think that 0c176d52 should be reverted because we do not have to refer
> > to the head page in this case and as we can see it causes confusion.

-- 
Michal Hocko
SUSE Labs

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

[Hillf Danton]

On Fri, Aug 10, 2012 at 8:51 PM, Michal Hocko <mhocko@suse.cz> wrote:
> On Fri 10-08-12 20:37:20, Hillf Danton wrote:
>> On Fri, Aug 10, 2012 at 8:27 PM, Michal Hocko <mhocko@suse.cz> wrote:
>> >
>> > I guess you mean unmap_ref_private and that has been changed by you
>> > (0c176d5 mm: hugetlb: fix pgoff computation when unmapping page from
>> > vma)...  I was wrong at that time when giving my Reviewed-by. The patch
>> > didn't break anything because you still find all relevant vmas because
>> > vma_hugecache_offset just provides a smaller index which is still within
>> > boundaries.
>>
>> No, as shown by the log message of 0c176d52b,  that fix was
>> triggered by  (vma->vm_pgoff >> PAGE_SHIFT), thus I dont see
>> what you really want to revert.
>
> fix for that would be a part of the revert of course.
>

Fine, go ahead ;)

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

[Michal Hocko]

[CCing Kamezawa and David]

On Fri 10-08-12 20:53:36, Hillf Danton wrote:
> On Fri, Aug 10, 2012 at 8:51 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Fri 10-08-12 20:37:20, Hillf Danton wrote:
> >> On Fri, Aug 10, 2012 at 8:27 PM, Michal Hocko <mhocko@suse.cz> wrote:
> >> >
> >> > I guess you mean unmap_ref_private and that has been changed by you
> >> > (0c176d5 mm: hugetlb: fix pgoff computation when unmapping page from
> >> > vma)...  I was wrong at that time when giving my Reviewed-by. The patch
> >> > didn't break anything because you still find all relevant vmas because
> >> > vma_hugecache_offset just provides a smaller index which is still within
> >> > boundaries.
> >>
> >> No, as shown by the log message of 0c176d52b,  that fix was
> >> triggered by  (vma->vm_pgoff >> PAGE_SHIFT), thus I dont see
> >> what you really want to revert.
> >
> > fix for that would be a part of the revert of course.
> >
> 
> Fine, go ahead ;)

there you go
---
>From 973187e9bad72181f4563a675ca64235f8612a2a Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Fri, 10 Aug 2012 15:03:07 +0200
Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
 vma_prio_tree_foreach

0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
from vma) fixed pgoff calculation but it has replaced it by
vma_hugecache_offset which is not approapriate for offsets used for
vma_prio_tree_foreach because that one expects index in page units
rather than in huge_page_shift.
Using vma_hugecache_offset is not incorrect because the pgoff will fit
into the same vmas but it is confusing.

Cc: Hillf Danton <dhillf@gmail.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: David Rientjes <rientjes@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/hugetlb.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index c39e4be..a74ea31 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2462,7 +2462,8 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
 	 * from page cache lookup which is in HPAGE_SIZE units.
 	 */
 	address = address & huge_page_mask(h);
-	pgoff = vma_hugecache_offset(h, vma, address);
+	pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) +
+			vma->vm_pgoff;
 	mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
 
 	/*
-- 
1.7.10.4


-- 
Michal Hocko
SUSE Labs

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

[Hillf Danton]

On Fri, Aug 10, 2012 at 9:16 PM, Michal Hocko <mhocko@suse.cz> wrote:
> Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
>  vma_prio_tree_foreach
>
> 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
> from vma) fixed pgoff calculation but it has replaced it by
> vma_hugecache_offset which is not approapriate for offsets used for
> vma_prio_tree_foreach because that one expects index in page units
> rather than in huge_page_shift.
> Using vma_hugecache_offset is not incorrect because the pgoff will fit
> into the same vmas but it is confusing.
>

Well, how is the patch tested?

> Cc: Hillf Danton <dhillf@gmail.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: David Rientjes <rientjes@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  mm/hugetlb.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index c39e4be..a74ea31 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2462,7 +2462,8 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
>          * from page cache lookup which is in HPAGE_SIZE units.
>          */
>         address = address & huge_page_mask(h);
> -       pgoff = vma_hugecache_offset(h, vma, address);
> +       pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) +
> +                       vma->vm_pgoff;
>         mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
>
>         /*
> --
> 1.7.10.4
>
>
> --
> Michal Hocko
> SUSE Labs

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

[Hillf Danton]

On Fri, Aug 10, 2012 at 9:21 PM, Hillf Danton <dhillf@gmail.com> wrote:
> On Fri, Aug 10, 2012 at 9:16 PM, Michal Hocko <mhocko@suse.cz> wrote:
>> Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
>>  vma_prio_tree_foreach
>>
>> 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
>> from vma) fixed pgoff calculation but it has replaced it by
>> vma_hugecache_offset which is not approapriate for offsets used for
>> vma_prio_tree_foreach because that one expects index in page units
>> rather than in huge_page_shift.
>> Using vma_hugecache_offset is not incorrect because the pgoff will fit
>> into the same vmas but it is confusing.
>>
>
> Well, how is the patch tested?


You see, Michal, it is weekend and I have to be offline now.

See you next week ;)

Hillf

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

[Michal Hocko]

On Fri 10-08-12 21:21:15, Hillf Danton wrote:
> On Fri, Aug 10, 2012 at 9:16 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
> >  vma_prio_tree_foreach
> >
> > 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
> > from vma) fixed pgoff calculation but it has replaced it by
> > vma_hugecache_offset which is not approapriate for offsets used for
> > vma_prio_tree_foreach because that one expects index in page units
> > rather than in huge_page_shift.
> > Using vma_hugecache_offset is not incorrect because the pgoff will fit
> > into the same vmas but it is confusing.
> >
> 
> Well, how is the patch tested?

It's been compile tested because it only restores the previous code with
a simple and obvious bug fix.
Do you see any issue in the patch?

> > Cc: Hillf Danton <dhillf@gmail.com>
> > Cc: Mel Gorman <mel@csn.ul.ie>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: David Rientjes <rientjes@google.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > ---
> >  mm/hugetlb.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index c39e4be..a74ea31 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -2462,7 +2462,8 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
> >          * from page cache lookup which is in HPAGE_SIZE units.
> >          */
> >         address = address & huge_page_mask(h);
> > -       pgoff = vma_hugecache_offset(h, vma, address);
> > +       pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) +
> > +                       vma->vm_pgoff;
> >         mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
> >
> >         /*
> > --
> > 1.7.10.4
> >
> >
> > --
> > Michal Hocko
> > SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

[Hillf Danton]

On Fri, Aug 10, 2012 at 9:48 PM, Michal Hocko <mhocko@suse.cz> wrote:

> It's been compile tested because it only restores the previous code with
> a simple and obvious bug fix.

It helps more if you elaborate on such a simple and obvious bug and
enrich your change log accordingly?

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

[Michal Hocko]

On Sun 12-08-12 12:08:21, Hillf Danton wrote:
> On Fri, Aug 10, 2012 at 9:48 PM, Michal Hocko <mhocko@suse.cz> wrote:
> 
> > It's been compile tested because it only restores the previous code with
> > a simple and obvious bug fix.
> 
> It helps more if you elaborate on such a simple and obvious bug and
> enrich your change log accordingly?

Hmmm, to be honest I really don't care much about this change. It is just
that your previous patch (0c176d5) made the code more confusing and this
aims at fixing that.

But anyway. I will post this to Andrew unless somebody has any
objections.
---
>From d07b88a70ee1dbcc96502c48cde878931e7deb38 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Fri, 10 Aug 2012 15:03:07 +0200
Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
 vma_prio_tree_foreach

0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
from vma) fixed pgoff calculation but it has replaced it by
vma_hugecache_offset which is not approapriate for offsets used for
vma_prio_tree_foreach because that one expects index in page units
rather than in huge_page_shift.
Using vma_hugecache_offset is not incorrect because the pgoff will fit
into the same vmas but it is confusing so the standard PAGE_SHIFT based
index calculation is used instead.

Cc: Hillf Danton <dhillf@gmail.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: David Rientjes <rientjes@google.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/hugetlb.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index c39e4be..a74ea31 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2462,7 +2462,8 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
 	 * from page cache lookup which is in HPAGE_SIZE units.
 	 */
 	address = address & huge_page_mask(h);
-	pgoff = vma_hugecache_offset(h, vma, address);
+	pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) +
+			vma->vm_pgoff;
 	mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
 
 	/*
-- 
1.7.10.4


-- 
Michal Hocko
SUSE Labs

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

[Hillf Danton]

On Sun, Aug 12, 2012 at 5:31 PM, Michal Hocko <mhocko@suse.cz> wrote:
> From d07b88a70ee1dbcc96502c48cde878931e7deb38 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Fri, 10 Aug 2012 15:03:07 +0200
> Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
>  vma_prio_tree_foreach
>
> 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
> from vma) fixed pgoff calculation but it has replaced it by
> vma_hugecache_offset which is not approapriate for offsets used for
> vma_prio_tree_foreach because that one expects index in page units
> rather than in huge_page_shift.


What if another case of vma_prio_tree_foreach in try_to_unmap_file
is correct?


> Using vma_hugecache_offset is not incorrect because the pgoff will fit
> into the same vmas but it is confusing so the standard PAGE_SHIFT based
> index calculation is used instead.
>
> Cc: Hillf Danton <dhillf@gmail.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: David Rientjes <rientjes@google.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  mm/hugetlb.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index c39e4be..a74ea31 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2462,7 +2462,8 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
>          * from page cache lookup which is in HPAGE_SIZE units.
>          */
>         address = address & huge_page_mask(h);
> -       pgoff = vma_hugecache_offset(h, vma, address);
> +       pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) +
> +                       vma->vm_pgoff;
>         mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
>
>         /*
> --
> 1.7.10.4
>
>
> --
> Michal Hocko
> SUSE Labs

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

[Michal Hocko]

On Mon 13-08-12 20:10:41, Hillf Danton wrote:
> On Sun, Aug 12, 2012 at 5:31 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > From d07b88a70ee1dbcc96502c48cde878931e7deb38 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Fri, 10 Aug 2012 15:03:07 +0200
> > Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
> >  vma_prio_tree_foreach
> >
> > 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
> > from vma) fixed pgoff calculation but it has replaced it by
> > vma_hugecache_offset which is not approapriate for offsets used for
> > vma_prio_tree_foreach because that one expects index in page units
> > rather than in huge_page_shift.
> 
> 
> What if another case of vma_prio_tree_foreach in try_to_unmap_file
> is correct?

That one is surely correct (linear_page_index converts the page offset).
Anyway do you actually have any _real_ objection to the patch? I think
the generated discussion is not worth the whole discussion.
 
> > Using vma_hugecache_offset is not incorrect because the pgoff will fit
> > into the same vmas but it is confusing so the standard PAGE_SHIFT based
> > index calculation is used instead.
> >
> > Cc: Hillf Danton <dhillf@gmail.com>
> > Cc: Mel Gorman <mel@csn.ul.ie>
> > Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Cc: Andrea Arcangeli <aarcange@redhat.com>
> > Cc: David Rientjes <rientjes@google.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > ---
> >  mm/hugetlb.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index c39e4be..a74ea31 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -2462,7 +2462,8 @@ static int unmap_ref_private(struct mm_struct *mm, struct vm_area_struct *vma,
> >          * from page cache lookup which is in HPAGE_SIZE units.
> >          */
> >         address = address & huge_page_mask(h);
> > -       pgoff = vma_hugecache_offset(h, vma, address);
> > +       pgoff = ((address - vma->vm_start) >> PAGE_SHIFT) +
> > +                       vma->vm_pgoff;
> >         mapping = vma->vm_file->f_dentry->d_inode->i_mapping;
> >
> >         /*
> > --
> > 1.7.10.4
> >
> >
> > --
> > Michal Hocko
> > SUSE Labs

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] hugetlb: do not use vma_hugecache_offset for vma_prio_tree_foreach

[Hillf Danton]

On Mon, Aug 13, 2012 at 9:09 PM, Michal Hocko <mhocko@suse.cz> wrote:
> On Mon 13-08-12 20:10:41, Hillf Danton wrote:
>> On Sun, Aug 12, 2012 at 5:31 PM, Michal Hocko <mhocko@suse.cz> wrote:
>> > From d07b88a70ee1dbcc96502c48cde878931e7deb38 Mon Sep 17 00:00:00 2001
>> > From: Michal Hocko <mhocko@suse.cz>
>> > Date: Fri, 10 Aug 2012 15:03:07 +0200
>> > Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
>> >  vma_prio_tree_foreach
>> >
>> > 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
>> > from vma) fixed pgoff calculation but it has replaced it by
>> > vma_hugecache_offset which is not approapriate for offsets used for
>> > vma_prio_tree_foreach because that one expects index in page units
>> > rather than in huge_page_shift.
>>
>>
>> What if another case of vma_prio_tree_foreach in try_to_unmap_file
>> is correct?
>
> That one is surely correct (linear_page_index converts the page offset).

But linear_page_index is not used in this patch, why?

> Anyway do you actually have any _real_ objection to the patch?

I will sign ack only after I see your answers to my questions.
Feel free to info me if you are unlikely to answer questions, Michal.

Re: [PATCH] hugetlb: do not use vma_hugecache_offset for vma_prio_tree_foreach

[Michal Hocko]

On Mon 13-08-12 21:24:36, Hillf Danton wrote:
> On Mon, Aug 13, 2012 at 9:09 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Mon 13-08-12 20:10:41, Hillf Danton wrote:
> >> On Sun, Aug 12, 2012 at 5:31 PM, Michal Hocko <mhocko@suse.cz> wrote:
> >> > From d07b88a70ee1dbcc96502c48cde878931e7deb38 Mon Sep 17 00:00:00 2001
> >> > From: Michal Hocko <mhocko@suse.cz>
> >> > Date: Fri, 10 Aug 2012 15:03:07 +0200
> >> > Subject: [PATCH] hugetlb: do not use vma_hugecache_offset for
> >> >  vma_prio_tree_foreach
> >> >
> >> > 0c176d5 (mm: hugetlb: fix pgoff computation when unmapping page
> >> > from vma) fixed pgoff calculation but it has replaced it by
> >> > vma_hugecache_offset which is not approapriate for offsets used for
> >> > vma_prio_tree_foreach because that one expects index in page units
> >> > rather than in huge_page_shift.
> >>
> >>
> >> What if another case of vma_prio_tree_foreach in try_to_unmap_file
> >> is correct?
> >
> > That one is surely correct (linear_page_index converts the page offset).
> 
> But linear_page_index is not used in this patch, why?

I will leave it as an excersise for the careful reader...
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] hugetlb: do not use vma_hugecache_offset for vma_prio_tree_foreach

[Hillf Danton]

On Mon, Aug 13, 2012 at 9:49 PM, Michal Hocko <mhocko@suse.cz> wrote:
>
> I will leave it as an excersise for the careful reader...

Is it too late for you to prepare a redelivery?