On 03/02/2008, Arjan van de Ven <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
