Re: [PATCH] x86: don't compile vsmp_64 for 32bit

[Ravikiran G Thirumalai]

On Sun, Mar 22, 2009 at 01:48:18PM +0100, Ingo Molnar wrote:
>
>* Ravikiran G Thirumalai <kiran@xxxxxxxxxxxx> wrote:
>
>> On Sat, Feb 28, 2009 at 10:44:30AM +0100, Ingo Molnar wrote:
>> >
>> >* Ravikiran G Thirumalai <kiran@xxxxxxxxxxxx> wrote:
>> >
>> >> 
>> >> True, but by how much? 212 bytes, out of 7285943 bytes which 
>> >> is very very small percentage wise.
>> >
>> >How does this eliminate the validity of the patch?
>> >
>> 
>> It costs 212 bytes to leave is_vsmp_box() to not just be a dummy 
>> no-op. Having is_vsmp_box() detect if the hardware is indeed vSMP, 
>> is meaningful even when CONFIG_VSMP is not turned on.  This is 
>> because is_vsmp_box() is used to tell the kernel, that although 
>> the cpus being used are supposed to have TSCs in sync, they are 
>> not really in sync.  This is because you cannot ensure TSCs won't 
>> drift between multiple boards being aggregated on vSMP systems. 
>> Take the case of distro kernels.  Distro kernels typically do not 
>> have CONFIG_X86_VSMP on.  Due to the large internode cacheline 
>> setting, CONFIG_VSMP would not be on on the generic distro 
>> installer kernels. If is_vsmp_box() is a no-op, the generic distro 
>> installer kernels will assume TSCs to be synched, which is bad.  
>> Hence, it will be nice if, for the cost of 212 bytes, vsmp64.o be 
>> compiled either unconditionally, OR conditionally for 64bit 
>> architectures only.  The question is, is 212 bytes out of 7285943 
>> bytes too expensive for the generic kernels?  I hope not.
>
>Sorry - got distracted and forgot about this thread. The TSC quirk 
>indeed looks required for your systems - you dont have a reliable 
>TSC due to virtualization, right?
>

Yes. Also, because  there is no way to avoid tsc drift on
multiple boards/nodes.


>Mind sending a patch (partial revert or so) against latest -tip that 
>fixes that?
>

Sure.  Here's  a revert, it is a partial revert which compiles vsmp64.c only
for 64bit architectures.

Thanks,
Kiran


Partial revert of commit 129d8bc828e011bda0b7110a097bf3a0167f966e
titled 'x86: don't compile vsmp_64 for 32bit'
Commit reverted to compile vsmp_64.c if CONFIG_X86_64 is defined,
since is_vsmp_box() needs to indicate that TSCs are not synchronized, and
hence, not a valid time source, even when CONFIG_X86_VSMP is not defined.

Signed-off-by: Ravikiran Thirumalai <kiran@xxxxxxxxxxxx>

Index: git.tip/arch/x86/include/asm/apic.h
===================================================================
--- git.tip.orig/arch/x86/include/asm/apic.h	2009-03-23 12:50:37.000000000 -0800
+++ git.tip/arch/x86/include/asm/apic.h	2009-03-23 20:21:02.000000000 -0800
@@ -75,7 +75,7 @@ static inline void default_inquire_remot
 #define setup_secondary_clock setup_secondary_APIC_clock
 #endif
 
-#ifdef CONFIG_X86_VSMP
+#ifdef CONFIG_X86_64
 extern int is_vsmp_box(void);
 #else
 static inline int is_vsmp_box(void)
Index: git.tip/arch/x86/include/asm/setup.h
===================================================================
--- git.tip.orig/arch/x86/include/asm/setup.h	2009-03-23 12:50:37.000000000 -0800
+++ git.tip/arch/x86/include/asm/setup.h	2009-03-23 20:19:18.000000000 -0800
@@ -64,7 +64,7 @@ extern void x86_quirk_time_init(void);
 #include <asm/bootparam.h>
 
 /* Interrupt control for vSMPowered x86_64 systems */
-#ifdef CONFIG_X86_VSMP
+#ifdef CONFIG_X86_64
 void vsmp_init(void);
 #else
 static inline void vsmp_init(void) { }
Index: git.tip/arch/x86/kernel/Makefile
===================================================================
--- git.tip.orig/arch/x86/kernel/Makefile	2009-03-23 12:50:37.000000000 -0800
+++ git.tip/arch/x86/kernel/Makefile	2009-03-23 20:29:34.000000000 -0800
@@ -71,7 +71,6 @@ obj-$(CONFIG_FTRACE_SYSCALLS)	+= ftrace.
 obj-$(CONFIG_KEXEC)		+= machine_kexec_$(BITS).o
 obj-$(CONFIG_KEXEC)		+= relocate_kernel_$(BITS).o crash.o
 obj-$(CONFIG_CRASH_DUMP)	+= crash_dump_$(BITS).o
-obj-$(CONFIG_X86_VSMP)		+= vsmp_64.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
 obj-$(CONFIG_MODULES)		+= module_$(BITS).o
 obj-$(CONFIG_EFI) 		+= efi.o efi_$(BITS).o efi_stub_$(BITS).o
@@ -121,4 +120,5 @@ ifeq ($(CONFIG_X86_64),y)
 	obj-$(CONFIG_AMD_IOMMU)		+= amd_iommu_init.o amd_iommu.o
 
 	obj-$(CONFIG_PCI_MMCONFIG)	+= mmconf-fam10h_64.o
+	obj-y				+= vsmp_64.o
 endif
Index: git.tip/arch/x86/kernel/vsmp_64.c
===================================================================
--- git.tip.orig/arch/x86/kernel/vsmp_64.c	2009-03-23 12:50:37.000000000 -0800
+++ git.tip/arch/x86/kernel/vsmp_64.c	2009-03-23 21:09:34.000000000 -0800
@@ -22,7 +22,7 @@
 #include <asm/paravirt.h>
 #include <asm/setup.h>
 
-#ifdef CONFIG_PARAVIRT
+#if defined CONFIG_PCI && defined CONFIG_PARAVIRT
 /*
  * Interrupt control on vSMPowered systems:
  * ~AC is a shadow of IF.  If IF is 'on' AC should be 'off'
@@ -114,6 +114,7 @@ static void __init set_vsmp_pv_ops(void)
 }
 #endif
 
+#ifdef CONFIG_PCI
 static int is_vsmp = -1;
 
 static void __init detect_vsmp_box(void)
@@ -139,6 +140,15 @@ int is_vsmp_box(void)
 	}
 }
 
+#else
+static void __init detect_vsmp_box(void)
+{
+}
+int is_vsmp_box(void)
+{
+	return 0;
+}
+#endif
 void __init vsmp_init(void)
 {
 	detect_vsmp_box();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/