Messages in this thread Patch in this message | | | Subject | Re: latencytop: optimize LT_BACKTRACEDEPTH loops a bit | From | Dmitry Adamushko <> | Date | Sun, 03 Feb 2008 22:33:58 +0100 |
| |
On 03/02/2008, Arjan van de Ven <arjan@linux.intel.com> wrote: > Dmitry Adamushko wrote: > > Subject: latencytop: optimize LT_BACKTRACEDEPTH loops a bit. > > > > It looks like there is no need to loop any longer when 'same == 0'. > > thanks for the contribution! > while I like your patch, I wonder if we should go even a little further in > cleaning this up > > > @@ -73,12 +73,12 @@ account_global_scheduler_latency(struct task_struct *tsk, struct latency_record > > continue; > > } > > for (q = 0 ; q < LT_BACKTRACEDEPTH ; q++) { > > - if (latency_record[i].backtrace[q] != > > - lat->backtrace[q]) > > + unsigned long record = lat->backtrace[q]; > > > > + if (latency_record[i].backtrace[q] != record) > > same = 0; > > - if (same && lat->backtrace[q] == 0) > > - break; > > - if (same && lat->backtrace[q] == ULONG_MAX) > > + > > + if (!same || record == 0 || record == ULONG_MAX) > > break; > > } > > I mean, we could make it look like this:
Yeah, I had some doubts regarding the '!same' case. We'd probably be better off taking a decision (i.e. break) from inside the first branch so to avoid the second one (I guess, it can be a bit more efficient, wrt the CPU's branch-prediction logic).
what about this one instead?
(I'd prefer to have a single 'if' for 'record == 0 || record == ULONG_MAX', it's just a matter of taste though)
---
Subject: latencytop: optimize LT_BACKTRACEDEPTH loops a bit.
It looks like there is no need to loop any longer when 'same == 0'.
Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>
---
diff --git a/kernel/latencytop.c b/kernel/latencytop.c index b4e3c85..5e4743d 100644 --- a/kernel/latencytop.c +++ b/kernel/latencytop.c @@ -64,8 +64,8 @@ account_global_scheduler_latency(struct task_struct *tsk, struct latency_record return; for (i = 0; i < MAXLR; i++) { - int q; - int same = 1; + int q, same = 1; + /* Nothing stored: */ if (!latency_record[i].backtrace[0]) { if (firstnonnull > i) @@ -73,12 +73,15 @@ account_global_scheduler_latency(struct task_struct *tsk, struct latency_record continue; } for (q = 0 ; q < LT_BACKTRACEDEPTH ; q++) { - if (latency_record[i].backtrace[q] != - lat->backtrace[q]) + unsigned long record = lat->backtrace[q]; + + if (latency_record[i].backtrace[q] != record) { same = 0; - if (same && lat->backtrace[q] == 0) break; - if (same && lat->backtrace[q] == ULONG_MAX) + } + + /* 0 and ULONG_MAX entries denote the end of backtrace: */ + if (record == 0 || record == ULONG_MAX) break; } if (same) { @@ -143,14 +146,18 @@ account_scheduler_latency(struct task_struct *tsk, int usecs, int inter) for (i = 0; i < LT_SAVECOUNT ; i++) { struct latency_record *mylat; int same = 1; + mylat = &tsk->latency_record[i]; for (q = 0 ; q < LT_BACKTRACEDEPTH ; q++) { - if (mylat->backtrace[q] != - lat.backtrace[q]) + unsigned long record = lat.backtrace[q]; + + if (mylat->backtrace[q] != record) { same = 0; - if (same && lat.backtrace[q] == 0) break; - if (same && lat.backtrace[q] == ULONG_MAX) + } + + /* 0 and ULONG_MAX entries denote the end of backtrace: */ + if (record == 0 || record == ULONG_MAX) break; } if (same) {
-- Best regards, Dmitry Adamushko
| |