[patch] irqlock patch 2.5.27-H4

[patch] irqlock patch 2.5.27-H4

[Ingo Molnar]

the latest irqlock patch can be found at:

   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-H4

Changes in -H4:

 - fix the cli()/sti() hack in ide/main.c, per Marcin Dalecki's
   suggestion. [this leaves the tty layer as the only remaining subsystem
   that still has cli()/sti() related hacks.]

Changes in -H3:

 - init thread needs to have preempt_count of 1 until sched_init(). 
   (William Lee Irwin III)

 - clean up the irq-mask macros. (Linus)

 - add barrier() to irq_enter() and irq_exit(). (based on Oleg Nesterov's
   comment.)

 - move the irqs-off check into preempt_schedule() and remove
   CONFIG_DEBUG_IRQ_SCHEDULE.

Changes in -G5:

 - remove spin_unlock_no_resched() and comment the affected places more
   agressively.

Changes in -G3:

 - slab.c needs to spin_unlock_no_resched(), instead of spin_unlock(). (It
   also has to check for preemption in the right spot.) This should fix
   the memory corruption.

 - irq_exit() needs to run softirqs if interrupts not active - in the 
   previous patch it ran them when preempt_count() was 0, which is
   incorrect.

 - spinlock macros are updated to enable preemption after enabling 
   interrupts. Besides avoiding false positive warnings, this also 

 - fork.c has to call scheduler_tick() with preemption disabled - 
   otherwise scheduler_tick()'s spin_unlock can preempt!

 - irqs_disabled() macro introduced.

 - [ all other local_irq_enable() or sti instances conditional on
     CONFIG_DEBUG_IRQ_SCHEDULE are to fix false positive warnings. ]

Changes in -G0:

 - fix buggy in_softirq(). Fortunately the bug made the test broader,
   which didnt result in algorithmical breakage, just suboptimal
   performance.

 - move do_softirq() processing into irq_exit() => this also fixes the
   softirq processing bugs present in apic.c IRQ handlers that did not
   test for softirqs after irq_exit().

 - simplify local_bh_enable().

Changes in -F9:

 - replace all instances of:

	local_save_flags(flags);
	local_irq_disable();

   with the shorter form of:

	local_irq_save(flags);

  about 30 files are affected by this change.

Changes in -F8:

 - preempt/hardirq/softirq count separation, cleanups.

 - skbuff.c fix.

 - use irq_count() in scheduler_tick()

Changes in -F3:

 - the entry.S cleanups/speedups by Oleg Nesterov.

 - a rather critical synchronize_irq() bugfix: if a driver frees an 
   interrupt that is still being probed then synchronize_irq() locks up.
   This bug has caused a spurious boot-lockup on one of my testsystems,
   ifconfig would lock up trying to close eth0.

 - remove duplicate definitions from asm-i386/system.h, this fixes 
   compiler warnings.

	Ingo

Re: [patch] irqlock patch 2.5.27-H4

[Christoph Hellwig]

>  - move the irqs-off check into preempt_schedule() and remove
>    CONFIG_DEBUG_IRQ_SCHEDULE.

the config.in entry is still present..

Re: [patch] irqlock patch 2.5.27-H4

[Ingo Molnar]

On Wed, 24 Jul 2002, Christoph Hellwig wrote:

> >  - move the irqs-off check into preempt_schedule() and remove
> >    CONFIG_DEBUG_IRQ_SCHEDULE.
> 
> the config.in entry is still present..

indeed. Fix is in -H5:

   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-H5

	Ingo

Re: [patch] irqlock patch 2.5.27-H4

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Ingo Molnar wrote:

> the latest irqlock patch can be found at:
>
>    http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-H4
>
> Changes in -H4:
>
>  - fix the cli()/sti() hack in ide/main.c, per Marcin Dalecki's
>    suggestion. [this leaves the tty layer as the only remaining subsystem
>    that still has cli()/sti() related hacks.]

Hi Ingo,

Marcin's suggestions will bring you nowhere.

You should remove all these locking from ide_unregister_subdriver()
because in 100% cases it is already called with ide_lock held.

Also in ide subsystem ide-tape.c needs fixing, however it is already
broken and proper locking fixing may be non-trivial task.

Regards
--
Bartlomiej

Re: [patch] irqlock patch 2.5.27-H4

[Marcin Dalecki]

