[RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Jesper Juhl]

Hi,

There are a lot of places in the kernel where we dereference several pointers
in a row several times.
As far as I can tell it should be more efficient to just deref those pointers
once, store the result in a variable, and then use that (unless the use is very
limited of course). 

To "test the waters" before I do a lot of work that won't be accepted, I picked
a random file with generic kernel code that has this issue and made a patch.

What I'd like to know is if anyone sees any problems with this and if patches
such as this one would be considered a good thing.

If patches like the one below are wanted I'll have a bunch of similar ones
ready to go pretty soon.

I've compile tested this patch and I'm currently running a 2.6.15-rc5-git1
kernel with it applied without problems.

Ohh, and before I forget, besides the fact that this should speed things up a
little bit it also has the added benefit of reducing the size of the generated
code.
The original kernel/exit.o file was 19604 bytes in size, the patched one is
19508 bytes in size.

Comments very welcome.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 kernel/exit.c |   37 +++++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 16 deletions(-)

--- linux-2.6.15-rc5-git1-orig/kernel/exit.c	2005-12-04 18:48:53.000000000 +0100
+++ linux-2.6.15-rc5-git1/kernel/exit.c	2005-12-06 22:11:17.000000000 +0100
@@ -1068,6 +1068,9 @@ static int wait_task_zombie(task_t *p, i
 	}
 
 	if (likely(p->real_parent == p->parent) && likely(p->signal)) {
+		struct signal_struct *psig;
+		struct signal_struct *sig;
+		
 		/*
 		 * The resource counters for the group leader are in its
 		 * own task_struct.  Those for dead threads in the group
@@ -1084,24 +1087,26 @@ static int wait_task_zombie(task_t *p, i
 		 * here reaping other children at the same time.
 		 */
 		spin_lock_irq(&p->parent->sighand->siglock);
-		p->parent->signal->cutime =
-			cputime_add(p->parent->signal->cutime,
+		psig = p->parent->signal;
+		sig = p->signal;
+		psig->cutime =
+			cputime_add(psig->cutime,
 			cputime_add(p->utime,
-			cputime_add(p->signal->utime,
-				    p->signal->cutime)));
-		p->parent->signal->cstime =
-			cputime_add(p->parent->signal->cstime,
+			cputime_add(sig->utime,
+				    sig->cutime)));
+		psig->cstime =
+			cputime_add(psig->cstime,
 			cputime_add(p->stime,
-			cputime_add(p->signal->stime,
-				    p->signal->cstime)));
-		p->parent->signal->cmin_flt +=
-			p->min_flt + p->signal->min_flt + p->signal->cmin_flt;
-		p->parent->signal->cmaj_flt +=
-			p->maj_flt + p->signal->maj_flt + p->signal->cmaj_flt;
-		p->parent->signal->cnvcsw +=
-			p->nvcsw + p->signal->nvcsw + p->signal->cnvcsw;
-		p->parent->signal->cnivcsw +=
-			p->nivcsw + p->signal->nivcsw + p->signal->cnivcsw;
+			cputime_add(sig->stime,
+				    sig->cstime)));
+		psig->cmin_flt +=
+			p->min_flt + sig->min_flt + sig->cmin_flt;
+		psig->cmaj_flt +=
+			p->maj_flt + sig->maj_flt + sig->cmaj_flt;
+		psig->cnvcsw +=
+			p->nvcsw + sig->nvcsw + sig->cnvcsw;
+		psig->cnivcsw +=
+			p->nivcsw + sig->nivcsw + sig->cnivcsw;
 		spin_unlock_irq(&p->parent->sighand->siglock);
 	}
 




-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Ingo Molnar]

* Jesper Juhl <jesper.juhl@gmail.com> wrote:

> Ohh, and before I forget, besides the fact that this should speed 
> things up a little bit it also has the added benefit of reducing the 
> size of the generated code. The original kernel/exit.o file was 19604 
> bytes in size, the patched one is 19508 bytes in size.

nice. Just to underline your point, on x86, with gcc 4.0.2, i'm getting 
this with your patch:

   text    data     bss     dec     hex filename
  11077       0       0   11077    2b45 exit.o.orig
  10997       0       0   10997    2af5 exit.o

