[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) {
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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 ;)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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
---

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.
---

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] hugetlb: do not use vma_hugecache_offset for vma_prio_tree_foreach

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hillf Danton]

On Mon, Aug 13, 2012 at 9:55 PM, Michal Hocko <mhocko@suse.cz> wrote:
> 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>
> ---

Thanks

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


>  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
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Michal Hocko]

On Thu 16-08-12 20:45:15, Hillf Danton wrote:
> On Mon, Aug 13, 2012 at 9:55 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > 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>
> > ---
> 
> Thanks
> 
> Acked-by: Hillf Danton <dhillf@gmail.com>

Thanks Hillf!

> >  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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Johannes Weiner]

On Mon, Aug 13, 2012 at 03:55:41PM +0200, Michal Hocko wrote:
> 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.

I do think it's incorrect.  The resulting index may not be too big,
but it can be too small: assume hpage size of 2M and the address to
unmap to be 0x200000.  This is regular page index 512 and hpage index
1.  If you have a VMA that maps the file only starting at the second
huge page, that VMAs vm_pgoff will be 512 but you ask for offset 1 and
miss it even though it does map the page of interest.  hugetlb_cow()
will try to unmap, miss the vma, and retry the cow until the
allocation succeeds or the skipped vma(s) go away.

Unless I missed something, this should not be deferred as a cleanup.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Michal Hocko]

On Wed 26-09-12 16:56:17, Johannes Weiner wrote:
> On Mon, Aug 13, 2012 at 03:55:41PM +0200, Michal Hocko wrote:
> > 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.
> 
> I do think it's incorrect.  The resulting index may not be too big,
> but it can be too small: assume hpage size of 2M and the address to
> unmap to be 0x200000.  This is regular page index 512 and hpage index
> 1.  If you have a VMA that maps the file only starting at the second
> huge page, that VMAs vm_pgoff will be 512 but you ask for offset 1 and
> miss it even though it does map the page of interest.  hugetlb_cow()
> will try to unmap, miss the vma, and retry the cow until the
> allocation succeeds or the skipped vma(s) go away.
> 
> Unless I missed something, this should not be deferred as a cleanup.

You are right and I have totally missed this because I focused on the
other boundary too much :/ This vma_hugecache_offset is really
confusing.
Andrew has already updated the changelog so we will not get even more
confusion into the Linus tree.
Thanks for spotting this Johannes!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Johannes Weiner]

On Mon, Oct 01, 2012 at 06:22:26PM +0200, Michal Hocko wrote:
> On Wed 26-09-12 16:56:17, Johannes Weiner wrote:
> > On Mon, Aug 13, 2012 at 03:55:41PM +0200, Michal Hocko wrote:
> > > 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.
> > 
> > I do think it's incorrect.  The resulting index may not be too big,
> > but it can be too small: assume hpage size of 2M and the address to
> > unmap to be 0x200000.  This is regular page index 512 and hpage index
> > 1.  If you have a VMA that maps the file only starting at the second
> > huge page, that VMAs vm_pgoff will be 512 but you ask for offset 1 and
> > miss it even though it does map the page of interest.  hugetlb_cow()
> > will try to unmap, miss the vma, and retry the cow until the
> > allocation succeeds or the skipped vma(s) go away.
> > 
> > Unless I missed something, this should not be deferred as a cleanup.
> 
> You are right and I have totally missed this because I focused on the
> other boundary too much :/ This vma_hugecache_offset is really
> confusing.
> Andrew has already updated the changelog so we will not get even more
> confusion into the Linus tree.
> Thanks for spotting this Johannes!

Just saw the update on ozlabs, thanks guys.  Andrew, could you please
also add:

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>