Bartlomiej Zolnierkiewicz wrote:
> On Wed, 24 Jul 2002, Ingo Molnar wrote:
> 
> 
>>the latest irqlock patch can be found at:
>>
>>   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-H4
>>
>>Changes in -H4:
>>
>> - fix the cli()/sti() hack in ide/main.c, per Marcin Dalecki's
>>   suggestion. [this leaves the tty layer as the only remaining subsystem
>>   that still has cli()/sti() related hacks.]
> 
> 
> Hi Ingo,
> 
> Marcin's suggestions will bring you nowhere.
> 
> You should remove all these locking from ide_unregister_subdriver()
> because in 100% cases it is already called with ide_lock held.

Indeed they can be just removed.

Re: [patch] irqlock patch 2.5.27-H4

[Bartlomiej Zolnierkiewicz]

On Wed, 24 Jul 2002, Marcin Dalecki wrote:

> Bartlomiej Zolnierkiewicz wrote:
> > On Wed, 24 Jul 2002, Ingo Molnar wrote:
> >
> >
> >>the latest irqlock patch can be found at:
> >>
> >>   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-H4
> >>
> >>Changes in -H4:
> >>
> >> - fix the cli()/sti() hack in ide/main.c, per Marcin Dalecki's
> >>   suggestion. [this leaves the tty layer as the only remaining subsystem
> >>   that still has cli()/sti() related hacks.]
> >
> >
> > Hi Ingo,
> >
> > Marcin's suggestions will bring you nowhere.
> >
> > You should remove all these locking from ide_unregister_subdriver()
> > because in 100% cases it is already called with ide_lock held.
>
> Indeed they can be just removed.

[bugging mode still on ;-)))]

No, they _have to_ be removed, you introduced bug with your fix.

Regards
--
Bartlomeij

Re: [patch] irqlock patch 2.5.27-H4

[george anzinger]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

Ingo Molnar wrote:
> 
> On Wed, 24 Jul 2002, Christoph Hellwig wrote:
> 
> > >  - move the irqs-off check into preempt_schedule() and remove
> > >    CONFIG_DEBUG_IRQ_SCHEDULE.
> >
> > the config.in entry is still present..
> 
> indeed. Fix is in -H5:

We need to verify correct usage of the
irq_enable/irq_restore() least we miss a preemption.  The
attached patch allows one to enable a test on the these
macros to verify that they are being correctly used.

Ingo, I am not sure about the change in fork.c.  Don't we
always want to check for preemption at the end of this
irq_disable?

Patch is against Ingo's H6.
-- 
George Anzinger   george@mvista.com
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Real time sched:  http://sourceforge.net/projects/rtsched/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

[-- Attachment #2: irq-test-2.5.27-ingo-H6.patch --]
[-- Type: text/plain, Size: 6154 bytes --]

diff -urP -I \$Id:.*Exp \$ -X /usr/src/patch.exclude linux-2.5.27-ingo-H6-org/arch/i386/config.in linux/arch/i386/config.in
--- linux-2.5.27-ingo-H6-org/arch/i386/config.in	Mon Jul 22 14:19:20 2002
+++ linux/arch/i386/config.in	Wed Jul 24 12:13:56 2002
@@ -412,6 +412,7 @@
 
 bool 'Kernel debugging' CONFIG_DEBUG_KERNEL
 if [ "$CONFIG_DEBUG_KERNEL" != "n" ]; then
+   bool '  Debug IRQ preempt interaction' CONFIG_DEBUG_IRQ
    bool '  Debug memory allocations' CONFIG_DEBUG_SLAB
    bool '  Memory mapped I/O debugging' CONFIG_DEBUG_IOVIRT
    bool '  Magic SysRq key' CONFIG_MAGIC_SYSRQ
diff -urP -I \$Id:.*Exp \$ -X /usr/src/patch.exclude linux-2.5.27-ingo-H6-org/include/asm-i386/system.h linux/include/asm-i386/system.h
--- linux-2.5.27-ingo-H6-org/include/asm-i386/system.h	Wed Jul 24 12:50:03 2002
+++ linux/include/asm-i386/system.h	Wed Jul 24 12:36:08 2002
@@ -312,9 +312,26 @@
 
 /* interrupt control.. */
 #define local_save_flags(x)	__asm__ __volatile__("pushfl ; popl %0":"=g" (x): /* no input */)
-#define local_irq_restore(x) 	__asm__ __volatile__("pushl %0 ; popfl": /* no output */ :"g" (x):"memory", "cc")
+#define _local_irq_restore(x) 	__asm__ __volatile__("pushl %0 ; popfl": /* no output */ :"g" (x):"memory", "cc")
 #define local_irq_disable() 	__asm__ __volatile__("cli": : :"memory")
-#define local_irq_enable()	__asm__ __volatile__("sti": : :"memory")
+#define _local_irq_enable()	__asm__ __volatile__("sti": : :"memory")
+#ifdef CONFIG_DEBUG_IRQ
+#define test_preempt() if (unlikely(! current_thread_info()->preempt_count)){ \
+		printk("bad: irq_restore() with preemption_enabled!\n");\
+		show_stack(NULL); } 
+#else
+#define test_preempt()
+#endif
+#define local_irq_enable() \
+                do { _local_irq_enable(); test_preempt(); } while(0)
+#define local_irq_restore(x) \
+                do { _local_irq_restore(x); test_preempt(); } while(0)
+#define local_irq_enable_preempt()  \
+                do { _local_irq_enable(); preempt_check_resched() ; } while(0)
+#define local_irq_restore_preempt(x) \
+                do { _local_irq_restore(x);  preempt_check_resched(); } while(0)
+
+
 /* used in the idle loop; sti takes one instruction cycle to complete */
 #define safe_halt()		__asm__ __volatile__("sti; hlt": : :"memory")
 
