Re: [PATCH] i386 no idle hz - aka dynticks v051118-1

[Zwane Mwaikambo]

It may be a little hard to see my comments as i've re-attached the patch 
inline as i couldn't reply-to properly (it was an attachment).

@@ -932,6 +933,9 @@ void (*wait_timer_tick)(void) __devinitd
 
 #define APIC_DIVISOR 16
 
+static u32 apic_timer_val;

__read_mostly ?

+void dyn_tick_interrupt(int irq, struct pt_regs *regs)
+{
+	int all_were_sleeping = 0;
+	int cpu = smp_processor_id();
+
+	if (!cpu_isset(cpu, nohz_cpu_mask))
+		return;
+
+	spin_lock(dyn_tick_lock);

This is going to cause contention problems for things like 
smp_call_function since all processors will be calling back to 
dyn_tick_interrupt.

+	if (cpus_equal(nohz_cpu_mask, cpu_online_map))
+		all_were_sleeping = 1;
+	cpu_clear(cpu, nohz_cpu_mask);
+
+	if (all_were_sleeping) {
+		/* Recover jiffies */
+		if (irq) {

Perhaps simply call the 'irq' parameter as in_irq as right now it seems to 
mean anything between irq or vector and somewhat confusing.

@@ -1190,15 +1191,19 @@ static inline void ioapic_register_intr(
 		if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
 				trigger == IOAPIC_LEVEL)
 			irq_desc[vector].handler = &ioapic_level_type;
-		else
+		else if (vector)
 			irq_desc[vector].handler = &ioapic_edge_type;
+		else
+			irq_desc[vector].handler = IOAPIC_EDGE_TYPE_IRQ0;

Please at least be explicit and not hide things behind #defines

if (vector == 0)
	irq_desc[vector].handler = &ioapic_edge_type_irq0

 DEFINE_PER_CPU(irq_cpustat_t, irq_stat) ____cacheline_maxaligned_in_smp;
 EXPORT_PER_CPU_SYMBOL(irq_stat);
@@ -76,6 +77,8 @@ fastcall unsigned int do_IRQ(struct pt_r
 	}
 #endif
 
+	dyn_tick_interrupt(irq, regs);

This looks like it might contribute quite some contention in the irq fast 
path.

Thanks,
	Zwane

-
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/