x86-64: make pda's cpunumber and nodenumber unsigned
PATCH 1/1 x86-64: make pda's cpunumber and nodenumber unsigned
This generally allows better code to be generated, since the zero- extension during 32-bit operations comes for free (needed when the result is used as array index or similar), whereas sign extension must be done explicitly and frequently requires a one byte larger instruction due to the necessary rex64 prefix.
include/asm-x86/pda.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/asm-x86/pda.h +2 -2
@@ -15,14 +15,14 @@ struct x8664_pda {unsigned long kernelstack;/* 16 top of kernel stack for current */ unsigned long oldrsp;/* 24 user rsp for system call */ int irqcount;/* 32 Irq nesting counter. Starts with -1 */ -int cpunumber;/* 36 Logical CPU number */ +unsigned int cpunumber;/* 36 Logical CPU number */ #ifdef CONFIG_CC_STACKPROTECTOR unsigned long stack_canary;/* 40 stack canary value */ /* gcc-ABI: this canary MUST be at offset 40!!! */ #endif char *irqstackptr; -int nodenumber;/* number of current node */ +unsigned int nodenumber;/* number of current node */ unsigned int __softirq_pending;unsigned int __nmi_count;/* number of NMI on this CPUs */ short mmu_state;
Quoted 5 lines · Jan Beulich
This generally allows better code to be generated, since the zero- extension during 32-bit operations comes for free (needed when the result is used as array index or similar), whereas sign extension must be done explicitly and frequently requires a one byte larger instruction due to the necessary rex64 prefix.
good catch! Applied your patch to x86.git - queued it up for v2.6.25. I bet there are tons of other instances where we use signed instead of unsigned and get worse code generation.
Ingo
Quoted 10 lines
Quoted 1 line
Quoted 1 line
Ingo Molnar <[email protected]> 17.12.07 14:26 >>>
Quoted 5 lines · Jan Beulich
This generally allows better code to be generated, since the zero- extension during 32-bit operations comes for free (needed when the result is used as array index or similar), whereas sign extension must be done explicitly and frequently requires a one byte larger instruction due to the necessary rex64 prefix.
Quoted 3 lines
good catch! Applied your patch to x86.git - queued it up for v2.6.25. I bet there are tons of other instances where we use signed instead of unsigned and get worse code generation.
Yes, definitely. This patch was kind of a testing one whether this is a welcome change. As it appears to be, I'll probably produce more as I run into respective cases.
Jan
Quoted 7 lines · Jan Beulich
Quoted 3 lines
good catch! Applied your patch to x86.git - queued it up for v2.6.25. I bet there are tons of other instances where we use signed instead of unsigned and get worse code generation.
Yes, definitely. This patch was kind of a testing one whether this is a welcome change. As it appears to be, I'll probably produce more as I run into respective cases.
they are definitely welcome! Especially when fields are also used in integer multiplications or divisions then the cost of signedness can be quite significant. We regularly do such int -> uint sweeps in the scheduler code and it's a great code size reductor.
Btw., a sidenote: if you are touching files that you care about, and if you've got a few spare cycles, you might also want to take a look at checkpatch.pl output:
$ scripts/checkpatch.pl --file include/asm-x86/pda.h ... total: 23 errors, 1 warnings, 133 lines checked
as it might have a few low hanging fruits ripe for cleanup ;-)
Ingo
[Ingo Molnar - Mon, Dec 17, 2007 at 03:53:17PM +0100]
| | * Jan Beulich <[email protected]> wrote: | | > > good catch! Applied your patch to x86.git - queued it up for | > > v2.6.25. I bet there are tons of other instances where we use signed | > > instead of unsigned and get worse code generation. | > | > Yes, definitely. This patch was kind of a testing one whether this is | > a welcome change. As it appears to be, I'll probably produce more as I | > run into respective cases. | | they are definitely welcome! Especially when fields are also used in | integer multiplications or divisions then the cost of signedness can be | quite significant. We regularly do such int -> uint sweeps in the | scheduler code and it's a great code size reductor. | | Btw., a sidenote: if you are touching files that you care about, and if | you've got a few spare cycles, you might also want to take a look at | checkpatch.pl output: | | $ scripts/checkpatch.pl --file include/asm-x86/pda.h | ... | total: 23 errors, 1 warnings, 133 lines checked | | as it might have a few low hanging fruits ripe for cleanup ;-) | | Ingo |
Hi, I've tried to make a one ;) It's over last (today synced) linus tree.
Cyrill
---
From: Cyrill Gorcunov <[email protected]>
Subject: [PATCH] x86: cleanup pda.h to fulfil checkpatch
---
Checkpatch still does complain about
if (0) { T__ tmp__; tmp__ = (val)I'm not sure if we need this line at all.
include/asm-x86/pda.h | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)
include/asm-x86/pda.h +18 -18
@@ -7,14 +7,14 @@#include <linux/cache.h> #include <asm/page.h> -/* Per processor datastructure. %gs points to it while the kernel runs */ +/* Per processor datastructure. %gs points to it while the kernel runs */ struct x8664_pda {struct task_struct *pcurrent;/* 0 Current process */ unsigned long data_offset;/* 8 Per cpu data offset from linker address */ unsigned long kernelstack;/* 16 top of kernel stack for current */ unsigned long oldrsp;/* 24 user rsp for system call */ -int irqcount;/* 32 Irq nesting counter. Starts with -1 */ +int irqcount;/* 32 Irq nesting counter. Starts with -1 */ int cpunumber;/* 36 Logical CPU number */ #ifdef CONFIG_CC_STACKPROTECTOR unsigned long stack_canary;/* 40 stack canary value */
@@ -43,10 +43,10 @@ extern struct x8664_pda boot_cpu_pda[];#define cpu_pda (i) (_cpu_pda[i]) -/* +/* * There is no fast way to get the base address of the PDA, all the accesses * have to mention %fs/%gs. So it needs to be done this Torvaldian way. -*/ +*/ extern void __bad_pda_field (void ) __attribute__((noreturn));/*
@@ -57,7 +57,7 @@ extern struct x8664_pda _proxy_pda;#define pda_offset (field) offsetof(struct x8664_pda, field) -#define pda_to_op (op,field,val) do { \ +#define pda_to_op (op, field, val) do { \ typedef typeof(_proxy_pda.field) T__; \ if (0) { T__ tmp__; tmp__ = (val); }/* type checking */ \switch (sizeof (_proxy_pda.field)) { \
@@ -66,7 +66,7 @@ extern struct x8664_pda _proxy_pda; "+m" (_proxy_pda.field) : \"ri" ((T__ )val), \ "i"(pda_offset(field))); \ -break ; \ +break ; \case 4 : \ asm(op"l %1,%%gs:%c2" : \ "+m" (_proxy_pda.field) : \
@@ -79,15 +79,15 @@ extern struct x8664_pda _proxy_pda;"ri" ((T__ )val), \ "i"(pda_offset(field))); \break ; \ -default : \ +default : \__bad_pda_field (); \ - } \ - } while (0) + } \ + } while (0)#define pda_from_op (op,field) ({ \ typeof(_proxy_pda.field) ret__; \ switch (sizeof(_proxy_pda.field)) { \ - case 2: \ + case 2: \ asm(op "w %%gs:%c1,%0" : \ "=r" (ret__) : \ "i" (pda_offset(field)), \
@@ -99,25 +99,25 @@ extern struct x8664_pda _proxy_pda;"i" (pda_offset (field)), \ "m" (_proxy_pda.field)); \break ; \ -case 8 : \ +case 8 : \ asm(op"q %%gs:%c1,%0" : \"=r" (ret__) : \"i" (pda_offset (field)), \"m" (_proxy_pda.field)); \break ; \ -default : \ +default : \__bad_pda_field (); \ } \ ret__; }) -#define read_pda (field) pda_from_op("mov",field) -#define write_pda (field,val) pda_to_op("mov",field,val) -#define add_pda (field,val) pda_to_op("add",field,val) -#define sub_pda (field,val) pda_to_op("sub",field,val) -#define or_pda (field,val) pda_to_op("or",field,val) +#define read_pda (field) pda_from_op("mov", field) +#define write_pda (field, val) pda_to_op("mov", field, val) +#define add_pda (field, val) pda_to_op("add", field, val) +#define sub_pda (field, val) pda_to_op("sub", field, val) +#define or_pda (field, val) pda_to_op("or", field, val)/* This is not atomic against other CPUs -- CPU preemption needs to be off */ -#define test_and_clear_bit_pda (bit,field) ({ \ +#define test_and_clear_bit_pda (bit, field) ({ \ int old__; \ asm volatile("btr %2,%%gs:%c3\n\tsbbl %0,%0" \ : "=r" (old__), "+m" (_proxy_pda.field) \
Ingo Molnar
AppliedQuoted 2 lines · Cyrill Gorcunov
Hi, I've tried to make a one ;) It's over last (today synced) linus tree.
thanks, applied :-)
Some small details: i had to hand-apply your patch to the x86.git#mm tree (see the instructions below how to fetch that tree). It's best (for us maintainers) if you send such cleanup patches against that tree, to reduce patch collision risk. I have attached the final merged patch further below.
just in case you are on the lookout for other cleanup targets, i've attached the "code-quality" script that gives us the mechanic code quality metric of any file or tree. For example, it nicely shows the cleanup effects of your patch on the pda.h file:
errors lines of code errors/KLOC
include/asm-x86/pda.h [before] 23 133 172.9 include/asm-x86/pda.h [after] 1 133 7.5
if you want to pick cleanup targets from the x86 tree, do something like this:
code-quality find arch/x86 include/asm-x86/ -name '*.c' | tee quality.txt
sort -n -k 4 quality.txt
Warning: some files are very hard to clean up. Ask on lkml if unsure.
The current code quality of arch/x86/ (as of x86.git#mm) is:
$ code-quality arch/x86/
errors lines of code errors/KLOC
arch/x86/ 4277 117235 36.4And the long term goal is to achieve the output of:
code-quality kernel/sched*.[ch]
/me runs and hides ;-)
Ingo
----->
git-clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git linux-2.6.git cd linux-2.6.git
git-branch x86
git-checkout x86
git-pull git://git.kernel.org/pub/scm/linux/kernel/git/x86/linux-2.6-x86.git mm
(do subsequent pulls via "git-pull --force", as we frequently rebase the git tree. NOTE: this might override your own local changes, so do this only if you dont mind about losing thse changes in that tree.)
--------------->
Subject: x86: clean up include/asm-x86/pda.h
From: Cyrill Gorcunov <[email protected]>
clean up include/asm-x86/pda.h, as suggested by checkpatch.pl.
--- include/asm-x86/pda.h | 70 +++++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 35 deletions(-)
include/asm-x86/pda.h +35 -35
@@ -7,15 +7,15 @@#include <linux/cache.h> #include <asm/page.h> -/* Per processor datastructure. %gs points to it while the kernel runs */ +/* Per processor datastructure. %gs points to it while the kernel runs */ struct x8664_pda {struct task_struct *pcurrent;/* 0 Current process */ unsigned long data_offset;/* 8 Per cpu data offset from linker address */ -unsigned long kernelstack;/* 16 top of kernel stack for current */ -unsigned long oldrsp;/* 24 user rsp for system call */ -int irqcount;/* 32 Irq nesting counter. Starts with -1 */ -unsigned int cpunumber;/* 36 Logical CPU number */ +unsigned long kernelstack;/* 16 top of kernel stack for current */ +unsigned long oldrsp;/* 24 user rsp for system call */ +int irqcount;/* 32 Irq nesting counter. Starts -1 */ +unsigned int cpunumber;/* 36 Logical CPU number */ #ifdef CONFIG_CC_STACKPROTECTOR unsigned long stack_canary;/* 40 stack canary value */ /* gcc-ABI: this canary MUST be at
@@ -44,10 +44,10 @@ extern void pda_init(int);#define cpu_pda (i) (_cpu_pda[i]) -/* +/* * There is no fast way to get the base address of the PDA, all the accesses * have to mention %fs/%gs. So it needs to be done this Torvaldian way. -*/ +*/ extern void __bad_pda_field (void ) __attribute__((noreturn));/*
@@ -58,70 +58,70 @@ extern struct x8664_pda _proxy_pda;#define pda_offset (field) offsetof(struct x8664_pda, field) -#define pda_to_op(op,field,val) do { \ +#define pda_to_op(op, field, val) do { \ typedef typeof(_proxy_pda.field) T__; \ if (0) { T__ tmp__; tmp__ = (val); } /* type checking */ \ switch (sizeof(_proxy_pda.field)) { \ case 2: \ - asm(op "w %1,%%gs:%c2" : \ + asm(op "w %1,%%gs:%c2" : \ "+m" (_proxy_pda.field) : \ "ri" ((T__)val), \ - "i"(pda_offset(field))); \ - break; \ + "i"(pda_offset(field))); \ + break; \ case 4: \ - asm(op "l %1,%%gs:%c2" : \ + asm(op "l %1,%%gs:%c2" : \ "+m" (_proxy_pda.field) : \ "ri" ((T__)val), \ - "i" (pda_offset(field))); \ + "i" (pda_offset(field))); \ break; \ case 8: \ - asm(op "q %1,%%gs:%c2": \ + asm(op "q %1,%%gs:%c2": \ "+m" (_proxy_pda.field) : \ "ri" ((T__)val), \ - "i"(pda_offset(field))); \ + "i"(pda_offset(field))); \ break; \ - default: \ + default: \ __bad_pda_field(); \ - } \ - } while (0) + } \ + } while (0)#define pda_from_op (op,field) ({ \ typeof(_proxy_pda.field) ret__; \ switch (sizeof(_proxy_pda.field)) { \ - case 2: \ - asm(op "w %%gs:%c1,%0" : \ + case 2: \ + asm(op "w %%gs:%c1,%0" : \ "=r" (ret__) : \ - "i" (pda_offset(field)), \ - "m" (_proxy_pda.field)); \ + "i" (pda_offset(field)), \ + "m" (_proxy_pda.field)); \ break; \ case 4: \ asm(op "l %%gs:%c1,%0": \ "=r" (ret__): \ - "i" (pda_offset(field)), \ - "m" (_proxy_pda.field)); \ + "i" (pda_offset(field)), \ + "m" (_proxy_pda.field)); \ break; \ - case 8: \ + case 8: \ asm(op "q %%gs:%c1,%0": \ "=r" (ret__) : \ - "i" (pda_offset(field)), \ - "m" (_proxy_pda.field)); \ + "i" (pda_offset(field)), \ + "m" (_proxy_pda.field)); \ break; \ - default: \ + default: \ __bad_pda_field(); \ } \ ret__; }) -#define read_pda (field) pda_from_op("mov",field) -#define write_pda (field,val) pda_to_op("mov",field,val) -#define add_pda (field,val) pda_to_op("add",field,val) -#define sub_pda (field,val) pda_to_op("sub",field,val) -#define or_pda (field,val) pda_to_op("or",field,val) +#define read_pda (field) pda_from_op("mov", field) +#define write_pda (field, val) pda_to_op("mov", field, val) +#define add_pda (field, val) pda_to_op("add", field, val) +#define sub_pda (field, val) pda_to_op("sub", field, val) +#define or_pda (field, val) pda_to_op("or", field, val)/* This is not atomic against other CPUs -- CPU preemption needs to be off */ -#define test_and_clear_bit_pda (bit,field) ({ \ +#define test_and_clear_bit_pda (bit, field) ({ \ int old__; \ asm volatile("btr %2,%%gs:%c3\n\tsbbl %0,%0" \ - : "=r" (old__), "+m" (_proxy_pda.field) \ + : "=r" (old__), "+m" (_proxy_pda.field) \ : "dIr" (bit), "i" (pda_offset(field)) : "memory"); \ old__; \ })
SRC=$@
[ "$SRC" = "" ] && { SRC=q files; }
FILE=/tmp/1.c
echo " errors lines of code errors/KLOC"
for DIR in $SRC; do
printf "%-35s " $DIR find $DIR -name '*.[ch]' | xargs cat > $FILE RES=`scripts/checkpatch.pl --file $FILE | grep ^total`
# echo $RES
if [ "$RES" != "" ]; then
ERR=`echo $RES | cut -d' ' -f2`
WARN=`echo $RES | cut -d' ' -f4`
LINES=`echo $RES | cut -d' ' -f6`
else
ERR=0
WARN=0
LINES=`cat $FILE | wc -l`fi
rm -f $FILE
ERR_KLOC=( echo "scale=1"; echo "$ERR*1000/$LINES" ) | bc
printf "%8s %10s %10s\n" $ERR $LINES $ERR_KLOC
done
Quoted 3 lines · Cyrill Gorcunov
Checkpatch still does complain about
if (0) { T__ tmp__; tmp__ = (val)I'm not sure if we need this line at all.
that's a type-checking trick. It does not result in any generated code but gcc flags it with a build time warning if there's a type mismatch. So it's a checkpatch.pl false positive.
Ingo
[Ingo Molnar - Mon, Dec 17, 2007 at 05:01:27PM +0100]
| | * Cyrill Gorcunov <[email protected]> wrote: | | > Checkpatch still does complain about | > if (0) { T__ tmp__; tmp__ = (val) | > I'm not sure if we need this line at all. | | that's a type-checking trick. It does not result in any generated code | but gcc flags it with a build time warning if there's a type mismatch. | So it's a checkpatch.pl false positive. | | Ingo |
Thanks Ingo!
I'll fetch up the tree you mentoined in prev letter and next time try to be more carefull with patches I'm sending (i'm a bit busy with my second machine on which i'm trying to install fc8 but anaconda hangs again and again so that is the reason I'm not sync'ed with x86 and mm trees yet, sorry)
Cyrill