[RFC][PATCH 00/10] incorrect unlikely() and likely() cleanups

[RFC][PATCH 00/10] incorrect unlikely() and likely() cleanups

[Steven Rostedt]

This is my end of year branch annotation cleanups. I compiled my
kernel with:

CONFIG_PROFILE_ANNOTATED_BRANCHES

And booted this on my main work station, as well as a couple of
test boxes. Letting it run for a while I then looked at the results.

 cat /debug/tracing/trace_stat/branch_annotated

Which gives a result looking something like this:

 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
       0 366631256 100 sched_info_switch              sched_stats.h        269
       0 211488512 100 sched_info_queued              sched_stats.h        222
       0 197083456 100 sched_info_dequeued            sched_stats.h        177
       0 130545377 100 syscall_trace_enter            ptrace.c             1375
       0 130545340 100 syscall_trace_leave            ptrace.c             1403
       0   326960 100 pre_schedule_rt                sched_rt.c           1476
       0     5214 100 signal_pending                 sched.h              2313
       0     1305 100 trylock_page                   pagemap.h            316

It sorts the list with those that have the highest percentage of
incorrect uses and the largest number of them.

The sched_info_switch() is part of the sched_stats which falls into:

	if (unlikely(1))

And gcc is smart enough to ignore it, but because it uses a static
inline function that returns 1, the branch profiler misses that it
is indeed a constant.

I also ignored those that only had a few hits, like the signal_pending
above.

Anyway, there are 10 areas that I found that really do not need to have
an annotated branch, or the annotation is simply the opposite of
what it should be.

If people are fine with these, I'm hoping to get acks and then push this
out as a real clean up for 2.6.38.


The following patches are in:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git

    branch: unlikely/core


Steven Rostedt (10):
      sched: Change rt_task(prev) in pre_schedule_rt to likely
      mm: Remove likely() from mapping_unevictable()
      workqueue: It is likely that WORKER_NOT_RUNNING is true
      sched: Change pick_next_task_rt from unlikely to likely
      mm: Remove likely() from grab_cache_page_write_begin()
      sched: Remove unlikely() from rt_policy() in sched.c
      x86: Remove unlikey()'s from sched_switch segment tests
      fs: Remove unlikely() from fput_light()
      fs: Remove unlikely() from fget_light()
      sched: Remove unlikely() from ttwu_post_activation

----
 arch/x86/kernel/process_64.c |    6 +++---
 fs/file_table.c              |    2 +-
 include/linux/file.h         |    2 +-
 include/linux/pagemap.h      |    2 +-
 kernel/sched.c               |    4 ++--
 kernel/sched_rt.c            |    4 ++--
 kernel/workqueue.c           |    4 ++--
 mm/filemap.c                 |    2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

[RFC][PATCH 01/10] sched: Change rt_task(prev) in pre_schedule_rt to likely

[Steven Rostedt]

