Reduce number of pointer derefs in various files (kernel/exit.c used as example)
RFC PATCH 1/1 Reduce number of pointer derefs in various files (kernel/exit.c used as example)
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.
---
kernel/exit.c | 37 +++++++++++++++++++++---------------- 1 files changed, 21 insertions(+), 16 deletions(-)
kernel/exit.c +21 -16
@@ -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); }
Signature · 3 lines
Jesper Juhl <[email protected]> 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
Quoted 4 lines · Jesper Juhl
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.ofile 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.
Ingo
Quoted 8 lines · Ingo Molnar
Quoted 4 lines · Jesper Juhl
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.ofile was 19604 bytes in size, the patched one is 19508 bytes in size.
Quoted 8 lines
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.oso 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.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Am Freitag, 9. Dezember 2005 02:46 schrieb Matt Mackall:
Quoted 18 lines · Ingo Molnar
Quoted 8 lines · Jesper Juhl
Quoted 5 lines
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.ofile was 19604 bytes in size, the patched one is 19508 bytes in size.Quoted 8 lines
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.oso 80 bytes shaved off. I think such patches also increase readability.
Quoted 7 lines
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
Quoted 27 lines · Oliver Neukum
Am Freitag, 9. Dezember 2005 02:46 schrieb Matt Mackall:
Quoted 18 lines · Ingo Molnar
Quoted 8 lines · Jesper Juhl
Quoted 5 lines
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.ofile was 19604 bytes in size, the patched one is 19508 bytes in size.Quoted 8 lines
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.oso 80 bytes shaved off. I think such patches also increase readability.
Quoted 7 lines
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.
Quoted 4 lines
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.
Signature · 1 line
Mathematics is the supreme nostalgia of our time.
Quoted 8 lines · Matt Mackall
Quoted 8 lines
Quoted 4 lines
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.ofile 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:
Quoted 8 lines
Quoted 5 lines
text data bss dec hex filename
11077 0 0 11077 2b45 exit.o.orig 10997 0 0 10997 2af5 exit.oso 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.
Quoted 7 lines
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
Quoted 15 lines · Ingo Molnar
Quoted 12 lines · Matt Mackall
Quoted 12 lines
Quoted 4 lines
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.ofile 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
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 5 lines
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!
Signature · 1 line
Mathematics is the supreme nostalgia of our time.