Re: The machdep.hyperthreading_allowed & ULE weirdness in 7.1

Robert Watson wrote:
>> So, are you suggesting that we should disable 
>> machdep.hyperthreading_allowed with ULE in 7.x and current to avoid 
>> confusion?
> 
> Possibly even without ULE.

I've verified - the tunable/sysctl works just fine with SCHED_4BSD in 
7.1, so that I am not sure it's worth ripping it off.

Instead, I've re-implemented the feature differently - by disabling HT 
cores at detection time when SCHED_ULE is compiled in and 
machdep.hyperthreading_allowed is set to 0 in loader.conf. Patch for 7.1 
is attached.  Apart from the fact that it's not possible to change the 
setting at run-time, this version should be even better by not starting 
up HT cores in the first place.

I would appreciate if somebody familiar with the code in question can 
review the patch, particularly part that moves assign_cpu_ids() and 
start_all_aps() calls little bit further in the cpu_mp_start(). I need 
them there so that I can use hyperthreading_cpus in assign_cpu_ids().

Thanks in advance.

-Maxim

--- i386/i386/mp_machdep.c	2009-01-19 07:34:49.000000000 -0800
+++ /tmp/mp_machdep.c	2009-02-23 21:38:18.000000000 -0800
_at__at_ -209,6 +210,7 _at__at_
 	int	cpu_present:1;
 	int	cpu_bsp:1;
 	int	cpu_disabled:1;
+	int	cpu_hyperthread:1;
 } static cpu_info[MAX_APIC_ID + 1];
 int cpu_apic_ids[MAXCPU];
 
_at__at_ -405,11 +407,6 _at__at_
 		    ("BSP's APIC ID doesn't match boot_cpu_id"));
 	cpu_apic_ids[0] = boot_cpu_id;
 
-	assign_cpu_ids();
-
-	/* Start each Application Processor */
-	start_all_aps();
-
 	/* Setup the initial logical CPUs info. */
 	logical_cpus = logical_cpus_mask = 0;
 	if (cpu_feature & CPUID_HTT)
_at__at_ -457,6 +454,11 _at__at_
 			hyperthreading_cpus = logical_cpus;
 	}
 
+	assign_cpu_ids();
+
+	/* Start each Application Processor */
+	start_all_aps();
+
 	set_interrupt_apic_ids();
 
 	/* Last, setup the cpu topology now that we have probed CPUs */
_at__at_ -471,18 +473,26 _at__at_
 cpu_mp_announce(void)
 {
 	int i, x;
+	const char *hyperthread;
 
 	/* List CPUs */
 	printf(" cpu0 (BSP): APIC ID: %2d\n", boot_cpu_id);
 	for (i = 1, x = 0; x <= MAX_APIC_ID; x++) {
 		if (!cpu_info[x].cpu_present || cpu_info[x].cpu_bsp)
 			continue;
+		if (cpu_info[x].cpu_hyperthread) {
+			hyperthread = "/HT";
+		} else {
+			hyperthread = "";
+		}
 		if (cpu_info[x].cpu_disabled)
-			printf("  cpu (AP): APIC ID: %2d (disabled)\n", x);
+			printf("  cpu (AP%s): APIC ID: %2d (disabled)\n",
+			    hyperthread, x);
 		else {
 			KASSERT(i < mp_ncpus,
 			    ("mp_ncpus and actual cpus are out of whack"));
-			printf(" cpu%d (AP): APIC ID: %2d\n", i++, x);
+			printf(" cpu%d (AP%s): APIC ID: %2d\n", i++,
+			    hyperthread, x);
 		}
 	}
 }
_at__at_ -691,11 +701,24 _at__at_
 {
 	u_int i;
 
+	TUNABLE_INT_FETCH("machdep.hyperthreading_allowed",
+	    &hyperthreading_allowed);
+
 	/* Check for explicitly disabled CPUs. */
 	for (i = 0; i <= MAX_APIC_ID; i++) {
 		if (!cpu_info[i].cpu_present || cpu_info[i].cpu_bsp)
 			continue;
 
+		if (hyperthreading_cpus > 1 && i % hyperthreading_cpus != 0) {
+			cpu_info[i].cpu_hyperthread = 1;
+#if defined(SCHED_ULE)
+			if (hyperthreading_allowed == 0) {
+				cpu_info[i].cpu_disabled = 1;
+				continue;
+			}
+#endif
+		}
+
 		/* Don't use this CPU if it has been disabled by a tunable. */
 		if (resource_disabled("lapic", i)) {
 			cpu_info[i].cpu_disabled = 1;
_at__at_ -1410,6 +1433,12 _at__at_
 	if (error || !req->newptr)
 		return (error);
 
+#ifdef SCHED_ULE
+	if (allowed != hyperthreading_allowed)
+		return (ENOTSUP);
+	return (error);
+#endif
+
 	if (allowed)
 		hlt_cpus_mask &= ~hyperthreading_cpus_mask;
 	else
_at__at_ -1454,8 +1483,6 _at__at_
 		 * of hlt_logical_cpus.
 		 */
 		if (hyperthreading_cpus_mask) {
-			TUNABLE_INT_FETCH("machdep.hyperthreading_allowed",
-			    &hyperthreading_allowed);
 			SYSCTL_ADD_PROC(&logical_cpu_clist,
 			    SYSCTL_STATIC_CHILDREN(_machdep), OID_AUTO,
 			    "hyperthreading_allowed", CTLTYPE_INT|CTLFLAG_RW,