[PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[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.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

 include/asm-x86/pda.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.24-rc5/include/asm-x86/pda.h	2007-12-12 16:48:43.000000000 +0100
+++ 2.6.24-rc5-x86_64-unsigned-cpu/include/asm-x86/pda.h	2007-12-07 15:32:15.000000000 +0100
@@ -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;

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> wrote:

> 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

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 17.12.07 14:26 >>>
>
>* Jan Beulich <jbeulich@novell.com> wrote:
>
>> 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.

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

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Ingo Molnar]

* Jan Beulich <jbeulich@novell.com> 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

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Cyrill Gorcunov]

[Ingo Molnar - Mon, Dec 17, 2007 at 03:53:17PM +0100]
| 
| * Jan Beulich <jbeulich@novell.com> 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 <gorcunov@gmail.com>
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(-)

diff --git a/include/asm-x86/pda.h b/include/asm-x86/pda.h
index 35962bb..00410ec 100644
--- a/include/asm-x86/pda.h
+++ b/include/asm-x86/pda.h
@@ -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) 		\

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Ingo Molnar]

[-- Attachment #1: Type: text/plain, Size: 7046 bytes --]


* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> 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.4

And 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 <gorcunov@gmail.com>

clean up include/asm-x86/pda.h, as suggested by checkpatch.pl.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/asm-x86/pda.h |   70 +++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

Index: linux-x86.q/include/asm-x86/pda.h
===================================================================
--- linux-x86.q.orig/include/asm-x86/pda.h
+++ linux-x86.q/include/asm-x86/pda.h
@@ -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__;							\
 })

[-- Attachment #2: code-quality --]
[-- Type: text/plain, Size: 654 bytes --]


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

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> 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

Re: [PATCH] x86-64: make pda's cpunumber and nodenumber unsigned

[Cyrill Gorcunov]

[Ingo Molnar - Mon, Dec 17, 2007 at 05:01:27PM +0100]
| 
| * Cyrill Gorcunov <gorcunov@gmail.com> 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