[PATCH] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[PATCH] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Kirill A. Shutemov]

page_anon_vma() directly checks PAGE_MAPPING_ANON and PAGE_MAPPING_KSM
bits on page->mapping to find out if page->mapping is anon_vma;

Let's use PageAnon() and PageKsm() helpers instead. It helps readability
and makes page_anon_vma() work correctly on tail pages.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/rmap.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 9c5ff69fa0cd..21f10e53bb9e 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -108,8 +108,7 @@ static inline void put_anon_vma(struct anon_vma *anon_vma)
 
 static inline struct anon_vma *page_anon_vma(struct page *page)
 {
-	if (((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) !=
-					    PAGE_MAPPING_ANON)
+	if (!PageAnon(page) || PageKsm(page))
 		return NULL;
 	return page_rmapping(page);
 }
-- 
2.1.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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Christoph Lameter]

On Tue, 31 Mar 2015, Kirill A. Shutemov wrote:

> Let's use PageAnon() and PageKsm() helpers instead. It helps readability
> and makes page_anon_vma() work correctly on tail pages.

But it adds a branch due to the use of ||.

--
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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Kirill A. Shutemov]

On Tue, Mar 31, 2015 at 08:11:02AM -0500, Christoph Lameter wrote:
> On Tue, 31 Mar 2015, Kirill A. Shutemov wrote:
> 
> > Let's use PageAnon() and PageKsm() helpers instead. It helps readability
> > and makes page_anon_vma() work correctly on tail pages.
> 
> But it adds a branch due to the use of ||.

Which caller is hot enough to care?

-- 
 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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Andrew Morton]

On Tue, 31 Mar 2015 17:35:34 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Tue, Mar 31, 2015 at 08:11:02AM -0500, Christoph Lameter wrote:
> > On Tue, 31 Mar 2015, Kirill A. Shutemov wrote:
> > 
> > > Let's use PageAnon() and PageKsm() helpers instead. It helps readability
> > > and makes page_anon_vma() work correctly on tail pages.
> > 
> > But it adds a branch due to the use of ||.
> 
> Which caller is hot enough to care?
> 

It's a surprisingly expensive patch.

   text    data     bss     dec     hex filename

  19984    1153   15192   36329    8de9 mm/ksm.o-before
  20028    1153   15216   36397    8e2d mm/ksm.o-after

  14728     116    5168   20012    4e2c mm/rmap.o-before
  14763     116    5192   20071    4e67 mm/rmap.o-after

  25723    1417    9776   36916    9034 mm/swapfile.o-before
  25769    1417    9800   36986    907a mm/swapfile.o-after

197 bytes more text+bss, 125 bytes more text.

(Why the heck do changes like this allegedly affect bss size?)

--
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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Kirill A. Shutemov]

On Tue, Mar 31, 2015 at 01:33:38PM -0700, Andrew Morton wrote:
> On Tue, 31 Mar 2015 17:35:34 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Tue, Mar 31, 2015 at 08:11:02AM -0500, Christoph Lameter wrote:
> > > On Tue, 31 Mar 2015, Kirill A. Shutemov wrote:
> > > 
> > > > Let's use PageAnon() and PageKsm() helpers instead. It helps readability
> > > > and makes page_anon_vma() work correctly on tail pages.
> > > 
> > > But it adds a branch due to the use of ||.
> > 
> > Which caller is hot enough to care?
> > 
> 
> It's a surprisingly expensive patch.
> 
>    text    data     bss     dec     hex filename
> 
>   19984    1153   15192   36329    8de9 mm/ksm.o-before
>   20028    1153   15216   36397    8e2d mm/ksm.o-after
> 
>   14728     116    5168   20012    4e2c mm/rmap.o-before
>   14763     116    5192   20071    4e67 mm/rmap.o-after
> 
>   25723    1417    9776   36916    9034 mm/swapfile.o-before
>   25769    1417    9800   36986    907a mm/swapfile.o-after
> 
> 197 bytes more text+bss, 125 bytes more text.
> 
> (Why the heck do changes like this allegedly affect bss size?)

What about this?

Re: [PATCH] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Andrew Morton]

On Wed, 1 Apr 2015 14:50:54 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> >From adc384977898173d65c2567fc5eb421da9b272e0 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Wed, 1 Apr 2015 14:33:56 +0300
> Subject: [PATCH] mm: uninline and cleanup page-mapping related helpers
> 
> Most-used page->mapping helper -- page_mapping() -- has already
> uninlined. Let's uninline also page_rmapping() and page_anon_vma().
> It saves us depending on configuration around 400 bytes in text:
> 
>    text	   data	    bss	    dec	    hex	filename
>  660318	  99254	 410000	1169572	 11d8a4	mm/built-in.o-before
>  659854	  99254	 410000	1169108	 11d6d4	mm/built-in.o

Well, code size isn't the only thing to care about.  Some functions
really should be inlined for performance reasons even if that makes the
overall code larger.  But the changes you're proposing here look OK to
me.

> As side effect page_anon_vma() now works properly on tail pages.

Let's fix the bug in a separate patch, please.  One which can be
backported to earlier kernels if that should be needed.  ie: it should
precede any uninlining.

--
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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Kirill A. Shutemov]

