[patch 5/6] mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix

[patch 5/6] mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix

[akpm]

From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix

add comment which may not be true :(

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Bob Liu <lliubbo@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
Cc: Matthew Wilcox <matthew.r.wilcox@intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/memory.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff -puN mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix mm/memory.c
--- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
+++ a/mm/memory.c
@@ -3009,6 +3009,12 @@ static int do_shared_fault(struct mm_str
 
 	if (set_page_dirty(fault_page))
 		dirtied = 1;
+	/*
+	 * Take a local copy of the address_space - page.mapping may be zeroed
+	 * by truncate after unlock_page().   The address_space itself remains
+	 * pinned by vma->vm_file's reference.  We rely on unlock_page()'s
+	 * release semantics to prevent the compiler from undoing this copying.
+	 */
 	mapping = fault_page->mapping;
 	unlock_page(fault_page);
 	if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
_

--
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 5/6] mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix

[Kirill A. Shutemov]

On Mon, Dec 15, 2014 at 03:03:43PM -0800, akpm@linux-foundation.org wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> 
> add comment which may not be true :(
> 
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Bob Liu <lliubbo@gmail.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
> Cc: Matthew Wilcox <matthew.r.wilcox@intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/memory.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff -puN mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix mm/memory.c
> --- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> +++ a/mm/memory.c
> @@ -3009,6 +3009,12 @@ static int do_shared_fault(struct mm_str
>  
>  	if (set_page_dirty(fault_page))
>  		dirtied = 1;
> +	/*
> +	 * Take a local copy of the address_space - page.mapping may be zeroed
> +	 * by truncate after unlock_page().   The address_space itself remains
> +	 * pinned by vma->vm_file's reference.  We rely on unlock_page()'s
> +	 * release semantics to prevent the compiler from undoing this copying.
> +	 */

Looks correct to me.

We need the same comment or reference to this one in do_wp_page().

>  	mapping = fault_page->mapping;

BTW, I noticed that fault_page here can be a tail page: sound subsytem
allocates its pages with GFP_COMP and maps them with ptes. The problem is
that we never set ->mapping for tail pages and the check below is always
false. It seems doesn't cause any problems right now (looks like ->mapping
is NULL also for head page sound case), but logic is somewhat broken.

I only triggered the problem when tried to reuse ->mapping in first tail
page for compound_mapcount in my thp refcounting rework.

If it sounds right, I will prepare patch to replace the line above and the
same case in do_wp_page() with

	mapping = compound_head(fault_page)->mapping;

Ok?

>  	unlock_page(fault_page);
>  	if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
> _
> 
-- 
 Kirill A. Shutemov

--
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 5/6] mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix

[Andrew Morton]

On Tue, 16 Dec 2014 01:55:32 +0200 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Mon, Dec 15, 2014 at 03:03:43PM -0800, akpm@linux-foundation.org wrote:
> > From: Andrew Morton <akpm@linux-foundation.org>
> > Subject: mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> > 
> > add comment which may not be true :(
> > 
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Bob Liu <lliubbo@gmail.com>
> > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
> > Cc: Matthew Wilcox <matthew.r.wilcox@intel.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > Cc: Rik van Riel <riel@redhat.com>
> > Cc: Sasha Levin <sasha.levin@oracle.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  mm/memory.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff -puN mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix mm/memory.c
> > --- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> > +++ a/mm/memory.c
> > @@ -3009,6 +3009,12 @@ static int do_shared_fault(struct mm_str
> >  
> >  	if (set_page_dirty(fault_page))
> >  		dirtied = 1;
> > +	/*
> > +	 * Take a local copy of the address_space - page.mapping may be zeroed
> > +	 * by truncate after unlock_page().   The address_space itself remains
> > +	 * pinned by vma->vm_file's reference.  We rely on unlock_page()'s
> > +	 * release semantics to prevent the compiler from undoing this copying.
> > +	 */
> 
> Looks correct to me.
> 
> We need the same comment or reference to this one in do_wp_page().

Can you please send a patch some time?

> >  	mapping = fault_page->mapping;
> 
> BTW, I noticed that fault_page here can be a tail page: sound subsytem
> allocates its pages with GFP_COMP and maps them with ptes.

hm, why does it use __GFP_COMP?  It could just use plain old
alloc_pages(GFP_KERNEL) then set up a pte per 4k page?

> The problem is
> that we never set ->mapping for tail pages and the check below is always
> false. It seems doesn't cause any problems right now (looks like ->mapping
> is NULL also for head page sound case), but logic is somewhat broken.
> 
> I only triggered the problem when tried to reuse ->mapping in first tail
> page for compound_mapcount in my thp refcounting rework.
> 
> If it sounds right, I will prepare patch to replace the line above and the
> same case in do_wp_page() with
> 
> 	mapping = compound_head(fault_page)->mapping;
> 
> Ok?

Generally I don't think we should encourage (or even permit) random
driver code to use somewhat-internal-to-MM features unless they really
need to.  But I note that a lot of drivers are allocating with
__GFP_COMP.  Why is this?

--
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 5/6] mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix

[Kirill A. Shutemov]

On Tue, Dec 16, 2014 at 04:08:04PM -0800, Andrew Morton wrote:
> On Tue, 16 Dec 2014 01:55:32 +0200 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Mon, Dec 15, 2014 at 03:03:43PM -0800, akpm@linux-foundation.org wrote:
> > > From: Andrew Morton <akpm@linux-foundation.org>
> > > Subject: mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> > > 
> > > add comment which may not be true :(
> > > 
> > > Cc: Andi Kleen <ak@linux.intel.com>
> > > Cc: Bob Liu <lliubbo@gmail.com>
> > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
> > > Cc: Matthew Wilcox <matthew.r.wilcox@intel.com>
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > > Cc: Rik van Riel <riel@redhat.com>
> > > Cc: Sasha Levin <sasha.levin@oracle.com>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > ---
> > > 
> > >  mm/memory.c |    6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff -puN mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix mm/memory.c
> > > --- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> > > +++ a/mm/memory.c
> > > @@ -3009,6 +3009,12 @@ static int do_shared_fault(struct mm_str
> > >  
> > >  	if (set_page_dirty(fault_page))
> > >  		dirtied = 1;
> > > +	/*
> > > +	 * Take a local copy of the address_space - page.mapping may be zeroed
> > > +	 * by truncate after unlock_page().   The address_space itself remains
> > > +	 * pinned by vma->vm_file's reference.  We rely on unlock_page()'s
> > > +	 * release semantics to prevent the compiler from undoing this copying.
> > > +	 */
> > 
> > Looks correct to me.
> > 
> > We need the same comment or reference to this one in do_wp_page().
> 
> Can you please send a patch some time?

Sure.
But I think patchset with do_wp_page() cleanup by Shachar Raindel should
go into -mm first, shouldn't it? Count my ack.

> > >  	mapping = fault_page->mapping;
> > 
> > BTW, I noticed that fault_page here can be a tail page: sound subsytem
> > allocates its pages with GFP_COMP and maps them with ptes.
> 
> hm, why does it use __GFP_COMP?  It could just use plain old
> alloc_pages(GFP_KERNEL) then set up a pte per 4k page?

IIUC, it wants compound page semantics for get_page()/put_page() -- free
whole compond page when last compound reference goes away instead of
per-order-0 handling.

https://lkml.org/lkml/2005/11/21/216

Hugh?
 
> > The problem is
> > that we never set ->mapping for tail pages and the check below is always
> > false. It seems doesn't cause any problems right now (looks like ->mapping
> > is NULL also for head page sound case), but logic is somewhat broken.
> > 
> > I only triggered the problem when tried to reuse ->mapping in first tail
> > page for compound_mapcount in my thp refcounting rework.
> > 
> > If it sounds right, I will prepare patch to replace the line above and the
> > same case in do_wp_page() with
> > 
> > 	mapping = compound_head(fault_page)->mapping;
> > 
> > Ok?
> 
> Generally I don't think we should encourage (or even permit) random
> driver code to use somewhat-internal-to-MM features unless they really
> need to.  But I note that a lot of drivers are allocating with
> __GFP_COMP.  Why is this?

I think, the reason is the same -- get_page()/put_page() semantics. But
will give a closer look later.

-- 
 Kirill A. Shutemov

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