Skip to contentSkip to conversation

Disable TSCs on CONFIG_MULTIQUAD

RFC PATCH In review john stultz 3 participants 7 messages May 29, 2002 – May 30, 2002

Subsystems (2)
Networking Traffic control (tc)
lore
Share & tools
lore

john stultz

author
#0 lore raw

RFC PATCH 1/1 Disable TSCs on CONFIG_MULTIQUAD

Hi all,
Just wanted to submit this for comments. As we've been having trouble
keeping the TSCs on NUMA hardware synced (resulting in gettimeofday occasionally going backward), this patch (against 2.4.18) disables TSCs if CONFIG_MULTIQUAD is enabled. Any suggestions for simplifying the changes to config.in would be appreciated.

Flames/etc also welcome.
-john

Index: linux24/arch/i386/config.in

arch/i386/config.in renamed from arch/i386/config.in:1.1.1.3 +21 -11
diff -u linux24/arch/i386/config.in:1.1.1.3 linux24/arch/i386/config.in:1.1.1.3.14.2
@@ -80,20 +80,20 @@
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
    define_bool CONFIG_X86_USE_STRING_486 y
    define_bool CONFIG_X86_ALIGNMENT_16 y
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_PPRO_FENCE y
 fi
 if [ "$CONFIG_M586MMX" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
    define_bool CONFIG_X86_USE_STRING_486 y
    define_bool CONFIG_X86_ALIGNMENT_16 y
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_GOOD_APIC y
    define_bool CONFIG_X86_PPRO_FENCE y
 fi
 if [ "$CONFIG_M686" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_GOOD_APIC y
    define_bool CONFIG_X86_PGE y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y

@@ -101,14 +101,14 @@
 fi
 if [ "$CONFIG_MPENTIUMIII" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_GOOD_APIC y
    define_bool CONFIG_X86_PGE y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y
 fi
 if [ "$CONFIG_MPENTIUM4" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 7
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_GOOD_APIC y
    define_bool CONFIG_X86_PGE y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y

@@ -116,12 +116,12 @@
 if [ "$CONFIG_MK6" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
    define_bool CONFIG_X86_ALIGNMENT_16 y
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y
 fi
 if [ "$CONFIG_MK7" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 6
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_GOOD_APIC y
    define_bool CONFIG_X86_USE_3DNOW y
    define_bool CONFIG_X86_PGE y

@@ -134,14 +134,14 @@
 fi
 if [ "$CONFIG_MCYRIXIII" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_ALIGNMENT_16 y
    define_bool CONFIG_X86_USE_3DNOW y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y
 fi
 if [ "$CONFIG_MCRUSOE" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
 fi
 if [ "$CONFIG_MWINCHIPC6" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5

@@ -152,14 +152,14 @@
 if [ "$CONFIG_MWINCHIP2" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
    define_bool CONFIG_X86_ALIGNMENT_16 y
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y
    define_bool CONFIG_X86_OOSTORE y
 fi
 if [ "$CONFIG_MWINCHIP3D" = "y" ]; then
    define_int  CONFIG_X86_L1_CACHE_SHIFT 5
    define_bool CONFIG_X86_ALIGNMENT_16 y
-   define_bool CONFIG_X86_TSC y
+   define_bool CONFIG_X86_HAS_TSC y
    define_bool CONFIG_X86_USE_PPRO_CHECKSUM y
    define_bool CONFIG_X86_OOSTORE y
 fi

@@ -196,6 +196,16 @@
    fi
 else
    bool 'Multiquad NUMA system' CONFIG_MULTIQUAD
+fi
+
+if [ "$CONFIG_X86_HAS_TSC" = "y" ]; then
+	if [ "$CONFIG_MULTIQUAD" = "y" ]; then
+		define_bool CONFIG_X86_TSC n
+	else
+		define_bool CONFIG_X86_TSC y
+	fi
+else
+	define_bool CONFIG_X86_TSC n
 fi
 
 if [ "$CONFIG_SMP" = "y" -a "$CONFIG_X86_CMPXCHG" = "y" ]; then

Index: linux24/arch/i386/kernel/setup.c

arch/i386/kernel/setup.c renamed from arch/i386/kernel/setup.c:1.1.1.3.14.1 +4 -0
diff -u linux24/arch/i386/kernel/setup.c:1.1.1.3.14.1 linux24/arch/i386/kernel/setup.c:1.1.1.3.14.3
@@ -1054,7 +1054,11 @@
 
 
 #ifndef CONFIG_X86_TSC
+#ifdef CONFIG_MULTIQUAD /*MULTIQUAD & TSCs don't play well*/
+static int tsc_disable __initdata = 1;	
+#else /*CONFIG_MULTIQUAD*/
 static int tsc_disable __initdata = 0;
+#endif /*CONFIG_MULTIQUAD*/
 
 static int __init tsc_setup(char *str)
 {
#1 lore raw
Quoted 6 lines · john stultz

Hi all,
Just wanted to submit this for comments. As we've been having trouble
keeping the TSCs on NUMA hardware synced (resulting in gettimeofday occasionally going backward), this patch (against 2.4.18) disables TSCs if CONFIG_MULTIQUAD is enabled. Any suggestions for simplifying the changes to config.in would be appreciated.

Add CONFIG_X86_TSC_NOT_SYNCHRONOUS or similar and then check

#if defined(CONFIG_X86_TSC) && !defined(CONFIG_X86_NOT_SYNCHRONOUS))

??

author
#2 lore raw
Quoted 3 lines

Add CONFIG_X86_TSC_NOT_SYNCHRONOUS or similar and then check

#if defined(CONFIG_X86_TSC) && !defined(CONFIG_X86_NOT_SYNCHRONOUS))

<laugh> I guess I asked for it. :) I actually implemented something similar to the above earlier, and was convinced out of it. The trade off with this method is that while it simplifies the config.in changes, it complicates the 12 or so #ifdef CONFIG_X86_TSC lines.

Keeping the "multi-quad TSCs are broken" fact in just one place (config.in), rather then spread about the tree in precompiler statements, seems like the best choice to me. I was actually hoping there was a good config.in trick for turning CONFIG_X86_TSC off after it had already been turned on, rather then adding a meta-option, but I'm guessing there isn't.

Anyway, if you really would rather see what you suggested, I'll happily change it (I do like the idea of breaking the CONFIG_X86_TSC_UNSYNCED notion out of CONFIG_MULTIQUAD).

Thanks for the feedback!
-john

#3 lore raw
Quoted 3 lines · john stultz

Anyway, if you really would rather see what you suggested, I'll happily change it (I do like the idea of breaking the CONFIG_X86_TSC_UNSYNCED notion out of CONFIG_MULTIQUAD).

Not all the other places are "there is no TSC" most of them deal with the ability to use a TSC. There are other setups where TSC exists but isnt usable so distinguishing matters

Also there is one case where TSC that doesn't work matters specifically
- you want to turn off RDTSC access from user space to avoid user space tools having little accidents

#4 lore raw
Quoted 7 lines
Quoted 3 lines

Anyway, if you really would rather see what you suggested, I'll happily change it (I do like the idea of breaking the CONFIG_X86_TSC_UNSYNCED notion out of CONFIG_MULTIQUAD).

Not all the other places are "there is no TSC" most of them deal with the ability to use a TSC. There are other setups where TSC exists but isnt usable so distinguishing matters

I think the CONFIG_X86_TSC option is most confusing (bad naming, at best).

Without CONFIG_X86_TSC:
You get the ability to have a TSC or not, both code paths are compiled in, and it dynamically detects at boot time. You can override this with the "notsc" option, or overriding the tsc_disable variable, as we did here.

With CONFIG_X86_TSC:
You remove all the code which supports non-TSC systems.

Perhaps I'm just mentally slow, but I think my little brain would find this area easier of it was called CONFIG_X86_ONLY_TSC or some such.

So if John's patch was rewritten to leave the CPU type switching on CONFIG_X86_TSC, then have the multiquad switch turn that into CONFIG_X86_ONLY_TSC (and change the in code #ifdefs to that) would that be more palletable? Would make things more readable in the main code to my mind ....

M.

author
#5 lore raw
In reply to Alan Cox
Quoted 3 lines

Not all the other places are "there is no TSC" most of them deal with the ability to use a TSC. There are other setups where TSC exists but isnt usable so distinguishing matters

Martin's clarification aside, I'll agree that distinguishing between no TSC and unsynced TSCs is reasonable. There might be kernel code paths that cannot migrate between cpus, use the TSC for timing, and do not save the value in global structures. So in that case, reading the non synced TSC would be safe and disabling TSCs totally w/ multiquad might be a touch rash. I just haven't actually looked to see if such cases exist. Are there other reasons unsynced TSCs might still be useful?

Quoted 3 lines

Also there is one case where TSC that doesn't work matters specifically
- you want to turn off RDTSC access from user space to avoid user space tools having little accidents

I was hoping that would fall into place when the X86_FEATURE_TSC bit of boot_cpu_data.x86_capability (aka: cpu_has_tsc) was disabled. I'll double check however, just to be sure.

Thanks again.
-john

author
#6 lore raw
In reply to Alan Cox
Quoted 3 lines

Add CONFIG_X86_TSC_NOT_SYNCHRONOUS or similar and then check

#if defined(CONFIG_X86_TSC) && !defined(CONFIG_X86_NOT_SYNCHRONOUS))

Attached is the patch implemented as suggested above. It seems a bit uglier, but perhaps I'm missing exactly what you meant.

thanks
-john

Index: linux24/arch/i386/config.in

arch/i386/config.in renamed from arch/i386/config.in:1.1.1.3 +5 -0
diff -u linux24/arch/i386/config.in:1.1.1.3 linux24/arch/i386/config.in:1.1.1.3.14.4
@@ -198,6 +198,11 @@
    bool 'Multiquad NUMA system' CONFIG_MULTIQUAD
 fi
 
+# Multiquad NUMA boxes can't keep their TSCs in sync
+if [ "$CONFIG_MULTIQUAD" = "y" ]; then
+		define_bool CONFIG_TSC_DISABLE y
+fi
+
 if [ "$CONFIG_SMP" = "y" -a "$CONFIG_X86_CMPXCHG" = "y" ]; then
    define_bool CONFIG_HAVE_DEC_LOCK y
 fi

Index: linux24/arch/i386/kernel/setup.c

arch/i386/kernel/setup.c renamed from arch/i386/kernel/setup.c:1.1.1.3.14.1 +7 -3
diff -u linux24/arch/i386/kernel/setup.c:1.1.1.3.14.1 linux24/arch/i386/kernel/setup.c:1.1.1.3.14.6
@@ -1053,8 +1053,12 @@
 __setup("cachesize=", cachesize_setup);
 
 
-#ifndef CONFIG_X86_TSC
+#if !defined(CONFIG_X86_TSC)||defined(CONFIG_TSC_DISABLE)
+#ifdef CONFIG_TSC_DISABLE
+static int tsc_disable __initdata = 1;	
+#else /*CONFIG_TSC_DISABLE*/
 static int tsc_disable __initdata = 0;
+#endif /*CONFIG_TSC_DISABLE*/
 
 static int __init tsc_setup(char *str)
 {

@@ -2650,7 +2654,7 @@
 	 */
 
 	/* TSC disabled? */
-#ifndef CONFIG_X86_TSC
+#if !defined(CONFIG_X86_TSC)||defined(CONFIG_TSC_DISABLE)
 	if ( tsc_disable )
 		clear_bit(X86_FEATURE_TSC, &c->x86_capability);
 #endif

@@ -2888,7 +2892,7 @@
 
 	if (cpu_has_vme || cpu_has_tsc || cpu_has_de)
 		clear_in_cr4(X86_CR4_VME|X86_CR4_PVI|X86_CR4_TSD|X86_CR4_DE);
-#ifndef CONFIG_X86_TSC
+#if !defined(CONFIG_X86_TSC)||defined(CONFIG_TSC_DISABLE)
 	if (tsc_disable && cpu_has_tsc) {
 		printk(KERN_NOTICE "Disabling TSC...\n");
 		/**** FIX-HPA: DOES THIS REALLY BELONG HERE? ****/

Index: linux24/arch/i386/kernel/time.c

arch/i386/kernel/time.c renamed from arch/i386/kernel/time.c:1.1.1.2 +1 -1
diff -u linux24/arch/i386/kernel/time.c:1.1.1.2 linux24/arch/i386/kernel/time.c:1.1.1.2.14.2
@@ -118,7 +118,7 @@
 
 extern spinlock_t i8259A_lock;
 
-#ifndef CONFIG_X86_TSC
+#if !defined(CONFIG_X86_TSC)||defined(CONFIG_TSC_DISABLE)
 
 /* This function must be called with interrupts disabled 
  * It was inspired by Steve McCanne's microtime-i386 for BSD.  -- jrs

Index: linux24/include/asm-i386/bugs.h

include/asm-i386/bugs.h renamed from include/asm-i386/bugs.h:1.1.1.1 +1 -1
diff -u linux24/include/asm-i386/bugs.h:1.1.1.1 linux24/include/asm-i386/bugs.h:1.1.1.1.40.2
@@ -173,7 +173,7 @@
 /*
  * If we configured ourselves for a TSC, we'd better have one!
  */
-#ifdef CONFIG_X86_TSC
+#if defined(CONFIG_X86_TSC)&&!defined(CONFIG_TSC_DISABLE)
 	if (!cpu_has_tsc)
 		panic("Kernel compiled for Pentium+, requires TSC feature!");
 #endif

Index: linux24/include/asm-i386/timex.h

include/asm-i386/timex.h renamed from include/asm-i386/timex.h:1.1.1.2 +1 -1
diff -u linux24/include/asm-i386/timex.h:1.1.1.2 linux24/include/asm-i386/timex.h:1.1.1.2.14.2
@@ -40,7 +40,7 @@
 
 static inline cycles_t get_cycles (void)
 {
-#ifndef CONFIG_X86_TSC
+#if !defined(CONFIG_X86_TSC) || defined(CONFIG_TSC_DISABLE)
 	return 0;
 #else
 	unsigned long long ret;

Index: linux24/include/net/pkt_sched.h

include/net/pkt_sched.h renamed from include/net/pkt_sched.h:1.1.1.1 +2 -2
diff -u linux24/include/net/pkt_sched.h:1.1.1.1 linux24/include/net/pkt_sched.h:1.1.1.1.40.2
@@ -11,7 +11,7 @@
 #include <linux/pkt_sched.h>
 #include <net/pkt_cls.h>
 
-#ifdef CONFIG_X86_TSC
+#if defined(CONFIG_X86_TSC)&&!defined(CONFIG_TSC_DISABLE)
 #include <asm/msr.h>
 #endif
 

@@ -252,7 +252,7 @@
 
 #define PSCHED_US2JIFFIE(delay) (((delay)+psched_clock_per_hz-1)/psched_clock_per_hz)
 
-#ifdef CONFIG_X86_TSC
+#if defined(CONFIG_X86_TSC)&&!defined(CONFIG_TSC_DISABLE)
 
 #define PSCHED_GET_TIME(stamp) \
 ({ u64 __cur; \

Index: linux24/include/net/profile.h

include/net/profile.h renamed from include/net/profile.h:1.1.1.1 +2 -2
diff -u linux24/include/net/profile.h:1.1.1.1 linux24/include/net/profile.h:1.1.1.1.40.2
@@ -9,7 +9,7 @@
 #include <linux/kernel.h>
 #include <asm/system.h>
 
-#ifdef CONFIG_X86_TSC
+#if defined(CONFIG_X86_TSC) && !defined(CONFIG_TSC_DISABLE)
 #include <asm/msr.h>
 #endif
 

@@ -29,7 +29,7 @@
 extern struct timeval net_profile_adjust;
 extern void net_profile_irq_adjust(struct timeval *entered, struct timeval* leaved);
 
-#ifdef CONFIG_X86_TSC
+#if defined(CONFIG_X86_TSC)&&!defined(CONFIG_TSC_DISABLE)
 
 static inline void  net_profile_stamp(struct timeval *pstamp)
 {