[PATCH] New cpu macro and i386 cleanup

[PATCH] New cpu macro and i386 cleanup

[Chuck Ebbert]

 This annoyed me just enough to try and fix it.  I'd
fix all the rest but I know how annoyed people get at
massive changes:


diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/include/linux/smp.h linux-2.5.66-uni/include/linux/smp.h
--- linux-2.5.66-ref/include/linux/smp.h	Tue Mar  4 22:29:21 2003
+++ linux-2.5.66-uni/include/linux/smp.h	Sat Mar 29 15:05:56 2003
@ -134,6 +134,8 @
 }
 #endif /* !SMP */
 
+#define is_current_cpu(cpu)	((cpu) == smp_processor_id())
+
 #define get_cpu()		({ preempt_disable(); smp_processor_id(); })
 #define put_cpu()		preempt_enable()
 #define put_cpu_no_resched()	preempt_enable_no_resched()
diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/arch/i386/kernel/cpuid.c linux-2.5.66-uni/arch/i386/kernel/cpuid.c
--- linux-2.5.66-ref/arch/i386/kernel/cpuid.c	Sat Mar 29 09:16:32 2003
+++ linux-2.5.66-uni/arch/i386/kernel/cpuid.c	Fri Apr  4 18:45:19 2003
@ -56,7 +56,7 @
 {
   struct cpuid_command *cmd = (struct cpuid_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
+  if ( is_current_cpu(cmd->cpu) )
     cpuid(cmd->reg, &cmd->data[0], &cmd->data[1], &cmd->data[2], &cmd->data[3]);
 }
 
@ -65,7 +65,7 @
   struct cpuid_command cmd;
   
   preempt_disable();
-  if ( cpu == smp_processor_id() ) {
+  if ( is_current_cpu(cpu) ) {
     cpuid(reg, &data[0], &data[1], &data[2], &data[3]);
   } else {
     cmd.cpu  = cpu;
diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/arch/i386/kernel/msr.c linux-2.5.66-uni/arch/i386/kernel/msr.c
--- linux-2.5.66-ref/arch/i386/kernel/msr.c	Sat Mar 29 09:16:32 2003
+++ linux-2.5.66-uni/arch/i386/kernel/msr.c	Fri Apr  4 18:47:27 2003
@ -100,7 +100,7 @
 {
   struct msr_command *cmd = (struct msr_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
+  if ( is_current_cpu(cmd->cpu) )
     cmd->err = wrmsr_eio(cmd->reg, cmd->data[0], cmd->data[1]);
 }
 
@ -108,7 +108,7 @
 {
   struct msr_command *cmd = (struct msr_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
+  if ( is_current_cpu(cmd->cpu) )
     cmd->err = rdmsr_eio(cmd->reg, &cmd->data[0], &cmd->data[1]);
 }
 
@ -118,7 +118,7 @
   int ret;
 
   preempt_disable();
-  if ( cpu == smp_processor_id() ) {
+  if ( is_current_cpu(cpu) ) {
     ret = wrmsr_eio(reg, eax, edx);
     preempt_enable();
     return ret;
@ -138,7 +138,7 @
 {
   struct msr_command cmd;
 
-  if ( cpu == smp_processor_id() ) {
+  if ( is_current_cpu(cpu) ) {
     return rdmsr_eio(reg, eax, edx);
   } else {
     cmd.cpu = cpu;

--
 Chuck
 I am not a number!

Re: [PATCH] New cpu macro and i386 cleanup

[Robert Love]

On Fri, 2003-04-04 at 19:29, Chuck Ebbert wrote:
>  This annoyed me just enough to try and fix it.  I'd
> fix all the rest but I know how annoyed people get at
> massive changes:

I like, although I am not hot on the name, but that is just taste.

One minor nit: it is not preempt-safe.  But then again, neither is any
code that calls it without disabling preemption first.  Maybe put a
comment above it like:

	/*
	 * is_current_cpu() - are we running on the given processor?
	 *
	 * Note this is not preempt-safe: you need to explicitly
	 * disable preemption before calling this if you care
	 * about any consistency in the result
	 */

The problem is not so much the call itself, but the use of the results. 
Anywhere you care that 'cpu' is the current processor, kernel preemption
needs to remain disabled.

	Robert Love

Re: [PATCH] New cpu macro and i386 cleanup

[Chuck Ebbert]

Robert Love wrote:

> I like, although I am not hot on the name, but that is just taste.


I found myself wishing for C++ just for the function overloading so
it could be 'is_current(cpu)' or similar.


> One minor nit: it is not preempt-safe.


I was wondering whether the code I converted was running with preempt
disabled or not but didn't check.  (The very thought of preempt on
SMP scares me anyway, so I avoid it.)

smp_processor_id() is not preempt-safe either, since the id could
change before you even get a chance to use the value.  How many
thousands of lines of code remain that were written assuming things
would not change underneath them in kernel mode?


> Maybe put a comment above it like:


How about one for the whole kernel?

        /**********
         * WARNING: Use preempt at your own risk.
         **********/  

--
 Chuck
 I am not a number!

Re: [PATCH] New cpu macro and i386 cleanup

[Robert Love]

On Sat, 2003-04-05 at 05:24, Chuck Ebbert wrote:

> I was wondering whether the code I converted was running with preempt
> disabled or not but didn't check.  (The very thought of preempt on
> SMP scares me anyway, so I avoid it.)

On a glance, it looked like at least some of them were.

> smp_processor_id() is not preempt-safe either, since the id could
> change before you even get a chance to use the value.  How many
> thousands of lines of code remain that were written assuming things
> would not change underneath them in kernel mode?

Code that needs the processor number to remain fixed now uses get_cpu()
and put_cpu() which disable preemption.

> > Maybe put a comment above it like:
> 
> 
> How about one for the whole kernel?
> 
>         /**********
>          * WARNING: Use preempt at your own risk.
>          **********/

No :)

	Robert Love

Re: [PATCH] New cpu macro and i386 cleanup

[Chuck Ebbert]

Robert Love wrote:


> On a glance, it looked like at least some of them were.


Looks like do_rdmsr is not disabling preempt.  This should fix
it (compiled not tested.)


diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/arch/i386/kernel/cpuid.c linux-2.5.66-uni/arch/i386/kernel/cpuid.c
--- linux-2.5.66-ref/arch/i386/kernel/cpuid.c	Sat Mar 29 09:16:32 2003
+++ linux-2.5.66-uni/arch/i386/kernel/cpuid.c	Fri Apr  4 18:45:19 2003
@ -56,7 +56,7 @
 {
   struct cpuid_command *cmd = (struct cpuid_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
+  if ( is_current_cpu(cmd->cpu) )
     cpuid(cmd->reg, &cmd->data[0], &cmd->data[1], &cmd->data[2], &cmd->data[3]);
 }
 
@ -65,7 +65,7 @
   struct cpuid_command cmd;
   
   preempt_disable();
-  if ( cpu == smp_processor_id() ) {
+  if ( is_current_cpu(cpu) ) {
     cpuid(reg, &data[0], &data[1], &data[2], &data[3]);
   } else {
     cmd.cpu  = cpu;
diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/arch/i386/kernel/msr.c linux-2.5.66-uni/arch/i386/kernel/msr.c
--- linux-2.5.66-ref/arch/i386/kernel/msr.c	Sat Mar 29 09:16:32 2003
+++ linux-2.5.66-uni/arch/i386/kernel/msr.c	Sat Apr  5 16:18:29 2003
@ -100,7 +100,7 @
 {
   struct msr_command *cmd = (struct msr_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
+  if ( is_current_cpu(cmd->cpu) )
     cmd->err = wrmsr_eio(cmd->reg, cmd->data[0], cmd->data[1]);
 }
 
@ -108,7 +108,7 @
 {
   struct msr_command *cmd = (struct msr_command *) cmd_block;
   
-  if ( cmd->cpu == smp_processor_id() )
+  if ( is_current_cpu(cmd->cpu) )
     cmd->err = rdmsr_eio(cmd->reg, &cmd->data[0], &cmd->data[1]);
 }
 
@ -118,29 +118,31 @
   int ret;
 
   preempt_disable();
-  if ( cpu == smp_processor_id() ) {
+  if ( is_current_cpu(cpu) )
     ret = wrmsr_eio(reg, eax, edx);
-    preempt_enable();
-    return ret;
-  } else {
+  else {
     cmd.cpu = cpu;
     cmd.reg = reg;
     cmd.data[0] = eax;
     cmd.data[1] = edx;
     
     smp_call_function(msr_smp_wrmsr, &cmd, 1, 1);
-    preempt_enable();
-    return cmd.err;
+
+    ret = cmd.err;
   }
+  preempt_enable();
+  return ret;
 }
 
 static inline int do_rdmsr(int cpu, u32 reg, u32 *eax, u32 *edx)
 {
   struct msr_command cmd;
+  int ret;
 
-  if ( cpu == smp_processor_id() ) {
-    return rdmsr_eio(reg, eax, edx);
-  } else {
+  preempt_disable();
+  if ( is_current_cpu(cpu) )
+    ret = rdmsr_eio(reg, eax, edx);
+  else {
     cmd.cpu = cpu;
     cmd.reg = reg;
 
@ -148,9 +150,10 @
     
     *eax = cmd.data[0];
     *edx = cmd.data[1];
-
-    return cmd.err;
+    ret = cmd.err;
   }
+  preempt_enable();
+  return ret;
 }
 
 #else /* ! CONFIG_SMP */
diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/include/linux/smp.h linux-2.5.66-uni/include/linux/smp.h
--- linux-2.5.66-ref/include/linux/smp.h	Tue Mar  4 22:29:21 2003
+++ linux-2.5.66-uni/include/linux/smp.h	Sat Apr  5 16:33:09 2003
@ -134,6 +134,12 @
 }
 #endif /* !SMP */
 
+	/* is_current_cpu() is not preempt-safe.
+	 * Use 'cpu == get_cpu()' and then put_cpu()
+	 * if preempt not already disabled.
+	 */
+#define is_current_cpu(cpu)	((cpu) == smp_processor_id())
+
 #define get_cpu()		({ preempt_disable(); smp_processor_id(); })
 #define put_cpu()		preempt_enable()
 #define put_cpu_no_resched()	preempt_enable_no_resched()
diff -u --exclude-from=/home/me/.exclude -r linux-2.5.66-ref/include/linux/threads.h linux-2.5.66-uni/include/linux/threads.h
--- linux-2.5.66-ref/include/linux/threads.h	Tue Mar  4 22:28:55 2003
+++ linux-2.5.66-uni/include/linux/threads.h	Sat Mar 29 11:01:55 2003
@ -20,6 +20,9 @
 #define NR_CPUS		1
 #endif
 
+#define FIRST_CPU	0
+#define LAST_CPU	(NR_CPUS - 1)
+
 #define MIN_THREADS_LEFT_FOR_ROOT 4
 
 /*



--
 Chuck
 I am not a number!