diff -urP -I \$Id:.*Exp \$ -X /usr/src/patch.exclude linux-2.5.27-ingo-H6-org/include/linux/spinlock.h linux/include/linux/spinlock.h
--- linux-2.5.27-ingo-H6-org/include/linux/spinlock.h	Wed Jul 24 12:50:03 2002
+++ linux/include/linux/spinlock.h	Wed Jul 24 12:39:39 2002
@@ -26,17 +26,17 @@
 #define write_lock_irq(lock)			do { local_irq_disable();        write_lock(lock); } while (0)
 #define write_lock_bh(lock)			do { local_bh_disable();         write_lock(lock); } while (0)
 
-#define spin_unlock_irqrestore(lock, flags)	do { _raw_spin_unlock(lock);  local_irq_restore(flags); preempt_enable(); } while (0)
+#define spin_unlock_irqrestore(lock, flags)	do { _raw_spin_unlock(lock);  local_irq_restore_preempt(flags); } while (0)
 #define _raw_spin_unlock_irqrestore(lock, flags) do { _raw_spin_unlock(lock);  local_irq_restore(flags); } while (0)
-#define spin_unlock_irq(lock)			do { _raw_spin_unlock(lock);  local_irq_enable(); preempt_enable();       } while (0)
+#define spin_unlock_irq(lock)			do { _raw_spin_unlock(lock);  local_irq_enable_preempt();       } while (0)
 #define spin_unlock_bh(lock)			do { spin_unlock(lock); local_bh_enable(); } while (0)
 
-#define read_unlock_irqrestore(lock, flags)	do { _raw_read_unlock(lock);  local_irq_restore(flags); preempt_enable(); } while (0)
-#define read_unlock_irq(lock)			do { _raw_read_unlock(lock);  local_irq_enable(); preempt_enable(); } while (0)
+#define read_unlock_irqrestore(lock, flags)	do { _raw_read_unlock(lock);  local_irq_restore_preempt(flags); } while (0)
+#define read_unlock_irq(lock)			do { _raw_read_unlock(lock);  local_irq_enable_preempt(); } while (0)
 #define read_unlock_bh(lock)			do { read_unlock(lock);  local_bh_enable();        } while (0)
 
-#define write_unlock_irqrestore(lock, flags)	do { _raw_write_unlock(lock); local_irq_restore(flags); preempt_enable(); } while (0)
-#define write_unlock_irq(lock)			do { _raw_write_unlock(lock); local_irq_enable(); preempt_enable();       } while (0)
+#define write_unlock_irqrestore(lock, flags)	do { _raw_write_unlock(lock); local_irq_restore_preempt(flags); } while (0)
+#define write_unlock_irq(lock)			do { _raw_write_unlock(lock); local_irq_enable_preempt();       } while (0)
 #define write_unlock_bh(lock)			do { write_unlock(lock); local_bh_enable();        } while (0)
 #define spin_trylock_bh(lock)			({ int __r; local_bh_disable();\
 						__r = spin_trylock(lock);      \
diff -urP -I \$Id:.*Exp \$ -X /usr/src/patch.exclude linux-2.5.27-ingo-H6-org/kernel/fork.c linux/kernel/fork.c
--- linux-2.5.27-ingo-H6-org/kernel/fork.c	Wed Jul 24 12:50:03 2002
+++ linux/kernel/fork.c	Wed Jul 24 12:46:52 2002
@@ -753,10 +753,8 @@
 		current->time_slice = 1;
 		preempt_disable();
 		scheduler_tick(0, 0);
