From: Steven Rostedt <[email protected]>
The mapping_unevictable() has a likely() around the mapping parameter.
This mapping parameter comes from page_mapping() which has an
unlikely() that the page will be set as PAGE_MAPPING_ANON, and if
so, it will return NULL. One would think that this unlikely() means
that the mapping returned by page_mapping() would not be NULL, but
where page_mapping() is used just above mapping_unevictable(), that
unlikely() is incorrect most of the time. This means that the
"likely(mapping)" in mapping_unevictable() is incorrect most of the
time.
Running the annotated branch profiler on my main box which runs firefox,
evolution, xchat and is part of my distcc farm, I had this:
correct incorrect % Function File Line
------- --------- - -------- ---- ----
12872836 1269443893 98 mapping_unevictable pagemap.h 51
35935762 1270265395 97 page_mapping mm.h 659
1306198001 143659 0 page_mapping mm.h 657
203131478 121586 0 page_mapping mm.h 657
5415491 1116 0 page_mapping mm.h 657
74899487 1116 0 page_mapping mm.h 657
203132845 224 0 page_mapping mm.h 659
5415464 27 0 page_mapping mm.h 659
13552 0 0 page_mapping mm.h 657
13552 0 0 page_mapping mm.h 659
242630 0 0 page_mapping mm.h 657
242630 0 0 page_mapping mm.h 659
74899487 0 0 page_mapping mm.h 659
The page_mapping() is a static inline, which is why it shows up multiple
times. The mapping_unevictable() is also a static inline but seems to
be used only once in my setup.
The unlikely in page_mapping() was correct a total of 1909540379 times and
incorrect 1270533123 times, with a 39% being incorrect. Perhaps this is
enough to remove the unlikely from page_mapping() as well.
Cc: Andrew Morton <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Lee Schermerhorn <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/pagemap.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 2d1ffe3..9c66e99 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -48,7 +48,7 @@ static inline void mapping_clear_unevictable(struct address_space *mapping)
static inline int mapping_unevictable(struct address_space *mapping)
{
- if (likely(mapping))
+ if (mapping)
return test_bit(AS_UNEVICTABLE, &mapping->flags);
return !!mapping;
}
--
1.7.2.3
[ Resending to Nick's real email ]
From: Steven Rostedt <[email protected]>
The mapping_unevictable() has a likely() around the mapping parameter.
This mapping parameter comes from page_mapping() which has an
unlikely() that the page will be set as PAGE_MAPPING_ANON, and if
so, it will return NULL. One would think that this unlikely() means
that the mapping returned by page_mapping() would not be NULL, but
where page_mapping() is used just above mapping_unevictable(), that
unlikely() is incorrect most of the time. This means that the
"likely(mapping)" in mapping_unevictable() is incorrect most of the
time.
Running the annotated branch profiler on my main box which runs firefox,
evolution, xchat and is part of my distcc farm, I had this:
correct incorrect % Function File Line
------- --------- - -------- ---- ----
12872836 1269443893 98 mapping_unevictable pagemap.h 51
35935762 1270265395 97 page_mapping mm.h 659
1306198001 143659 0 page_mapping mm.h 657
203131478 121586 0 page_mapping mm.h 657
5415491 1116 0 page_mapping mm.h 657
74899487 1116 0 page_mapping mm.h 657
203132845 224 0 page_mapping mm.h 659
5415464 27 0 page_mapping mm.h 659
13552 0 0 page_mapping mm.h 657
13552 0 0 page_mapping mm.h 659
242630 0 0 page_mapping mm.h 657
242630 0 0 page_mapping mm.h 659
74899487 0 0 page_mapping mm.h 659
The page_mapping() is a static inline, which is why it shows up multiple
times. The mapping_unevictable() is also a static inline but seems to
be used only once in my setup.
The unlikely in page_mapping() was correct a total of 1909540379 times and
incorrect 1270533123 times, with a 39% being incorrect. Perhaps this is
enough to remove the unlikely from page_mapping() as well.
Cc: Andrew Morton <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Lee Schermerhorn <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/pagemap.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 2d1ffe3..9c66e99 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -48,7 +48,7 @@ static inline void mapping_clear_unevictable(struct address_space *mapping)
static inline int mapping_unevictable(struct address_space *mapping)
{
- if (likely(mapping))
+ if (mapping)
return test_bit(AS_UNEVICTABLE, &mapping->flags);
return !!mapping;
}
On Mon, Dec 06, 2010 at 09:22:13PM -0500, Steven Rostedt wrote:
> [ Resending to Nick's real email ]
>
>
> From: Steven Rostedt <[email protected]>
>
> The mapping_unevictable() has a likely() around the mapping parameter.
> This mapping parameter comes from page_mapping() which has an
> unlikely() that the page will be set as PAGE_MAPPING_ANON, and if
> so, it will return NULL. One would think that this unlikely() means
> that the mapping returned by page_mapping() would not be NULL, but
> where page_mapping() is used just above mapping_unevictable(), that
> unlikely() is incorrect most of the time. This means that the
> "likely(mapping)" in mapping_unevictable() is incorrect most of the
> time.
>
> Running the annotated branch profiler on my main box which runs firefox,
> evolution, xchat and is part of my distcc farm, I had this:
>
> correct incorrect % Function File Line
> ------- --------- - -------- ---- ----
> 12872836 1269443893 98 mapping_unevictable pagemap.h 51
> 35935762 1270265395 97 page_mapping mm.h 659
> 1306198001 143659 0 page_mapping mm.h 657
> 203131478 121586 0 page_mapping mm.h 657
> 5415491 1116 0 page_mapping mm.h 657
> 74899487 1116 0 page_mapping mm.h 657
> 203132845 224 0 page_mapping mm.h 659
> 5415464 27 0 page_mapping mm.h 659
> 13552 0 0 page_mapping mm.h 657
> 13552 0 0 page_mapping mm.h 659
> 242630 0 0 page_mapping mm.h 657
> 242630 0 0 page_mapping mm.h 659
> 74899487 0 0 page_mapping mm.h 659
>
> The page_mapping() is a static inline, which is why it shows up multiple
> times. The mapping_unevictable() is also a static inline but seems to
> be used only once in my setup.
>
> The unlikely in page_mapping() was correct a total of 1909540379 times and
> incorrect 1270533123 times, with a 39% being incorrect. Perhaps this is
> enough to remove the unlikely from page_mapping() as well.
I think so. We don't have good guidelines or empirical numbers on this,
unfortunately, but I think it would be somewhere over 90%, given that it
tends to add jumps to and from out of line icache in the incorrect case.
Acked-by: Nick Piggin <[email protected]>
On Tue, 2010-12-07 at 18:02 +1100, Nick Piggin wrote:
> On Mon, Dec 06, 2010 at 09:22:13PM -0500, Steven Rostedt wrote:
> I think so. We don't have good guidelines or empirical numbers on this,
> unfortunately, but I think it would be somewhere over 90%, given that it
> tends to add jumps to and from out of line icache in the incorrect case.
OK, so I'll add the removal of the unlikely() in page_mapping in my next
verison.
>
> Acked-by: Nick Piggin <[email protected]>
Thanks!
-- Steve
On 12/06/2010 09:22 PM, Steven Rostedt wrote:
> The unlikely in page_mapping() was correct a total of 1909540379 times and
> incorrect 1270533123 times, with a 39% being incorrect. Perhaps this is
> enough to remove the unlikely from page_mapping() as well.
>
> Cc: Andrew Morton<[email protected]>
> Cc: Nick Piggin<[email protected]>
> Cc: Rik van Riel<[email protected]>
> Cc: Lee Schermerhorn<[email protected]>
> Signed-off-by: Steven Rostedt<[email protected]>
Acked-by: Rik van Riel <[email protected]>
--
All rights reversed
> [ Resending to Nick's real email ]
>
>
> From: Steven Rostedt <[email protected]>
>
> The mapping_unevictable() has a likely() around the mapping parameter.
> This mapping parameter comes from page_mapping() which has an
> unlikely() that the page will be set as PAGE_MAPPING_ANON, and if
> so, it will return NULL. One would think that this unlikely() means
> that the mapping returned by page_mapping() would not be NULL, but
> where page_mapping() is used just above mapping_unevictable(), that
> unlikely() is incorrect most of the time. This means that the
> "likely(mapping)" in mapping_unevictable() is incorrect most of the
> time.
>
> Running the annotated branch profiler on my main box which runs firefox,
> evolution, xchat and is part of my distcc farm, I had this:
>
> correct incorrect % Function File Line
> ------- --------- - -------- ---- ----
> 12872836 1269443893 98 mapping_unevictable pagemap.h 51
> 35935762 1270265395 97 page_mapping mm.h 659
> 1306198001 143659 0 page_mapping mm.h 657
> 203131478 121586 0 page_mapping mm.h 657
> 5415491 1116 0 page_mapping mm.h 657
> 74899487 1116 0 page_mapping mm.h 657
> 203132845 224 0 page_mapping mm.h 659
> 5415464 27 0 page_mapping mm.h 659
> 13552 0 0 page_mapping mm.h 657
> 13552 0 0 page_mapping mm.h 659
> 242630 0 0 page_mapping mm.h 657
> 242630 0 0 page_mapping mm.h 659
> 74899487 0 0 page_mapping mm.h 659
>
> The page_mapping() is a static inline, which is why it shows up multiple
> times. The mapping_unevictable() is also a static inline but seems to
> be used only once in my setup.
>
> The unlikely in page_mapping() was correct a total of 1909540379 times and
> incorrect 1270533123 times, with a 39% being incorrect. Perhaps this is
> enough to remove the unlikely from page_mapping() as well.
>
> Cc: Andrew Morton <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Lee Schermerhorn <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> include/linux/pagemap.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 2d1ffe3..9c66e99 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -48,7 +48,7 @@ static inline void mapping_clear_unevictable(struct address_space *mapping)
>
> static inline int mapping_unevictable(struct address_space *mapping)
> {
> - if (likely(mapping))
> + if (mapping)
> return test_bit(AS_UNEVICTABLE, &mapping->flags);
> return !!mapping;
> }
I think you are right.
Reviewed-by: KOSAKI Motohiro <[email protected]>
On Fri, 2010-12-10 at 16:00 +0900, KOSAKI Motohiro wrote:
> > [ Resending to Nick's real email ]
> >
> >
> > From: Steven Rostedt <[email protected]>
> >
> > The mapping_unevictable() has a likely() around the mapping parameter.
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
[]
> > static inline int mapping_unevictable(struct address_space *mapping)
> > {
> > - if (likely(mapping))
> > + if (mapping)
> > return test_bit(AS_UNEVICTABLE, &mapping->flags);
> > return !!mapping;
> > }
> I think you are right.
> Reviewed-by: KOSAKI Motohiro <[email protected]>
It'd be better to use
if (!mapping)
return 0;
return test_bit(AS_UNEVICTABLE, &mapping->flags);
to avoid the unnecessary !!
Joe Perches <[email protected]> writes:
> It'd be better to use
>
> if (!mapping)
> return 0;
> return test_bit(AS_UNEVICTABLE, &mapping->flags);
>
> to avoid the unnecessary !!
How about
return mapping && test_bit(AS_UNEVICTABLE, &mapping->flags);
instead...?
-miles
--
Yossarian was moved very deeply by the absolute simplicity of
this clause of Catch-22 and let out a respectful whistle.
"That's some catch, that Catch-22," he observed.
"It's the best there is," Doc Daneeka agreed.
On Fri, 2010-12-10 at 17:08 +0900, Miles Bader wrote:
> Joe Perches <[email protected]> writes:
> > It'd be better to use
> >
> > if (!mapping)
> > return 0;
> > return test_bit(AS_UNEVICTABLE, &mapping->flags);
> >
> > to avoid the unnecessary !!
>
> How about
>
> return mapping && test_bit(AS_UNEVICTABLE, &mapping->flags);
>
> instead...?
I'll just add my current patch as a "likely()" clean up. Any other
change should be separate.
Thanks,
-- Steve