so 80 bytes shaved off. I think such patches also increase readability.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Matt Mackall]

On Tue, Dec 06, 2005 at 11:15:28PM +0100, Ingo Molnar wrote:
> 
> * Jesper Juhl <jesper.juhl@gmail.com> wrote:
> 
> > Ohh, and before I forget, besides the fact that this should speed 
> > things up a little bit it also has the added benefit of reducing the 
> > size of the generated code. The original kernel/exit.o file was 19604 
> > bytes in size, the patched one is 19508 bytes in size.
> 
> nice. Just to underline your point, on x86, with gcc 4.0.2, i'm getting 
> this with your patch:
> 
>    text    data     bss     dec     hex filename
>   11077       0       0   11077    2b45 exit.o.orig
>   10997       0       0   10997    2af5 exit.o
> 
> so 80 bytes shaved off. I think such patches also increase readability.

Readability improved: good.
37 lines of patch for 80-100 bytes saved: not so good.

So while this is a good style direction, I don't think it's worth the
churn. And unlike kzalloc and the like, this particular optimization
is perfectly doable by a compiler. So I'd rather wait for the compiler
to get smarter than change code for such modest improvements.

FYI, much other low-hanging size-reduction fruit remains in the
kernel. Lots of it in the form of duplicate code.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Oliver Neukum]

Am Freitag, 9. Dezember 2005 02:46 schrieb Matt Mackall:
> On Tue, Dec 06, 2005 at 11:15:28PM +0100, Ingo Molnar wrote:
> > 
> > * Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > 
> > > Ohh, and before I forget, besides the fact that this should speed 
> > > things up a little bit it also has the added benefit of reducing the 
> > > size of the generated code. The original kernel/exit.o file was 19604 
> > > bytes in size, the patched one is 19508 bytes in size.
> > 
> > nice. Just to underline your point, on x86, with gcc 4.0.2, i'm getting 
> > this with your patch:
> > 
> >    text    data     bss     dec     hex filename
> >   11077       0       0   11077    2b45 exit.o.orig
> >   10997       0       0   10997    2af5 exit.o
> > 
> > so 80 bytes shaved off. I think such patches also increase readability.
> 
> Readability improved: good.
> 37 lines of patch for 80-100 bytes saved: not so good.
> 
> So while this is a good style direction, I don't think it's worth the
> churn. And unlike kzalloc and the like, this particular optimization
> is perfectly doable by a compiler. So I'd rather wait for the compiler
> to get smarter than change code for such modest improvements.

How can the compiler do it? If a function call is between two evaluations
of a pointer chain, the compiler would have to make sure no pointer in
the chain is touched. For the case of a computed function call, it is
impossible in principle.

	Regards
		Oliver

Re: [RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Ingo Molnar]

* Matt Mackall <mpm@selenic.com> wrote:

> > > Ohh, and before I forget, besides the fact that this should speed 
> > > things up a little bit it also has the added benefit of reducing the 
> > > size of the generated code. The original kernel/exit.o file was 19604 
> > > bytes in size, the patched one is 19508 bytes in size.
> > 
> > nice. Just to underline your point, on x86, with gcc 4.0.2, i'm getting 
> > this with your patch:
> > 
> >    text    data     bss     dec     hex filename
> >   11077       0       0   11077    2b45 exit.o.orig
> >   10997       0       0   10997    2af5 exit.o
> > 
> > so 80 bytes shaved off. I think such patches also increase readability.
> 
> Readability improved: good.
> 37 lines of patch for 80-100 bytes saved: not so good.

i'd take a 37 lines readability patch even if it didnt give us a byte of 
text back. The fact that it also reduces text size on the latest gcc in 
rawhide is an added bonus. (of course the patch is 2.6.16 material)

in fact we frequently apply patches that _reduce_ readability but which 
e.g. reduce the number of 'current->' dereferences. (That too is 
something the compiler could figure out.)

also, besides the size reduction effect, this patch is also a 
speed-micro-optimization.

> So while this is a good style direction, I don't think it's worth the 
> churn. And unlike kzalloc and the like, this particular optimization 
> is perfectly doable by a compiler. So I'd rather wait for the compiler 
> to get smarter than change code for such modest improvements.
> 
> FYI, much other low-hanging size-reduction fruit remains in the 
> kernel. Lots of it in the form of duplicate code.

