LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* 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	[flat|nested] 4+ messages in thread

* Re: latencytop: optimize LT_BACKTRACEDEPTH loops a bit
  2008-02-03 21:33 latencytop: optimize LT_BACKTRACEDEPTH loops a bit Dmitry Adamushko
@ 2008-02-04  4:31 ` Arjan van de Ven
  0 siblings, 0 replies; 4+ messages in thread
From: Arjan van de Ven @ 2008-02-04  4:31 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'.
> 
> 
> Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>

looks good to me; Ingo can you queue this one up ?

^ permalink raw reply	[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

* 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	[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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).