-		local_irq_restore(flags);
-		preempt_enable();
-	} else
-		local_irq_restore(flags);
+	}
+        local_irq_restore_preempt(flags);
 
 	/*
 	 * Ok, add it to the run-queues and make it
diff -urP -I \$Id:.*Exp \$ -X /usr/src/patch.exclude linux-2.5.27-ingo-H6-org/kernel/sched.c linux/kernel/sched.c
--- linux-2.5.27-ingo-H6-org/kernel/sched.c	Wed Jul 24 12:50:03 2002
+++ linux/kernel/sched.c	Wed Jul 24 12:12:02 2002
@@ -907,6 +907,7 @@
 		printk("bad: schedule() with irqs disabled!\n");
 		show_stack(NULL);
 		preempt_enable_no_resched();
+                return;
 	}
 
 need_resched:
diff -urP -I \$Id:.*Exp \$ -X /usr/src/patch.exclude linux-2.5.27-ingo-H6-org/mm/slab.c linux/mm/slab.c
--- linux-2.5.27-ingo-H6-org/mm/slab.c	Wed Jul 24 12:50:03 2002
+++ linux/mm/slab.c	Wed Jul 24 12:38:27 2002
@@ -1386,9 +1386,8 @@
 			} else {
 				STATS_INC_ALLOCMISS(cachep);
 				objp = kmem_cache_alloc_batch(cachep,flags);
-				local_irq_restore(save_flags);
 				/* end of non-preemptible region */
-				preempt_enable();
+				local_irq_restore_preempt(save_flags);
 				if (!objp)
 					goto alloc_new_slab_nolock;
 				return objp;

Re: [patch] irqlock patch 2.5.27-H4

[Oleg Nesterov]

Hello.

arch/i386/kernel/microcode.c:do_microcode_update() calls
smp_call_function(do_update_one). do_update_one() does
spin_lock/unlock. So smp_call_function_interrupt() needs
irq_enter().

entry.S has unneeded GET_THREAD_INFO(%ebx) in
device_not_available() trap.

Patch against 2.5.28.

--- arch/i386/kernel/smp.c~	Fri Jul 26 07:49:03 2002
+++ arch/i386/kernel/smp.c	Fri Jul 26 07:53:45 2002
@@ -646,7 +646,10 @@
 	/*
 	 * At this point the info structure may be out of scope unless wait==1
 	 */
+	irq_enter();
 	(*func)(info);
+	irq_exit();
+
 	if (wait) {
 		mb();
 		atomic_inc(&call_data->finished);
--- arch/i386/kernel/entry.S~	Fri Jul 26 07:49:03 2002
+++ arch/i386/kernel/entry.S	Fri Jul 26 07:57:48 2002
@@ -417,7 +417,6 @@
 ENTRY(device_not_available)
 	pushl $-1			# mark this as an int
 	SAVE_ALL
-	GET_THREAD_INFO(%ebx)
 	movl %cr0, %eax
 	testl $0x4, %eax		# EM (math emulation bit)
 	jne device_not_available_emulate

Oleg.

Re: [patch] irqlock patch 2.5.27-H4

[Oleg Nesterov]

Hello.

Oleg Nesterov wrote:
> +       irq_enter();
>         (*func)(info);
> +       irq_exit();

It is better to update call_data->finished before
doing soft irqs in irq_exit().

Against 2.5.28:

--- arch/i386/kernel/smp.c~	Fri Jul 26 07:49:03 2002
+++ arch/i386/kernel/smp.c	Fri Jul 26 09:29:20 2002
@@ -646,10 +646,12 @@
 	/*
 	 * At this point the info structure may be out of scope unless wait==1
 	 */
+	irq_enter();
 	(*func)(info);
 	if (wait) {
 		mb();
 		atomic_inc(&call_data->finished);
 	}
+	irq_exit();
 }
 
--- arch/i386/kernel/entry.S~	Fri Jul 26 07:49:03 2002
+++ arch/i386/kernel/entry.S	Fri Jul 26 07:57:48 2002
@@ -417,7 +417,6 @@
 ENTRY(device_not_available)
 	pushl $-1			# mark this as an int
 	SAVE_ALL
-	GET_THREAD_INFO(%ebx)
 	movl %cr0, %eax
 	testl $0x4, %eax		# EM (math emulation bit)
 	jne device_not_available_emulate


Oleg.