On Wed, Apr 01, 2015 at 12:57:45PM -0700, Andrew Morton wrote:
> On Wed, 1 Apr 2015 14:50:54 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > >From adc384977898173d65c2567fc5eb421da9b272e0 Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Date: Wed, 1 Apr 2015 14:33:56 +0300
> > Subject: [PATCH] mm: uninline and cleanup page-mapping related helpers
> > 
> > Most-used page->mapping helper -- page_mapping() -- has already
> > uninlined. Let's uninline also page_rmapping() and page_anon_vma().
> > It saves us depending on configuration around 400 bytes in text:
> > 
> >    text	   data	    bss	    dec	    hex	filename
> >  660318	  99254	 410000	1169572	 11d8a4	mm/built-in.o-before
> >  659854	  99254	 410000	1169108	 11d6d4	mm/built-in.o
> 
> Well, code size isn't the only thing to care about.  Some functions
> really should be inlined for performance reasons even if that makes the
> overall code larger.  But the changes you're proposing here look OK to
> me.
> 
> > As side effect page_anon_vma() now works properly on tail pages.
> 
> Let's fix the bug in a separate patch, please.  One which can be
> backported to earlier kernels if that should be needed.  ie: it should
> precede any uninlining.

The bug is not triggerable in current upsteam. AFAIK, we don't call
page_anon_vma() on tail pages of THP, since we don't map THP with PTEs
yet. For rest of cases we will get NULL, which is valid answer.

Do you still want "page = compound_head(page);" line in separate patch?

-- 
 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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Andrew Morton]

On Thu, 2 Apr 2015 01:02:46 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Wed, Apr 01, 2015 at 12:57:45PM -0700, Andrew Morton wrote:
> > On Wed, 1 Apr 2015 14:50:54 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > 
> > > >From adc384977898173d65c2567fc5eb421da9b272e0 Mon Sep 17 00:00:00 2001
> > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > Date: Wed, 1 Apr 2015 14:33:56 +0300
> > > Subject: [PATCH] mm: uninline and cleanup page-mapping related helpers
> > > 
> > > Most-used page->mapping helper -- page_mapping() -- has already
> > > uninlined. Let's uninline also page_rmapping() and page_anon_vma().
> > > It saves us depending on configuration around 400 bytes in text:
> > > 
> > >    text	   data	    bss	    dec	    hex	filename
> > >  660318	  99254	 410000	1169572	 11d8a4	mm/built-in.o-before
> > >  659854	  99254	 410000	1169108	 11d6d4	mm/built-in.o
> > 
> > Well, code size isn't the only thing to care about.  Some functions
> > really should be inlined for performance reasons even if that makes the
> > overall code larger.  But the changes you're proposing here look OK to
> > me.
> > 
> > > As side effect page_anon_vma() now works properly on tail pages.
> > 
> > Let's fix the bug in a separate patch, please.  One which can be
> > backported to earlier kernels if that should be needed.  ie: it should
> > precede any uninlining.
> 
> The bug is not triggerable in current upsteam. AFAIK, we don't call
> page_anon_vma() on tail pages of THP, since we don't map THP with PTEs
> yet. For rest of cases we will get NULL, which is valid answer.

argh.  It rather helps if you can tell me when this happens (and which
patch it fixes).  I sometimes spend quite a bit of time runnnig around
in circles wondering what the heck tree/patch just got fixed.

> Do you still want "page = compound_head(page);" line in separate patch?

I think that would be best.  That way the offending patch gets fixed
and doesn't get bundled up with an unrelated change.

--
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] mm: use PageAnon() and PageKsm() helpers in page_anon_vma()

[Kirill A. Shutemov]

On Wed, Apr 01, 2015 at 03:06:53PM -0700, Andrew Morton wrote:
> On Thu, 2 Apr 2015 01:02:46 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Wed, Apr 01, 2015 at 12:57:45PM -0700, Andrew Morton wrote:
> > > On Wed, 1 Apr 2015 14:50:54 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> > > 
> > > > >From adc384977898173d65c2567fc5eb421da9b272e0 Mon Sep 17 00:00:00 2001
> > > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > > Date: Wed, 1 Apr 2015 14:33:56 +0300
> > > > Subject: [PATCH] mm: uninline and cleanup page-mapping related helpers
> > > > 
> > > > Most-used page->mapping helper -- page_mapping() -- has already
> > > > uninlined. Let's uninline also page_rmapping() and page_anon_vma().
> > > > It saves us depending on configuration around 400 bytes in text:
> > > > 
> > > >    text	   data	    bss	    dec	    hex	filename
> > > >  660318	  99254	 410000	1169572	 11d8a4	mm/built-in.o-before
> > > >  659854	  99254	 410000	1169108	 11d6d4	mm/built-in.o
> > > 
> > > Well, code size isn't the only thing to care about.  Some functions
> > > really should be inlined for performance reasons even if that makes the
> > > overall code larger.  But the changes you're proposing here look OK to
> > > me.
> > > 
> > > > As side effect page_anon_vma() now works properly on tail pages.
> > > 
> > > Let's fix the bug in a separate patch, please.  One which can be
> > > backported to earlier kernels if that should be needed.  ie: it should
> > > precede any uninlining.
> > 
> > The bug is not triggerable in current upsteam. AFAIK, we don't call
> > page_anon_vma() on tail pages of THP, since we don't map THP with PTEs
> > yet. For rest of cases we will get NULL, which is valid answer.
> 
> argh.  It rather helps if you can tell me when this happens (and which
> patch it fixes).  I sometimes spend quite a bit of time runnnig around
> in circles wondering what the heck tree/patch just got fixed.

Sorry for the mess.

> > Do you still want "page = compound_head(page);" line in separate patch?
> 
> I think that would be best.  That way the offending patch gets fixed
> and doesn't get bundled up with an unrelated change.

I've sent updated "mm: sanitize page->mapping for tail pages" patch that
care about this part. And this is uninlining patch on top of updated
patch.