* Re: latencytop: optimize LT_BACKTRACEDEPTH loops a bit
@ 2008-02-03 21:33 Dmitry Adamushko
2008-02-04 4:31 ` Arjan van de Ven
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Adamushko @ 2008-02-03 21:33 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Ingo Molnar, Linux Kernel Mailing List, dmitry.adamushko
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* latencytop: optimize LT_BACKTRACEDEPTH loops a bit
@ 2008-02-02 22:59 Dmitry Adamushko
2008-02-03 0:11 ` Arjan van de Ven
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Adamushko @ 2008-02-02 22:59 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Ingo Molnar, Linux Kernel Mailing List, dmitry.adamushko
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..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) {
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: latencytop: optimize LT_BACKTRACEDEPTH loops a bit
2008-02-02 22:59 Dmitry Adamushko
@ 2008-02-03 0:11 ` Arjan van de Ven
0 siblings, 0 replies; 4+ messages in thread
From: Arjan van de Ven @ 2008-02-03 0:11 UTC (permalink / raw)
To: Dmitry Adamushko; +Cc: Ingo Molnar, Linux Kernel Mailing List
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...
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-02-04 4:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-02-03 21:33 latencytop: optimize LT_BACKTRACEDEPTH loops a bit Dmitry Adamushko
2008-02-04 4:31 ` Arjan van de Ven
-- strict thread matches above, loose matches on Subject: below --
2008-02-02 22:59 Dmitry Adamushko
2008-02-03 0:11 ` Arjan van de Ven
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.