i agree with you that other low-hanging fruits exist, but this does not 
diminish the value of this patch. The patch is obviously correct, it is 
cleaner and improves size and speed. Size reduction alone does not 
necessarily trump cleanliness, but in this particular case all factors 
show towards acceptance.

furthermore, i think that even if it's a small step, we should encourage 
every effort that reduces the kernel's text size. The 2.4 -> 2.6 
transition blew up the kernel by ~50%, and we've got to win back some of 
that. (Kernel size is one of the main disadvantages of Linux in the 
embedded market, compared to other OSs.)

	Ingo

Re: [RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Matt Mackall]

On Fri, Dec 09, 2005 at 09:14:21AM +0100, Oliver Neukum wrote:
> Am Freitag, 9. Dezember 2005 02:46 schrieb Matt Mackall:
> > On Tue, Dec 06, 2005 at 11:15:28PM +0100, Ingo Molnar wrote:
> > > 
> > > * Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > > 
> > > > Ohh, and before I forget, besides the fact that this should speed 
> > > > things up a little bit it also has the added benefit of reducing the 
> > > > size of the generated code. The original kernel/exit.o file was 19604 
> > > > bytes in size, the patched one is 19508 bytes in size.
> > > 
> > > nice. Just to underline your point, on x86, with gcc 4.0.2, i'm getting 
> > > this with your patch:
> > > 
> > >    text    data     bss     dec     hex filename
> > >   11077       0       0   11077    2b45 exit.o.orig
> > >   10997       0       0   10997    2af5 exit.o
> > > 
> > > so 80 bytes shaved off. I think such patches also increase readability.
> > 
> > Readability improved: good.
> > 37 lines of patch for 80-100 bytes saved: not so good.
> > 
> > So while this is a good style direction, I don't think it's worth the
> > churn. And unlike kzalloc and the like, this particular optimization
> > is perfectly doable by a compiler. So I'd rather wait for the compiler
> > to get smarter than change code for such modest improvements.
> 
> How can the compiler do it? If a function call is between two evaluations
> of a pointer chain, the compiler would have to make sure no pointer in
> the chain is touched. For the case of a computed function call, it is
> impossible in principle.

Excellent point. It'd require marking functions const, which might not
be a bad idea.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH] Reduce number of pointer derefs in various files (kernel/exit.c used as example)

[Matt Mackall]

On Fri, Dec 09, 2005 at 11:29:14AM +0100, Ingo Molnar wrote:
> 
> * Matt Mackall <mpm@selenic.com> wrote:
> 
> > > > Ohh, and before I forget, besides the fact that this should speed 
> > > > things up a little bit it also has the added benefit of reducing the 
> > > > size of the generated code. The original kernel/exit.o file was 19604 
> > > > bytes in size, the patched one is 19508 bytes in size.
> > > 
> > > nice. Just to underline your point, on x86, with gcc 4.0.2, i'm getting 
> > > this with your patch:
> > > 
> > >    text    data     bss     dec     hex filename
> > >   11077       0       0   11077    2b45 exit.o.orig
> > >   10997       0       0   10997    2af5 exit.o
> > > 
> > > so 80 bytes shaved off. I think such patches also increase readability.
> > 
> > Readability improved: good.
> > 37 lines of patch for 80-100 bytes saved: not so good.
> 
> i'd take a 37 lines readability patch even if it didnt give us a byte of 
> text back. The fact that it also reduces text size on the latest gcc in 
> rawhide is an added bonus. (of course the patch is 2.6.16 material)

So long as we're primarily doing it for the former reason rather than
the latter.

> furthermore, i think that even if it's a small step, we should encourage 
> every effort that reduces the kernel's text size. The 2.4 -> 2.6 
> transition blew up the kernel by ~50%, and we've got to win back some of 
> that. (Kernel size is one of the main disadvantages of Linux in the 
> embedded market, compared to other OSs.)

Boggle. You're telling /me/ this? You're the one who's been adding all
the damn features!

-- 
Mathematics is the supreme nostalgia of our time.