* [PATCH 1/3] mm: Remove likely() from mapping_unevictable()
2010-12-14 0:43 [PATCH 0/3] mm: Remove unlikely likelys Steven Rostedt
@ 2010-12-14 0:43 ` Steven Rostedt
2010-12-14 0:43 ` [PATCH 2/3] mm: Remove unlikely() from page_mapping() Steven Rostedt
2010-12-14 0:43 ` [PATCH 3/3] mm: Remove likely() from grab_cache_page_write_begin() Steven Rostedt
2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2010-12-14 0:43 UTC (permalink / raw)
To: linux-kernel
Cc: Nick Piggin, Andrew Morton, KOSAKI Motohiro, Rik van Riel,
Lee Schermerhorn
[-- Attachment #1: 0001-mm-Remove-likely-from-mapping_unevictable.patch --]
[-- Type: text/plain, Size: 3107 bytes --]
From: Steven Rostedt <srostedt@redhat.com>
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.
Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Nick Piggin <npiggin@kernel.dk>
Acked-by: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH 2/3] mm: Remove unlikely() from page_mapping()
2010-12-14 0:43 [PATCH 0/3] mm: Remove unlikely likelys Steven Rostedt
2010-12-14 0:43 ` [PATCH 1/3] mm: Remove likely() from mapping_unevictable() Steven Rostedt
@ 2010-12-14 0:43 ` Steven Rostedt
2010-12-14 0:43 ` [PATCH 3/3] mm: Remove likely() from grab_cache_page_write_begin() Steven Rostedt
2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2010-12-14 0:43 UTC (permalink / raw)
To: linux-kernel
Cc: Nick Piggin, Andrew Morton, KOSAKI Motohiro, Rik van Riel,
Lee Schermerhorn
[-- Attachment #1: 0002-mm-Remove-unlikely-from-page_mapping.patch --]
[-- Type: text/plain, Size: 2625 bytes --]
From: Steven Rostedt <srostedt@redhat.com>
page_mapping() has a unlikely that the mapping has PAGE_MAPPING_ANON
set. But running the annotated branch profiler on a normal desktop
system doing vairous tasks (xchat, evolution, firefox, distcc),
it is not really that unlikely that the mapping here will have the
PAGE_MAPPING_ANON flag set:
correct incorrect % Function File Line
------- --------- - -------- ---- ----
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 unlikely in page_mapping() was correct a total of 1909540379 times and
incorrect 1270533123 times, with a 39% being incorrect. With this much of an
error, it's best to simply remove the unlikely and have the compiler and
branch prediction figure this out.
Cc: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Nick Piggin <npiggin@kernel.dk>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/linux/mm.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 721f451..b064264 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -657,7 +657,7 @@ static inline struct address_space *page_mapping(struct page *page)
VM_BUG_ON(PageSlab(page));
if (unlikely(PageSwapCache(page)))
mapping = &swapper_space;
- else if (unlikely((unsigned long)mapping & PAGE_MAPPING_ANON))
+ else if ((unsigned long)mapping & PAGE_MAPPING_ANON)
mapping = NULL;
return mapping;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 3/3] mm: Remove likely() from grab_cache_page_write_begin()
2010-12-14 0:43 [PATCH 0/3] mm: Remove unlikely likelys Steven Rostedt
2010-12-14 0:43 ` [PATCH 1/3] mm: Remove likely() from mapping_unevictable() Steven Rostedt
2010-12-14 0:43 ` [PATCH 2/3] mm: Remove unlikely() from page_mapping() Steven Rostedt
@ 2010-12-14 0:43 ` Steven Rostedt
2 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2010-12-14 0:43 UTC (permalink / raw)
To: linux-kernel; +Cc: Nick Piggin, Andrew Morton
[-- Attachment #1: 0003-mm-Remove-likely-from-grab_cache_page_write_begin.patch --]
[-- Type: text/plain, Size: 2413 bytes --]
From: Steven Rostedt <srostedt@redhat.com>
Running the annotated branch profiler on a box doing average work
(firefox, evolution, xchat, distcc farm), the likely() used in
grab_cache_page_write_begin() was incorrect most of the time:
correct incorrect % Function File Line
------- --------- - -------- ---- ----
1924262 71332401 97 grab_cache_page_write_begin filemap.c 2206
Adding a trace_printk() and running the function tracer limited to
just this function I can see:
gconfd-2-2696 [000] 4467.268935: grab_cache_page_write_begin: page= (null) mapping=ffff8800676a9460 index=7
gconfd-2-2696 [000] 4467.268946: grab_cache_page_write_begin <-ext3_write_begin
gconfd-2-2696 [000] 4467.268947: grab_cache_page_write_begin: page= (null) mapping=ffff8800676a9460 index=8
gconfd-2-2696 [000] 4467.268959: grab_cache_page_write_begin <-ext3_write_begin
gconfd-2-2696 [000] 4467.268960: grab_cache_page_write_begin: page= (null) mapping=ffff8800676a9460 index=9
gconfd-2-2696 [000] 4467.268972: grab_cache_page_write_begin <-ext3_write_begin
gconfd-2-2696 [000] 4467.268973: grab_cache_page_write_begin: page= (null) mapping=ffff8800676a9460 index=10
gconfd-2-2696 [000] 4467.268991: grab_cache_page_write_begin <-ext3_write_begin
gconfd-2-2696 [000] 4467.268992: grab_cache_page_write_begin: page= (null) mapping=ffff8800676a9460 index=11
gconfd-2-2696 [000] 4467.269005: grab_cache_page_write_begin <-ext3_write_begin
Which shows that a lot of calls from ext3_write_begin will result in
the page returned by "find_lock_page" will be NULL.
Acked-by: Nick Piggin <npiggin@kernel.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
mm/filemap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/mm/filemap.c b/mm/filemap.c
index ea89840..d557fe7 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2218,7 +2218,7 @@ struct page *grab_cache_page_write_begin(struct address_space *mapping,
gfp_notmask = __GFP_FS;
repeat:
page = find_lock_page(mapping, index);
- if (likely(page))
+ if (page)
return page;
page = __page_cache_alloc(mapping_gfp_mask(mapping) & ~gfp_notmask);
--
1.7.2.3
^ permalink raw reply related [flat|nested] 4+ messages in thread