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..61f7da0 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,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;
}
if (same) {
@@ -143,14 +143,15 @@ 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)
+
+ if (!same || record == 0 || record == ULONG_MAX)
break;
}
if (same) {
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:
if (latency_record[i].backtrace[q] != record) {
same = 0;
break;
}
/* 0 and ULONG_MAX entries denote the end of backtrace */
if (record == 0)
break;
if (record == ULONG_MAX)
break;
to me at least this is a bit more readable/simple than the good first step you've
already taken..
Do you want to do it this way? I'd sure encourage/endorse such a patch...
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
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'.
>
>
> Signed-off-by: Dmitry Adamushko <[email protected]>
looks good to me; Ingo can you queue this one up ?