[-- Attachment #1: 0001-sched-Change-rt_task-prev-in-pre_schedule_rt-to-like.patch --]
[-- Type: text/plain, Size: 1048 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

As found with the branch annotation profiler, the unlikely(rt_task(prev))
in pre_schedule_rt() is always wrong. In fact it should be likely due to
the fact that we got to this function because we used prev's scheduling
class (which had to be rt).

Change the unlikely(rt_task(prev)) to likely(rt_task(prev))

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Gregory Haskins <ghaskins@novell.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched_rt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index bea7d79..7a5c4db 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1474,7 +1474,7 @@ skip:
 static void pre_schedule_rt(struct rq *rq, struct task_struct *prev)
 {
 	/* Try to pull RT tasks here if we lower this rq's prio */
-	if (unlikely(rt_task(prev)) && rq->rt.highest_prio.curr > prev->prio)
+	if (likely(rt_task(prev)) && rq->rt.highest_prio.curr > prev->prio)
 		pull_rt_task(rq);
 }
 
-- 
1.7.2.3

[RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Steven Rostedt]

[-- Attachment #1: 0002-mm-Remove-likely-from-mapping_unevictable.patch --]
[-- Type: text/plain, Size: 3039 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.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: Rik van Riel <riel@redhat.com>
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

[RFC][PATCH 03/10] workqueue: It is likely that WORKER_NOT_RUNNING is true

[Steven Rostedt]

[-- Attachment #1: 0003-workqueue-It-is-likely-that-WORKER_NOT_RUNNING-is-tr.patch --]
[-- Type: text/plain, Size: 2290 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

Running the annotate branch profiler on three boxes, including my
main box that runs firefox, evolution, xchat, and is part of the distcc farm,
showed this with the likelys in the workqueue code:

 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
      96   996253  99 wq_worker_sleeping             workqueue.c          703
      96   996247  99 wq_worker_waking_up            workqueue.c          677

The likely()s in this case were assuming that WORKER_NOT_RUNNING will
most likely be false. But this is not the case. The reason is
(and shown by adding trace_printks and testing it) that most of the time
WORKER_PREP is set.

In worker_thread() we have:

	worker_clr_flags(worker, WORKER_PREP);

	[ do work stuff ]

	worker_set_flags(worker, WORKER_PREP, false);

(that 'false' means not to wake up an idle worker)

The wq_worker_sleeping() is called from schedule when a worker thread
is putting itself to sleep. Which happens most of the time outside
of that [ do work stuff ].

The wq_worker_waking_up is called by the wakeup worker code, which
is also callod outside that [ do work stuff ].

Thus, the likely and unlikely used by those two functions are actually
backwards.

Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/workqueue.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 90db1bd..b3afcce 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -661,7 +661,7 @@ void wq_worker_waking_up(struct task_struct *task, unsigned int cpu)
 {
 	struct worker *worker = kthread_data(task);
 
-	if (likely(!(worker->flags & WORKER_NOT_RUNNING)))
+	if (unlikely(!(worker->flags & WORKER_NOT_RUNNING)))
 		atomic_inc(get_gcwq_nr_running(cpu));
 }
 
@@ -687,7 +687,7 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task,
 	struct global_cwq *gcwq = get_gcwq(cpu);
 	atomic_t *nr_running = get_gcwq_nr_running(cpu);
 
-	if (unlikely(worker->flags & WORKER_NOT_RUNNING))
+	if (likely(worker->flags & WORKER_NOT_RUNNING))
 		return NULL;
 
 	/* this can only happen on the local cpu */
-- 
1.7.2.3

[RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely

[Steven Rostedt]

[-- Attachment #1: 0004-sched-Change-pick_next_task_rt-from-unlikely-to-like.patch --]
[-- Type: text/plain, Size: 1391 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The if (unlikely(!rt_rq->rt_nr_running)) test in pick_next_task_rt()
tests if there is another rt task ready to run. If so, then pick it.

In most systems, only one RT task runs at a time most of the time.
Running the branch unlikely annotator profiler on a system doing average
work "running firefox, evolution, xchat, distcc builds, etc", it showed the
following:

 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
  324344 135104992  99 _pick_next_task_rt             sched_rt.c           1064

99% of the time the condition is true. When an RT task schedules out,
it is unlikely that another RT task is waiting to run on that same run queue.

Cc:Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Gregory Haskins <ghaskins@novell.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched_rt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
index 7a5c4db..a249ae3 100644
--- a/kernel/sched_rt.c
+++ b/kernel/sched_rt.c
@@ -1062,7 +1062,7 @@ static struct task_struct *_pick_next_task_rt(struct rq *rq)
 
 	rt_rq = &rq->rt;
 
-	if (unlikely(!rt_rq->rt_nr_running))
+	if (likely(!rt_rq->rt_nr_running))
 		return NULL;
 
 	if (rt_rq_throttled(rt_rq))
-- 
1.7.2.3

[RFC][PATCH 05/10] mm: Remove likely() from grab_cache_page_write_begin()

[Steven Rostedt]

[-- Attachment #1: 0005-mm-Remove-likely-from-grab_cache_page_write_begin.patch --]
[-- Type: text/plain, Size: 2405 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.

Cc: Nick Piggin <npiggin@suse.de>
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

[RFC][PATCH 06/10] sched: Remove unlikely() from rt_policy() in sched.c

[Steven Rostedt]

[-- Attachment #1: 0006-sched-Remove-unlikely-from-rt_policy-in-sched.c.patch --]
[-- Type: text/plain, Size: 1331 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The rt_policy() has an unlikely() that the policy it is checking is
of RT priority (SCHED_FIFO or SCHED_RR).

According to the annotate branch profiler it is incorrect most of the time:

 correct incorrect  %        Function                  File              Line
 ------- ---------  -        --------                  ----              ----
   36667   654674  94 rt_policy                      sched.c              126

This makes sense because the rt_policy() is used by the sched_set_scheduler()
and nice(). Although users may use sys_nice a bit, all RT users use
the sched_set_scheduler() to set their RT priority, including kernel
threads.

The above numbers were from a normal desktop computer running
firefox, evolution, xchat and was part of a distcc compile farm.

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index dc91a4d..269a045 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -123,7 +123,7 @@
 
 static inline int rt_policy(int policy)
 {
-	if (unlikely(policy == SCHED_FIFO || policy == SCHED_RR))
+	if (policy == SCHED_FIFO || policy == SCHED_RR)
 		return 1;
 	return 0;
 }
-- 
1.7.2.3

[RFC][PATCH 07/10] x86: Remove unlikey()s from sched_switch segment tests

[Steven Rostedt]

[-- Attachment #1: 0007-x86-Remove-unlikey-s-from-sched_switch-segment-tests.patch --]
[-- Type: text/plain, Size: 2096 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

On a 64bit distro, the chances of having a process using segment registers
is very unlikely. But if the userspace is 32bit running on top of
a 64bit kernel (very common), then this will be very likely that
processes have segment registers in use.

Running on my main desktop (which is a 32bit userspace on top of
a 64bit kernel) the annotated branch profiler showed the following:

 correct incorrect  %        Function             File              Line
 ------- ---------  -        --------             ----              ----
25522442 304125815  92 __switch_to               process_64.c         408
25522430 304123341  92 __switch_to               process_64.c         412
25743877 303891250  92 __switch_to               process_64.c         464

Instead of punishing 32bit userspace systems with an unlikely, just remove
the unlikely and let gcc optimize for what it thinks is good and let
the branch prediction (hopefully) work.

Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 arch/x86/kernel/process_64.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index b3d7a3a..22de90c 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -405,11 +405,11 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	 * This won't pick up thread selector changes, but I guess that is ok.
 	 */
 	savesegment(es, prev->es);
-	if (unlikely(next->es | prev->es))
+	if (next->es | prev->es)
 		loadsegment(es, next->es);
 
 	savesegment(ds, prev->ds);
-	if (unlikely(next->ds | prev->ds))
+	if (next->ds | prev->ds)
 		loadsegment(ds, next->ds);
 
 
@@ -461,7 +461,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 		wrmsrl(MSR_FS_BASE, next->fs);
 	prev->fsindex = fsindex;
 
-	if (unlikely(gsindex | next->gsindex | prev->gs)) {
+	if (gsindex | next->gsindex | prev->gs) {
 		load_gs_index(next->gsindex);
 		if (gsindex)
 			prev->gs = 0;
-- 
1.7.2.3

[RFC][PATCH 08/10] fs: Remove unlikely() from fput_light()

[Steven Rostedt]

[-- Attachment #1: 0008-fs-Remove-unlikely-from-fput_light.patch --]
[-- Type: text/plain, Size: 1733 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

In fput_light(), there's an unlikely(fput_needed), which running on
my normal desktop doing firefox, xchat, evolution and part of my distcc farm,
and running the annotate branch profiler shows that the unlikely is not
very unlikely.

 correct incorrect  %        Function             File              Line
 ------- ---------  -        --------             ----              ----
       0       48 100 fput_light                file.h               26
115828710 897415279  88 fput_light              file.h               26
865271179 5286128445  85 fput_light             file.h               26
19568539  8923664  31 fput_light                file.h               26
12353677  3562279  22 fput_light                file.h               26
  267691    67062  20 fput_light                file.h               26
15014853   348172   2 fput_light                file.h               26
  209258      205   0 fput_light                file.h               26
 1364164        0   0 fput_light                file.h               26

Which gives 1032903812 times it was correct and 6203351846 times it was
incorrect, or 85% incorrect.

Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/file.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/file.h b/include/linux/file.h
index b1e1297..e85baeb 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -23,7 +23,7 @@ extern struct file *alloc_file(struct path *, fmode_t mode,
 
 static inline void fput_light(struct file *file, int fput_needed)
 {
-	if (unlikely(fput_needed))
+	if (fput_needed)
 		fput(file);
 }
 
-- 
1.7.2.3

[RFC][PATCH 09/10] fs: Remove unlikely() from fget_light()

[Steven Rostedt]

[-- Attachment #1: 0009-fs-Remove-unlikely-from-fget_light.patch --]
[-- Type: text/plain, Size: 1210 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

There's an unlikely() in fget_light() that assumes the file ref count
will be 1. Running the annotate branch profiler on a desktop that is
performing daily tasks (running firefox, evolution, xchat and is also part
of a distcc farm), it shows that the ref count is not 1 that often.

 correct incorrect      %    Function                  File              Line
 ------- ---------      -    --------                  ----              ----
1035099358 6209599193  85    fget_light              file_table.c         315

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 fs/file_table.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index c3dee38..c3e89ad 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -311,7 +311,7 @@ struct file *fget_light(unsigned int fd, int *fput_needed)
 	struct files_struct *files = current->files;
 
 	*fput_needed = 0;
-	if (likely((atomic_read(&files->count) == 1))) {
+	if (atomic_read(&files->count) == 1) {
 		file = fcheck_files(files, fd);
 	} else {
 		rcu_read_lock();
-- 
1.7.2.3

[RFC][PATCH 10/10] sched: Remove unlikely() from ttwu_post_activation

[Steven Rostedt]

[-- Attachment #1: 0010-sched-Remove-unlikely-from-ttwu_post_activation.patch --]
[-- Type: text/plain, Size: 1396 bytes --]

From: Steven Rostedt <srostedt@redhat.com>

The unlikely() used in ttwu_post_activation() tests if the rq->idle_stamp
is set. But since this is for a wakeup, and wakeups happen when tasks
block on IO, and blocking tasks on IO may put the system into idle,
this can actually be a common occurence.

Running the annotated branch profiler on an average desktop running
firefox, evolution, xchat and distcc, the report shows:

 correct incorrect  %        Function             File              Line
 ------- ---------  -        --------             ----              ----
34884862 146110926  80 ttwu_post_activation      sched.c            2309

80% of the time, this unlikely is incorrect. Best not to assume what the
result is, and just remove the branch annotation.

Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/sched.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 269a045..6d24b2e 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2458,7 +2458,7 @@ static inline void ttwu_post_activation(struct task_struct *p, struct rq *rq,
 	if (p->sched_class->task_woken)
 		p->sched_class->task_woken(rq, p);
 
-	if (unlikely(rq->idle_stamp)) {
+	if (rq->idle_stamp) {
 		u64 delta = rq->clock - rq->idle_stamp;
 		u64 max = 2*sysctl_sched_migration_cost;
 
-- 
1.7.2.3

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Steven Rostedt]

[ Resending to Nick's real email ]


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.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: Rik van Riel <riel@redhat.com>
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;
 }

Re: [RFC][PATCH 05/10] mm: Remove likely() from grab_cache_page_write_begin()

[Steven Rostedt]

[ Resending to Nick's real email address ]

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.

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

Re: [RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely

[Gregory Haskins]

>>> On 12/6/2010 at 08:58 PM, in message <20101207021329.185936860@goodmis.org>,
Steven Rostedt <rostedt@goodmis.org> wrote: 
> From: Steven Rostedt <srostedt@redhat.com>
> 
> The if (unlikely(!rt_rq->rt_nr_running)) test in pick_next_task_rt()
> tests if there is another rt task ready to run. If so, then pick it.
> 
> In most systems, only one RT task runs at a time most of the time.
> Running the branch unlikely annotator profiler on a system doing average
> work "running firefox, evolution, xchat, distcc builds, etc", it showed the
> following:

My feeling is that generally speaking, if the branch is workload dependent, we should probably not annotate it at all and let the CPUs branch-predictor do its thing.  I guess what I am not 100% clear on is how these annotations affect the BPU.  I.e. is it a static decision point or can the BPU still "learn" if the annotation is particularly wrong for a given workload?  For the former, I think we should just remove this particular annotation (and there are others that need review).  For the latter, this is obviously the right annotation we should be using in this particular case.

-Greg

> 
>  correct incorrect  %        Function                  File              
> Line
>  ------- ---------  -        --------                  ----              ----
>   324344 135104992  99 _pick_next_task_rt             sched_rt.c           
> 1064
> 
> 99% of the time the condition is true. When an RT task schedules out,
> it is unlikely that another RT task is waiting to run on that same run 
> queue.
> 
> Cc:Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Gregory Haskins <ghaskins@novell.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/sched_rt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index 7a5c4db..a249ae3 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1062,7 +1062,7 @@ static struct task_struct *_pick_next_task_rt(struct rq 
> *rq)
>  
>  	rt_rq = &rq->rt;
>  
> -	if (unlikely(!rt_rq->rt_nr_running))
> +	if (likely(!rt_rq->rt_nr_running))
>  		return NULL;
>  
>  	if (rt_rq_throttled(rt_rq))

Re: [RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely

[Steven Rostedt]

On Mon, 2010-12-06 at 19:46 -0700, Gregory Haskins wrote:
> >>> On 12/6/2010 at 08:58 PM, in message <20101207021329.185936860@goodmis.org>,
> Steven Rostedt <rostedt@goodmis.org> wrote: 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > The if (unlikely(!rt_rq->rt_nr_running)) test in pick_next_task_rt()
> > tests if there is another rt task ready to run. If so, then pick it.
> > 
> > In most systems, only one RT task runs at a time most of the time.
> > Running the branch unlikely annotator profiler on a system doing average
> > work "running firefox, evolution, xchat, distcc builds, etc", it showed the
> > following:
> 
> My feeling is that generally speaking, if the branch is workload
> dependent, we should probably not annotate it at all and let the CPUs
> branch-predictor do its thing.  I guess what I am not 100% clear on is
> how these annotations affect the BPU.  I.e. is it a static decision
> point or can the BPU still "learn" if the annotation is particularly
> wrong for a given workload?  For the former, I think we should just
> remove this particular annotation (and there are others that need
> review).  For the latter, this is obviously the right annotation we
> should be using in this particular case.

I'm fine with just removing the likely() here. It is a bit workload
dependent. We can't assume that we are running a lot of RT tasks on a
single CPU, but we also should not assume that we are not doing so.
Considering that we are just coming out of an RT task, I doubt it will
hurt anything to just keep the annotation off.

I don't think gcc does anything special for x86 with regards to the BPU.
But I do believe that gcc will add special ops for likely()'s on other
archs.

-- Steve

Re: [RFC][PATCH 01/10] sched: Change rt_task(prev) in pre_schedule_rt to likely

[Yong Zhang]

On Tue, Dec 7, 2010 at 9:58 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> From: Steven Rostedt <srostedt@redhat.com>
>
> As found with the branch annotation profiler, the unlikely(rt_task(prev))
> in pre_schedule_rt() is always wrong. In fact it should be likely due to
> the fact that we got to this function because we used prev's scheduling
> class (which had to be rt).
>
> Change the unlikely(rt_task(prev)) to likely(rt_task(prev))

I have sent a more radical patch before but it get ignored.
http://marc.info/?l=linux-kernel&m=126572702600950&w=2

>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Gregory Haskins <ghaskins@novell.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/sched_rt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sched_rt.c b/kernel/sched_rt.c
> index bea7d79..7a5c4db 100644
> --- a/kernel/sched_rt.c
> +++ b/kernel/sched_rt.c
> @@ -1474,7 +1474,7 @@ skip:
>  static void pre_schedule_rt(struct rq *rq, struct task_struct *prev)
>  {
>        /* Try to pull RT tasks here if we lower this rq's prio */
> -       if (unlikely(rt_task(prev)) && rq->rt.highest_prio.curr > prev->prio)
> +       if (likely(rt_task(prev)) && rq->rt.highest_prio.curr > prev->prio)

IMHO, we can delete the checking for rt_task(prev).
Or am I missing something?

Thanks,
Yong

Re: [RFC][PATCH 01/10] sched: Change rt_task(prev) in pre_schedule_rt to likely

[Steven Rostedt]

On Tue, 2010-12-07 at 11:25 +0800, Yong Zhang wrote:
> On Tue, Dec 7, 2010 at 9:58 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > From: Steven Rostedt <srostedt@redhat.com>
> >
> > As found with the branch annotation profiler, the unlikely(rt_task(prev))
> > in pre_schedule_rt() is always wrong. In fact it should be likely due to
> > the fact that we got to this function because we used prev's scheduling
> > class (which had to be rt).
> >
> > Change the unlikely(rt_task(prev)) to likely(rt_task(prev))
> 
> I have sent a more radical patch before but it get ignored.
> http://marc.info/?l=linux-kernel&m=126572702600950&w=2

I like yours better. I'll add that one to my queue instead.

Thanks,

-- Steve

Re: [RFC][PATCH 05/10] mm: Remove likely() from grab_cache_page_write_begin()

[Nick Piggin]

On Mon, Dec 06, 2010 at 09:24:10PM -0500, Steven Rostedt wrote:
> [ Resending to Nick's real email address ]
> 
> 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.
> 
> Cc: Nick Piggin <npiggin@kernel.dk>

Looks good, 

Acked-by: Nick Piggin <npiggin@kernel.dk>

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Nick Piggin]

On Mon, Dec 06, 2010 at 09:22:13PM -0500, Steven Rostedt wrote:
> [ Resending to Nick's real email ]
> 
> 
> 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.

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 <npiggin@kernel.dk>

Re: [RFC][PATCH 03/10] workqueue: It is likely that WORKER_NOT_RUNNING is true

[Tejun Heo]

Hello, Steven.

On 12/07/2010 02:58 AM, Steven Rostedt wrote:
> In worker_thread() we have:
> 
> 	worker_clr_flags(worker, WORKER_PREP);
> 
> 	[ do work stuff ]
> 
> 	worker_set_flags(worker, WORKER_PREP, false);
> 
> (that 'false' means not to wake up an idle worker)
> 
> The wq_worker_sleeping() is called from schedule when a worker thread
> is putting itself to sleep. Which happens most of the time outside
> of that [ do work stuff ].

Yeah, I was lost thinking about the busiest case where workers are
busy processing works consecutively.  Usually workers are of course
switching in and out of idle state all the time.

How about just dropping likely/unlikely?

Thanks.

-- 
tejun

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Steven Rostedt]

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 <npiggin@kernel.dk>

Thanks!

-- Steve

Re: [RFC][PATCH 03/10] workqueue: It is likely that WORKER_NOT_RUNNING is true

[Steven Rostedt]

On Tue, 2010-12-07 at 10:49 +0100, Tejun Heo wrote:

> > The wq_worker_sleeping() is called from schedule when a worker thread
> > is putting itself to sleep. Which happens most of the time outside
> > of that [ do work stuff ].
> 
> Yeah, I was lost thinking about the busiest case where workers are
> busy processing works consecutively.  Usually workers are of course
> switching in and out of idle state all the time.
> 
> How about just dropping likely/unlikely?

I have no problem in dropping them too. I'll do that in my next version.

Thanks,

-- Steve

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Rik van Riel]

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<akpm@linux-foundation.org>
> Cc: Nick Piggin<nickpiggin@yahoo.com.au>
> Cc: Rik van Riel<riel@redhat.com>
> Cc: Lee Schermerhorn<Lee.Schermerhorn@hp.com>
> Signed-off-by: Steven Rostedt<rostedt@goodmis.org>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[KOSAKI Motohiro]

> [ Resending to Nick's real email ]
> 
> 
> 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.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Nick Piggin <nickpiggin@yahoo.com.au>
> Cc: Rik van Riel <riel@redhat.com>
> 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;
>  }

I think you are right.
	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Joe Perches]

On Fri, 2010-12-10 at 16:00 +0900, KOSAKI Motohiro wrote:
> > [ Resending to Nick's real email ]
> > 
> > 
> > From: Steven Rostedt <srostedt@redhat.com>
> > 
> > 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 <kosaki.motohiro@jp.fujitsu.com>

It'd be better to use

	if (!mapping)
		return 0;
	return test_bit(AS_UNEVICTABLE, &mapping->flags);

to avoid the unnecessary !!

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Miles Bader]

Joe Perches <joe@perches.com> 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.

Re: [RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely

[Steven Rostedt]

On Mon, 2010-12-06 at 19:46 -0700, Gregory Haskins wrote:

> My feeling is that generally speaking, if the branch is workload dependent, we should probably not annotate it at all and let the CPUs branch-predictor do its thing.  I guess what I am not 100% clear on is how these annotations affect the BPU.  I.e. is it a static decision point or can the BPU still "learn" if the annotation is particularly wrong for a given workload?  For the former, I think we should just remove this particular annotation (and there are others that need review).  For the latter, this is obviously the right annotation we should be using in this particular case.
> 

You need to get a better email client ;-)

OK, if I just remove the likely() do you Ack it?

-- Steve

Re: [RFC][PATCH 03/10] workqueue: It is likely that WORKER_NOT_RUNNING is true

[Steven Rostedt]

On Tue, 2010-12-07 at 10:49 +0100, Tejun Heo wrote:

> Yeah, I was lost thinking about the busiest case where workers are
> busy processing works consecutively.  Usually workers are of course
> switching in and out of idle state all the time.
> 
> How about just dropping likely/unlikely?

OK, if I just remove it, can I add your Acked-by?

Thanks,

-- Steve

Re: [RFC][PATCH 02/10] mm: Remove likely() from mapping_unevictable()

[Steven Rostedt]

On Fri, 2010-12-10 at 17:08 +0900, Miles Bader wrote:
> Joe Perches <joe@perches.com> 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

Re: [RFC][PATCH 03/10] workqueue: It is likely that WORKER_NOT_RUNNING is true

[Tejun Heo]

On 12/11/2010 01:08 AM, Steven Rostedt wrote:
> On Tue, 2010-12-07 at 10:49 +0100, Tejun Heo wrote:
> 
>> Yeah, I was lost thinking about the busiest case where workers are
>> busy processing works consecutively.  Usually workers are of course
>> switching in and out of idle state all the time.
>>
>> How about just dropping likely/unlikely?
> 
> OK, if I just remove it, can I add your Acked-by?

Sure, then, I'll route it through my tree.

Thanks.

-- 
tejun

Re: [RFC][PATCH 03/10] workqueue: It is likely that WORKER_NOT_RUNNING is true

[Steven Rostedt]

On Sat, 2010-12-11 at 01:09 +0100, Tejun Heo wrote:
> On 12/11/2010 01:08 AM, Steven Rostedt wrote:
> > On Tue, 2010-12-07 at 10:49 +0100, Tejun Heo wrote:
> > 
> >> Yeah, I was lost thinking about the busiest case where workers are
> >> busy processing works consecutively.  Usually workers are of course
> >> switching in and out of idle state all the time.
> >>
> >> How about just dropping likely/unlikely?
> > 
> > OK, if I just remove it, can I add your Acked-by?
> 
> Sure, then, I'll route it through my tree.


OK, I'll post a new patch series again. Just pluck it from that, and let
me know so I can remove it from my tree.

Thanks,

-- Steve

Re: [RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely

[Gregory Haskins]

Sorry for toppost...mobile.

Re: mail client: ack
Re: patch: ack

;)

-Greg 
-----Original Message-----
From: Steven Rostedt <rostedt@goodmis.org>
To: Gregory Haskins <GHaskins@novell.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc:  <linux-kernel@vger.kernel.org>

Sent: 12/10/2010 7:07:29 PM
Subject: Re: [RFC][PATCH 04/10] sched: Change pick_next_task_rt from unlikely to likely

On Mon, 2010-12-06 at 19:46 -0700, Gregory Haskins wrote:

> My feeling is that generally speaking, if the branch is workload dependent, we should probably not annotate it at all and let the CPUs branch-predictor do its thing.  I guess what I am not 100% clear on is how these annotations affect the BPU.  I.e. is it a static decision point or can the BPU still "learn" if the annotation is particularly wrong for a given workload?  For the former, I think we should just remove this particular annotation (and there are others that need review).  For the latter, this is obviously the right annotation we should be using in this particular case.
> 

You need to get a better email client ;-)

OK, if I just remove the likely() do you Ack it?

-- Steve