this is the next iteration of the big-IRQ-lock removal patch, against
2.5.27 + sched-fixes + ldt-fixes:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A1
one clarification wrt. disable_irq(): while it does not synchronize with
all IRQ sources globally anymore, it does synchronize with that particular
irq source (globally) - so drivers can use it safely to ensure that no
pending IRQ handler is running on another CPU after disable_irq() has
returned.
Changes:
- Oleg Nesterov noticed a do_IRQ() bug which can cause a reschedule in
do_IRQ().
- George Anzinger noticed that local_bh_enable() did not re-run softirqs.
- Linus suggested to still define cli(), sti(), save_flags(),
restore_flags() on UP kernels, to ease the transition.
the patch compiles, boots & works just fine on UP - on SMP it boots just
fine using the following limited .config:
http://redhat.com/~mingo/remove-irqlock-patches/config
(the serial subsystem is disabled for example.)
Ingo
On Sun, 21 Jul 2002, Ingo Molnar wrote:
>
> http://redhat.com/~mingo/remove-irqlock-patches/config
This seems to have tons of stuff which makes it compile, but which is just
broken. Randomly changing "cli()" to "__cli()" apparently just to make it
compile, with no warning that its now buggy.
It's doubly wrong by virtue of the fact that you apparently left
"save_flags()" and "restore_flags()" alone, and changed their semantics
rather drastically.
Ugh. That should not happen. Either it compiles and is expected to work,
or it shouldn't compile (very limited config is fine, of course).
Robert, willing to take a look too?
Linus
On Sun, 21 Jul 2002, Linus Torvalds wrote:
> This seems to have tons of stuff which makes it compile, but which is
> just broken. Randomly changing "cli()" to "__cli()" apparently just to
> make it compile, with no warning that its now buggy.
indeed ...
fixed these, and have categorized every change whether it's safe,
known-unsafe or unknown-effect, and commented the latter two.
i also removed the save_flags() and restore_flags() macros on SMP which
were left there by accident.
new patch is at:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A3
(there's one more open bug, i can now see the 'exited with preempt_count
1' messages.)
Ingo
> (there's one more open bug, i can now see the 'exited with preempt_count
> 1' messages.)
fixed this bug as well, new patch is at:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A4
Ingo
On Sun, 2002-07-21 at 12:14, Ingo Molnar wrote:
>
> On Sun, 21 Jul 2002, Linus Torvalds wrote:
>
> > This seems to have tons of stuff which makes it compile, but which is
> > just broken. Randomly changing "cli()" to "__cli()" apparently just to
> > make it compile, with no warning that its now buggy.
>
> indeed ...
>
> fixed these, and have categorized every change whether it's safe,
> known-unsafe or unknown-effect, and commented the latter two.
Good.
I have brought up a machine with a config similar but not identical to
yours and I am putting it through the paces (SMP+preempt machine). I
really think you nailed this dead on - you did it right - we just need
to clean up the mess you left behind ;)
Ingo, looking over the FIXMEs in the tty layer I think they are
definitely _broke_. At least some of these paths have no global
synchronization now. Someone really needs to go through this cruft and
clean it up and do some proper locking.
Robert Love
On Sun, Jul 21, 2002 at 08:50:35PM +0200, Ingo Molnar wrote:
> (the serial subsystem is disabled for example.)
As far as the serial stuff goes:
- William Irvin and Zwane Mwaikambo have been testing it, found a
deadlock, now fixed. (yay)
- Zwane reports that serial console doesn't work for him. Oddly,
it works here on a Netwinder (which has all the bits'n'pieces to
be close enough to a PC with a PCI bus, southbridge, and standard
serial ports at standard IO bases and standard IRQs) so I'm at a
loss why this works for me but not Zwane.
I'm just sorting out a 2.5.26-rmk1 release, then update to 2.5.27,
make sure it builds, and then I'll be sending the serial stuff to
Linus. Until then, I've no idea if any patch I create will apply
to 2.5.27.
Gimme about an hour or so and I'll have the patch ready.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sun, 21 Jul 2002, Russell King wrote:
> - William Irvin and Zwane Mwaikambo have been testing it, found a
> deadlock, now fixed. (yay)
good.
> - Zwane reports that serial console doesn't work for him. Oddly,
> it works here on a Netwinder (which has all the bits'n'pieces to
> be close enough to a PC with a PCI bus, southbridge, and standard
> serial ports at standard IO bases and standard IRQs) so I'm at a
> loss why this works for me but not Zwane.
i can test this, on SMP as well.
> I'm just sorting out a 2.5.26-rmk1 release, then update to 2.5.27, make
> sure it builds, and then I'll be sending the serial stuff to Linus.
> Until then, I've no idea if any patch I create will apply to 2.5.27.
okay - since the IRQ changes do not (suppose to) affect UP functionality,
there's time to get it fixed.
Ingo
On Sun, Jul 21, 2002 at 08:58:01PM +0100, Russell King wrote:
> On Sun, Jul 21, 2002 at 08:50:35PM +0200, Ingo Molnar wrote:
> > (the serial subsystem is disabled for example.)
>
> As far as the serial stuff goes:
>
> - William Irvin and Zwane Mwaikambo have been testing it, found a
> deadlock, now fixed. (yay)
>
> - Zwane reports that serial console doesn't work for him. Oddly,
> it works here on a Netwinder (which has all the bits'n'pieces to
> be close enough to a PC with a PCI bus, southbridge, and standard
> serial ports at standard IO bases and standard IRQs) so I'm at a
> loss why this works for me but not Zwane.
>
> I'm just sorting out a 2.5.26-rmk1 release, then update to 2.5.27,
> make sure it builds, and then I'll be sending the serial stuff to
> Linus. Until then, I've no idea if any patch I create will apply
> to 2.5.27.
>
> Gimme about an hour or so and I'll have the patch ready.
Ok, 2.5.27 doesn't seem to touch any of the affected files; the patch
still applies. In such a short time period, I've not been able to
confirm that it actually works with 2.5.27, only with 2.5.26.
Here's the complete patch; it's rather large, so for mortals it's
available from:
http://www.arm.linux.org.uk/cvs/serial-2.5.26-3.diff.bz2
I'm going to send it in mail to Linus separately.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
reviewed and fixed the apm.c hacks, it's now using the proper spinlocks
and not cli()/sti(). Latest full patch is at:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A5
(the apm.c changes are attached as well.)
Ingo
--- linux/arch/i386/kernel/apm.c.orig Sun Jul 21 20:37:10 2002
+++ linux/arch/i386/kernel/apm.c Sun Jul 21 22:36:42 2002
@@ -222,6 +222,8 @@
#include <linux/sysrq.h>
+extern rwlock_t xtime_lock;
+extern spinlock_t i8253_lock;
extern unsigned long get_cmos_time(void);
extern void machine_real_restart(unsigned char *, int);
@@ -1141,40 +1143,25 @@
static void set_time(void)
{
- unsigned long flags;
-
- if (got_clock_diff) { /* Must know time zone in order to set clock */
- save_flags(flags);
- cli();
+ if (got_clock_diff) /* Must know time zone in order to set clock */
CURRENT_TIME = get_cmos_time() + clock_cmos_diff;
- restore_flags(flags);
- }
}
static void get_time_diff(void)
{
#ifndef CONFIG_APM_RTC_IS_GMT
- unsigned long flags;
-
/*
* Estimate time zone so that set_time can update the clock
*/
- save_flags(flags);
clock_cmos_diff = -get_cmos_time();
- cli();
clock_cmos_diff += CURRENT_TIME;
got_clock_diff = 1;
- restore_flags(flags);
#endif
}
-static void reinit_timer(void)
+static inline void reinit_timer(void)
{
#ifdef INIT_TIMER_AFTER_SUSPEND
- unsigned long flags;
-
- save_flags(flags);
- cli();
/* set the clock to 100 Hz */
outb_p(0x34,0x43); /* binary, mode 2, LSB/MSB, ch 0 */
udelay(10);
@@ -1182,7 +1169,6 @@
udelay(10);
outb(LATCH >> 8 , 0x40); /* MSB */
udelay(10);
- restore_flags(flags);
#endif
}
@@ -1203,13 +1189,21 @@
}
printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
}
+ /* serialize with the timer interrupt */
+ write_lock_irq(&xtime_lock);
+
+ /* protect against access to timer chip registers */
+ spin_lock(&i8253_lock);
+
get_time_diff();
- cli();
err = set_system_power_state(APM_STATE_SUSPEND);
reinit_timer();
set_time();
ignore_normal_resume = 1;
- sti();
+
+ spin_unlock(&i8253_lock);
+ write_unlock_irq(&xtime_lock);
+
if (err == APM_NO_ERROR)
err = APM_SUCCESS;
if (err != APM_SUCCESS)
@@ -1232,8 +1226,12 @@
{
int err;
+ /* serialize with the timer interrupt */
+ write_lock_irq(&xtime_lock);
/* If needed, notify drivers here */
get_time_diff();
+ write_unlock_irq(&xtime_lock);
+
err = set_system_power_state(APM_STATE_STANDBY);
if ((err != APM_SUCCESS) && (err != APM_NO_ERROR))
apm_error("standby", err);
@@ -1321,7 +1319,9 @@
ignore_bounce = 1;
if ((event != APM_NORMAL_RESUME)
|| (ignore_normal_resume == 0)) {
+ write_lock_irq(&xtime_lock);
set_time();
+ write_unlock_irq(&xtime_lock);
pm_send_all(PM_RESUME, (void *)0);
queue_event(event, NULL);
}
@@ -1336,7 +1336,9 @@
break;
case APM_UPDATE_TIME:
+ write_lock_irq(&xtime_lock);
set_time();
+ write_unlock_irq(&xtime_lock);
break;
case APM_CRITICAL_SUSPEND:
fixed the arch/i386/kernel/mca.c hacks as well:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A6
while there cannot be many MCA SMP boxes in existence, it should be SMP
safe as well.
Ingo
--- linux/arch/i386/kernel/mca.c.orig Sun Jun 9 07:27:54 2002
+++ linux/arch/i386/kernel/mca.c Sun Jul 21 22:49:42 2002
@@ -102,6 +102,12 @@
static struct MCA_info* mca_info = NULL;
+/*
+ * Motherboard register spinlock. Untested on SMP at the moment, but
+ * are there any MCA SMP boxes?
+ */
+static spinlock_t mca_lock = SPIN_LOCK_UNLOCKED;
+
/* MCA registers */
#define MCA_MOTHERBOARD_SETUP_REG 0x94
@@ -213,8 +219,11 @@
}
memset(mca_info, 0, sizeof(struct MCA_info));
- save_flags(flags);
- cli();
+ /*
+ * We do not expect many MCA interrupts during initialization,
+ * but let us be safe:
+ */
+ spin_lock_irq(&mca_lock);
/* Make sure adapter setup is off */
@@ -300,8 +309,7 @@
outb_p(0, MCA_ADAPTER_SETUP_REG);
/* Enable interrupts and return memory start */
-
- restore_flags(flags);
+ spin_unlock_irq(&mca_lock);
for (i = 0; i < MCA_STANDARD_RESOURCES; i++)
request_resource(&ioport_resource, mca_standard_resources + i);
@@ -514,8 +522,7 @@
if(slot < 0 || slot >= MCA_NUMADAPTERS || mca_info == NULL) return 0;
if(reg < 0 || reg >= 8) return 0;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&mca_lock, flags);
/* Make sure motherboard setup is off */
@@ -566,7 +573,7 @@
mca_info->slot[slot].pos[reg] = byte;
- restore_flags(flags);
+ spin_unlock_irqrestore(&mca_lock, flags);
return byte;
} /* mca_read_pos() */
@@ -610,8 +617,7 @@
if(mca_info == NULL)
return;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&mca_lock, flags);
/* Make sure motherboard setup is off */
@@ -623,7 +629,7 @@
outb_p(byte, MCA_POS_REG(reg));
outb_p(0, MCA_ADAPTER_SETUP_REG);
- restore_flags(flags);
+ spin_unlock_irqrestore(&mca_lock, flags);
/* Update the global register list, while we have the byte */
the arch/i386/kernel/vm86.c hacks are fixed now as well:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A7
this was relatively simple, the irqbits global variable needed to be
protected by a global spinlock. I found no other global locking assumption
in the code. In fact there was even a small bug in the code - if
free_vm86_irq() was called outside of the global IRQ lock then there's a
irqbits corruption window in free_vm87_irq() - this is true even though
that particular IRQ source will not interfere - it's other IRQ sources
that might change the irqbits variable. The patch fixes this.
this was the last remaining x86 usage of cli()/sti().
Ingo
--- linux/arch/i386/kernel/vm86.c.orig Sun Jun 9 07:27:22 2002
+++ linux/arch/i386/kernel/vm86.c Sun Jul 21 22:55:57 2002
@@ -571,6 +571,8 @@
struct task_struct *tsk;
int sig;
} vm86_irqs[16];
+
+static spinlock_t irqbits_lock = SPIN_LOCK_UNLOCKED;
static int irqbits;
#define ALLOWED_SIGS ( 1 /* 0 = don't send a signal */ \
@@ -580,9 +582,8 @@
static void irq_handler(int intno, void *dev_id, struct pt_regs * regs) {
int irq_bit;
unsigned long flags;
-
- save_flags(flags);
- cli();
+
+ spin_lock_irqsave(&irqbits_lock, flags);
irq_bit = 1 << intno;
if ((irqbits & irq_bit) || ! vm86_irqs[intno].tsk)
goto out;
@@ -591,14 +592,19 @@
send_sig(vm86_irqs[intno].sig, vm86_irqs[intno].tsk, 1);
/* else user will poll for IRQs */
out:
- restore_flags(flags);
+ spin_unlock_irqrestore(&irqbits_lock, flags);
}
static inline void free_vm86_irq(int irqnumber)
{
+ unsigned long flags;
+
free_irq(irqnumber,0);
vm86_irqs[irqnumber].tsk = 0;
+
+ spin_lock_irqsave(&irqbits_lock, flags);
irqbits &= ~(1 << irqnumber);
+ spin_unlock_irqrestore(&irqbits_lock, flags);
}
static inline int task_valid(struct task_struct *tsk)
@@ -635,11 +641,10 @@
if ( (irqnumber<3) || (irqnumber>15) ) return 0;
if (vm86_irqs[irqnumber].tsk != current) return 0;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&irqbits_lock, flags);
bit = irqbits & (1 << irqnumber);
irqbits &= ~bit;
- restore_flags(flags);
+ spin_unlock_irqrestore(&irqbits_lock, flags);
return bit;
}
On Sun, 2002-07-21 at 21:50, Ingo Molnar wrote:
>
> fixed the arch/i386/kernel/mca.c hacks as well:
>
> http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A6
>
> while there cannot be many MCA SMP boxes in existence, it should be SMP
> safe as well.
The NCR boxes and the 9595 are probably about it. There are plenty of
9595's around but if you ever acquire one make sure someone else is
paying postage 8)
using serial-2.5.26-3.diff, i get an oops here:
(gdb) list *0xc01b1f03
0xc01b1f03 is in serial_in (serial_8250.c:189).
184 return readb(up->port.membase + offset);
185
186 default:
187 return inb(up->port.iobase + offset);
188 }
189 }
190
191 static _INLINE_ void
192 serial_out(struct uart_8250_port *up, int offset, int value)
193 {
(gdb)
called from:
(gdb) list *0xc01b3450
0xc01b3450 is in serial8250_interrupt (serial_8250.c:946).
941 struct uart_8250_port *up;
942 unsigned int iir;
943
944 up = list_entry(l, struct uart_8250_port, list);
945
946 iir = serial_in(up, UART_IIR);
947 if (!(iir & UART_IIR_NO_INT)) {
948 spin_lock(&up->port.lock);
949 serial8250_handle_port(up, regs);
950 spin_unlock(&up->port.lock);
(gdb)
when echo-ing into a serial port, which is also set up for kernel serial
console. (the kernel serial console produces no output.)
Ingo
On Sun, 21 Jul 2002, Russell King wrote:
> > (gdb) list *0xc01b1f03
> > 0xc01b1f03 is in serial_in (serial_8250.c:189).
> > 184 return readb(up->port.membase + offset);
> > 185
> > 186 default:
> > 187 return inb(up->port.iobase + offset);
> > 188 }
> > 189 }
> > 190
> > 191 static _INLINE_ void
> > 192 serial_out(struct uart_8250_port *up, int offset, int value)
> > 193 {
> > (gdb)
>
> Interesting. Not had a report of that thus far. Can you send me any
> kernel messages relating to serial devices please, and the bad address
> that caused the oops? (line 189 is obviously a lie...)
i believe it was the inb's dereference.
i dont have the oops around anymore, since serial logging is not working
;-) Had to write it down from screen.
> > when echo-ing into a serial port, which is also set up for kernel serial
> > console. (the kernel serial console produces no output.)
>
> Weird. Currently I've no idea what's causing this; I've been booting
> machines with "console=ttyS0,115200n8" fine here with no noticable
> problems.
>
> Again, any useful kernel messages?
will reproduce and write down the full oops - it was an illegal
dereference on 0xfffffff8 (-8 offset of a NULL pointer) or something like
that.
my serial setup is:
append="console=ttyS1,115200 console=tty0"
old serial driver:
Serial driver version 5.05c (2001-07-08) with MANY_PORTS SHARE_IRQ
SERIAL_PCI enabled
ttyS00 at 0x03f8 (irq = 4) is a 16550A
ttyS01 at 0x02f8 (irq = 3) is a 16550A
i'm pretty sure it hung when i echoed to ttyS1 - not 100% sure though.
Ingo
On Sun, Jul 21, 2002 at 11:09:34PM +0200, Ingo Molnar wrote:
> using serial-2.5.26-3.diff, i get an oops here:
>
> (gdb) list *0xc01b1f03
> 0xc01b1f03 is in serial_in (serial_8250.c:189).
> 184 return readb(up->port.membase + offset);
> 185
> 186 default:
> 187 return inb(up->port.iobase + offset);
> 188 }
> 189 }
> 190
> 191 static _INLINE_ void
> 192 serial_out(struct uart_8250_port *up, int offset, int value)
> 193 {
> (gdb)
Interesting. Not had a report of that thus far. Can you send me any
kernel messages relating to serial devices please, and the bad address
that caused the oops? (line 189 is obviously a lie...)
> when echo-ing into a serial port, which is also set up for kernel serial
> console. (the kernel serial console produces no output.)
Weird. Currently I've no idea what's causing this; I've been booting
machines with "console=ttyS0,115200n8" fine here with no noticable
problems.
Again, any useful kernel messages?
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
the genhd.c bit is safe as well, removed the comment. Only the tty and the
ide/main.c changes are left the 'dubious' category - everything else is
supposed to be safe. (and of course there's all the other stuff that does
not compile at the moment.) Latest patch is at:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A9
Ingo
new-serial boot messages:
Serial: 8250/16550 driver $Revision: 1.80 $ IRQ sharing enabled
ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A
and it does not crash on UP. (will re-try with SMP now.) Still no serial
console output though.
Ingo
okay, the crash happens if i boot an SMP kernel, and do the following:
cat /dev/ttyS0
[ ... system works at this point, no crash ... ]
Ctrl-C
[ ... system crashes ... ]
note that ttyS0 is not the serial console device - so this is a plain
unconnected serial port.
the oops, written down by hand:
Unable to handle kernel paging request at virtual address ffffff8a
c01aa193
CPU: 1
EIP: 0010:[<c01aa193>] Not tainted
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010292
eax: 00000002 ebx: ffffff70 ecx: 00000000 edx: 000000ff
esi: ffffff70 edi: 00000000 ebp: c02d3ad0 esp: c13e1edc
Call Trace: [<c01ab696>] [<c0114210>] [<c0109d79>] [<c010a07e>]
[<c0113821>]
[<c0105470>] [<c0108347>] [<c01054c0>] [<c0105470>] [<c01054eb>]
[<c010553a>]
[<c011d3ac>] [<c011d1ca>]
Aiee, killing interrupt handler!
Warning (Oops_read): Code line not seen, dumping what data is available
>>EIP; c01aa193 <serial_in+13/80> <=====
Trace; c01ab696 <serial8250_interrupt+66/1a0>
Trace; c0114210 <move+50/90>
Trace; c0109d79 <handle_IRQ_event+69/a0>
Trace; c010a07e <do_IRQ+ee/190>
Trace; c0113821 <smp_apic_timer_interrupt+131/140>
Trace; c0105470 <default_idle+0/50>
Trace; c0108347 <common_interrupt+1f/24>
Trace; c01054c0 <poll_idle+0/40>
Trace; c0105470 <default_idle+0/50>
Trace; c01054eb <poll_idle+2b/40>
Trace; c010553a <cpu_idle+3a/50>
Trace; c011d3ac <release_console_sem+11c/120>
Trace; c011d1ca <printk+18a/200>
ie. the second, idle CPU received a serial interrupt and crashed in it.
the .config used for this is attached. The gdb backtrace:
(gdb) list *0xc01aa193
0xc01aa193 is in serial_in (serial_8250.c:176).
171 { "RSA", 2048, UART_CLEAR_FIFO | UART_USE_FIFO }
172 };
173
174 static _INLINE_ unsigned int serial_in(struct uart_8250_port *up, int offset)
175 {
176 offset <<= up->port.regshift;
177
178 switch (up->port.iotype) {
179 case SERIAL_IO_HUB6:
180 outb(up->port.hub6 - 1 + offset, up->port.iobase);
(gdb)
(gdb) list *0xc01ab696
0xc01ab696 is in serial8250_interrupt (serial_8250.c:947).
942 unsigned int iir;
943
944 up = list_entry(l, struct uart_8250_port, list);
945
946 iir = serial_in(up, UART_IIR);
947 if (!(iir & UART_IIR_NO_INT)) {
948 spin_lock(&up->port.lock);
949 serial8250_handle_port(up, regs);
950 spin_unlock(&up->port.lock);
951
(gdb)
this backtrace looks more credible to me.
Ingo
On Sun, Jul 21, 2002 at 11:26:40PM +0200, Ingo Molnar wrote:
>
> the genhd.c bit is safe as well, removed the comment.
Is there any reason the sti is there at all? In -dj almost all drivers
use module_init() now so it becomes increasingly useless..
On Sun, 21 Jul 2002, Christoph Hellwig wrote:
> > the genhd.c bit is safe as well, removed the comment.
>
> Is there any reason the sti is there at all? In -dj almost all drivers
> use module_init() now so it becomes increasingly useless..
well, indeed. While the sti() can be understood to a certain degree - we
used to boot with the IRQ lock on and accidentally leaving it enabled can
cause problems - but otherwise preceeding code should not disable
interrupts in an unbalanced way. I've removed the __sti() from my tree.
there's even more ancient code in the block driver init path, eg. in
drivers/block/ll_rw_blk.c:blk_dev_init():
outb_p(0xc, 0x3f2);
i suspect this is ancient Linux code. 0x3f2 is one of the floppy
controller ports - many modern x86 boxes do not even have a floppy
controller! I've removed this from my tree as well - if this is needed at
all then it belongs into the floppy driver. Latest patch is at:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-B0
Ingo
On 21 Jul 2002, Robert Love wrote:
> Ingo, looking over the FIXMEs in the tty layer I think they are
> definitely _broke_. At least some of these paths have no global
> synchronization now. Someone really needs to go through this cruft and
> clean it up and do some proper locking.
sure, feel free.
Ingo
On Mon, Jul 22, 2002 at 01:44:16AM +0200, Ingo Molnar wrote:
> On Mon, 22 Jul 2002, Russell King wrote:
>
> > Actually its to cover the case where you have a floppy drive, and you've
> > booted the kernel from a floppy disk, and the kernel doesn't have the
> > floppy driver built in. It turns the floppy drive off, cause there's
> > nothing else to do that.
>
> this should then be done by the floppy boot code?
Sounds like a better idea to me. Although I'm not one to try it out. 8)
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Mon, Jul 22, 2002 at 03:20:56PM +0200, Christoph Hellwig wrote:
> I'd prefer the following:
>
> void irq_off(void);
> void irq_on(void);
>
> flags_t irq_save(); /* the old irq_save_off() */
> void irq_restore(flags_t);
>
> void __irq_save(void); /* without saveing */
^^^^^ stupid ^^^^^
rmk's sanity checker caught this, should be without disabling.
On Sun, 2002-07-21 at 17:40, Russell King wrote:
> On Mon, Jul 22, 2002 at 02:31:16AM +0200, Ingo Molnar wrote:
> > +drivers that want to disable local interrupts (interrupts on the
> > +current CPU), can use the following four macros:
> > +
> > + __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
>
> Last mail before zzz (hopefully) - what about
> local_irq_{enable,disable,save,restore} ?
>
> With the exception of local_irq_save() which is actually
> local_irq_save_disable(), I find these to be more "descriptive" of
> their function.
Yes and double yes.
And for two reasons: First, the __ prefix is unnecessary now. Second,
not all the world is an x86 (it just looks that way).
local_irq_foo is definitely preferred.
I'd make the patch and go through the effort to rename and replace if
Linus assured me it was in.
Robert Love
On Mon, Jul 22, 2002 at 03:26:57PM +0200, Ingo Molnar wrote:
> no, the on is not implicit at all. If you restore into a disabled state
> then it will go from on to off.
well, for the normal use of it. Okay, I give up, even if the nameing
looks strange in the beginning is is consistant :)
On Mon, Jul 22, 2002 at 03:38:29PM +0200, Ingo Molnar wrote:
> > but not with irq_restore :) maybe irq_restore_on() although the on is
> > implicit.
>
> think about it - if this was true then irq_restore_on() would be
> equivalent to irq_on().
yupp, you're right. could you take the prototype changes anyway?
update: after some offline debugging and RMK fixing a number of (mostly
SMP) bugs, both the serial line discipline and serial kernel console
features are now fully functional under the new serial subsystem. x86 SMP
and UP works fine as well.
i also applied it to the irqlock tree, and only a single line had to be
changed [synchronize_irq()] and it worked flawlessly on SMP.
i've uploaded the patch (against sched + irqlock tree), it's at:
http://redhat.com/~mingo/remove-irqlock-patches/newserial-2.5.27-A0.bz2
I'd vote for Russell's stuff to be considered for inclusion ...
Ingo
On Sun, Jul 21, 2002 at 11:56:11PM +0200, Ingo Molnar wrote:
> there's even more ancient code in the block driver init path, eg. in
> drivers/block/ll_rw_blk.c:blk_dev_init():
>
> outb_p(0xc, 0x3f2);
>
> i suspect this is ancient Linux code. 0x3f2 is one of the floppy
> controller ports - many modern x86 boxes do not even have a floppy
> controller! I've removed this from my tree as well - if this is needed at
> all then it belongs into the floppy driver.
Actually its to cover the case where you have a floppy drive, and you've
booted the kernel from a floppy disk, and the kernel doesn't have the
floppy driver built in. It turns the floppy drive off, cause there's
nothing else to do that.
Obviously putting it in the floppy driver wouldn't be meaningful.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Mon, Jul 22, 2002 at 03:43:50PM +0200, Ingo Molnar wrote:
> i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
> and i do not see the need for a more complex (or more opaque) irqflags
> type. It's not that we confuse flags with some other flag all that
> frequently that would necessiate some structure-based more abstract
> protection of these variables.
It's just that we don't really care what it is. But the type change
wasn't my main point, rather the returning of the flags value in
irq_save to allow implementing it as non-macro.
On Sun, 21 Jul 2002, Linus Torvalds wrote:
> Looks good. Merged into my BK tree now, so please do future patches
> against this one..
i've done a minor comment update in softirq.c, plus i've written a
cli-sti-removal.txt guide to help driver writers do the transition:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-C2
(also attached)
Ingo
--- linux/Documentation/cli-sti-removal.txt.orig Mon Jul 22 02:25:53 2002
+++ linux/Documentation/cli-sti-removal.txt Mon Jul 22 02:31:47 2002
@@ -0,0 +1,115 @@
+
+#### cli()/sti() removal guide, started by Ingo Molnar <[email protected]>
+
+
+as of 2.5.28, four popular macros have been removed on SMP, and
+are being phased out on UP:
+
+ cli(), sti(), save_flags(flags), restore_flags(flags)
+
+until now it was possible to protect driver code against interrupt
+handlers via a cli(), but from now on other, more lightweight methods
+have to be used for synchronization, such as spinlocks or semaphores.
+
+for example, driver code that used to do something like:
+
+ struct driver_data;
+
+ irq_handler (...)
+ {
+ ....
+ driver_data.finish = 1;
+ driver_data.new_work = 0;
+ ....
+ }
+
+ ...
+
+ ioctl_func (...)
+ {
+ ...
+ cli();
+ ...
+ driver_data.finish = 0;
+ driver_data.new_work = 2;
+ ...
+ sti();
+ ...
+ }
+
+was SMP-correct because the cli() function ensured that no
+interrupt handler (amongst them the above irq_handler()) function
+would execute while the cli()-ed section is executing.
+
+but from now on a more direct method of locking has to be used:
+
+ spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;
+ struct driver_data;
+
+ irq_handler (...)
+ {
+ unsigned long flags;
+ ....
+ spin_lock_irqsave(&driver_lock, flags);
+ ....
+ driver_data.finish = 1;
+ driver_data.new_work = 0;
+ ....
+ spin_unlock_irqrestore(&driver_lock, flags);
+ ....
+ }
+
+ ...
+
+ ioctl_func (...)
+ {
+ ...
+ spin_lock_irq(&driver_lock);
+ ...
+ driver_data.finish = 0;
+ driver_data.new_work = 2;
+ ...
+ spin_unlock_irq(&driver_lock);
+ ...
+ }
+
+the above code has a number of advantages:
+
+- the locking relation is easier to understand - actual lock usage
+ pinpoints the critical sections. cli() usage is too opaque.
+ Easier to understand means it's easier to debug.
+
+- it's faster, because spinlocks are faster to acquire than the
+ potentially heavily-used IRQ lock. Furthermore, your driver does
+ not have to wait eg. for a big heavy SCSI interrupt to finish,
+ because the driver_lock spinlock is only used by your driver.
+ cli() on the other hand was used by many drivers, and extended
+ the critical section to the whole IRQ handler function - creating
+ serious lock contention.
+
+
+to make the transition easier, we've still kept the cli(), sti(),
+save_flags() and restore_flags() macros defined on UP systems - but
+their usage will be phased out until the 2.6 kernel is released.
+
+drivers that want to disable local interrupts (interrupts on the
+current CPU), can use the following four macros:
+
+ __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
+
+but beware, their meaning and semantics are much simpler, far from
+that of cli(), sti(), save_flags(flags) and restore_flags(flags).
+
+
+another related change is that synchronize_irq() now takes a parameter:
+synchronize_irq(irq). This change too has the purpose of making SMP
+synchronization more lightweight - this way you can wait for your own
+interrupt handler to finish, no need to wait for other IRQ sources.
+
+
+why were these changes done? The main reason was the architectural burden
+of maintaining the cli()/sti() interface - it became a real problem. The
+new interrupt system is much more streamlined, easier to understand, debug,
+and it's also a bit faster - the same happened to it that will happen to
+cli()/sti() using drivers once they convert to spinlocks :-)
+
--- linux/kernel/softirq.c.orig Mon Jul 22 01:37:31 2002
+++ linux/kernel/softirq.c Mon Jul 22 01:38:18 2002
@@ -26,10 +26,6 @@
execution. Hence, we get something sort of weak cpu binding.
Though it is still not clear, will it result in better locality
or will not.
- - These softirqs are not masked by global cli() and start_bh_atomic()
- (by clear reasons). Hence, old parts of code still using global locks
- MUST NOT use softirqs, but insert interfacing routines acquiring
- global locks. F.e. look at BHs implementation.
Examples:
- NET RX softirq. It is multithreaded and does not require
On Sun, 2002-07-21 at 17:31, Ingo Molnar wrote:
> i've done a minor comment update in softirq.c, plus i've written a
> cli-sti-removal.txt guide to help driver writers do the transition:
Nice document.
One more doc correction while we are at it...
Robert Love
diff -urN linux-2.5.27/Documentation/preempt-locking.txt linux/Documentation/preempt-locking.txt
--- linux-2.5.27/Documentation/preempt-locking.txt Sat Jul 20 12:11:06 2002
+++ linux/Documentation/preempt-locking.txt Sun Jul 21 17:59:13 2002
@@ -70,7 +70,8 @@
preempt_enable() decrement the preempt counter
preempt_disable() increment the preempt counter
preempt_enable_no_resched() decrement, but do not immediately preempt
-preempt_get_count() return the preempt counter
+preempt_check_resched() if needed, reschedule
+preempt_count() return the preempt counter
The functions are nestable. In other words, you can call preempt_disable
n-times in a code path, and preemption will not be reenabled until the n-th
On Mon, Jul 22, 2002 at 03:43:50PM +0200, Ingo Molnar wrote:
> i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
> and i do not see the need for a more complex (or more opaque) irqflags
> type.
A feature request then. Type checking. Too many people try to stuff
the value into an int or signed long.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sun, 2002-07-21 at 15:19, Ingo Molnar wrote:
> sure, feel free.
Har Har... where is Ted? ;-)
Seriously, though, I check my mail after a couple hours and you have
solved nearly every outstanding issue. Excellent.
Robert Love
On Mon, 22 Jul 2002, Russell King wrote:
> > > Actually its to cover the case where you have a floppy drive, and you've
> > > booted the kernel from a floppy disk, and the kernel doesn't have the
> > > floppy driver built in. It turns the floppy drive off, cause there's
> > > nothing else to do that.
> >
> > this should then be done by the floppy boot code?
>
> Sounds like a better idea to me. Although I'm not one to try it out. 8)
i've started adding it, just to realize that bootsect.S already turns off
the floppy motor.
so i think the issue got solved the easy way ;)
Ingo
On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> > irq_off()
> > irq_on()
> > irq_save(flags)
> > irq_save_off(flags)
> > irq_restore(flags)
>
> I'd prefer the following:
>
> void irq_off(void);
> void irq_on(void);
>
> flags_t irq_save(); /* the old irq_save_off() */
> void irq_restore(flags_t);
>
> void __irq_save(void); /* without saveing */
>
> rational: proper function-like API (should be inlines), irq save
> without disableing is very uncommon, better make the API symmetric.
i agree mostly, but i do not agree with __irq_save() and irq_save().
What's wrong with "flags_t irq_save_off()" - the name carries the proper
meaning, and it also harmonizes with irq_off().
Ingo
On Mon, 22 Jul 2002, Russell King wrote:
> > i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
> > and i do not see the need for a more complex (or more opaque) irqflags
> > type.
>
> A feature request then. Type checking. Too many people try to stuff
> the value into an int or signed long.
the next portion of the quote deals with this:
> > It's not that we confuse flags with some other flag all that
> > frequently that would necessiate some structure-based more abstract
> > protection of these variables.
are you sure type-checking is really needed? Sure people can mess up the
flags variable, but 64-bit archs could do a sizeof at compile-time.
Ingo
On 22 Jul 2002, Alan Cox wrote:
> > > Actually its to cover the case where you have a floppy drive, and you've
> > > booted the kernel from a floppy disk, and the kernel doesn't have the
> > > floppy driver built in. It turns the floppy drive off, cause there's
> > > nothing else to do that.
> >
> > this should then be done by the floppy boot code?
>
> Most definitely. On legacy free boxes there may not even be a floppy
> controller present, and on non x86 your guess is as good as mine at
> where the fdc lives.
non-x86 was covered via an #ifdef, but legacy-free is not covered.
Ingo
On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> > i agree mostly, but i do not agree with __irq_save() and irq_save().
> > What's wrong with "flags_t irq_save_off()" - the name carries the proper
> > meaning, and it also harmonizes with irq_off().
>
> but not with irq_restore :) maybe irq_restore_on() although the on is
> implicit.
think about it - if this was true then irq_restore_on() would be
equivalent to irq_on().
Ingo
On Mon, 2002-07-22 at 00:44, Ingo Molnar wrote:
>
> On Mon, 22 Jul 2002, Russell King wrote:
>
> > Actually its to cover the case where you have a floppy drive, and you've
> > booted the kernel from a floppy disk, and the kernel doesn't have the
> > floppy driver built in. It turns the floppy drive off, cause there's
> > nothing else to do that.
>
> this should then be done by the floppy boot code?
Most definitely. On legacy free boxes there may not even be a floppy
controller present, and on non x86 your guess is as good as mine at
where the fdc lives.
Alan
On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> void irq_off(void);
> void irq_on(void);
>
> flags_t irq_save();
> void irq_restore(flags_t);
i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
and i do not see the need for a more complex (or more opaque) irqflags
type. It's not that we confuse flags with some other flag all that
frequently that would necessiate some structure-based more abstract
protection of these variables.
(wrt. inline functions, every architecture is free to define them as
inline functions as they see fit.)
Ingo
On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> > i agree mostly, but i do not agree with __irq_save() and irq_save().
> > What's wrong with "flags_t irq_save_off()" - the name carries the proper
> > meaning, and it also harmonizes with irq_off().
>
> but not with irq_restore :) maybe irq_restore_on() although the on
> is implicit.
no, the on is not implicit at all. If you restore into a disabled state
then it will go from on to off.
Ingo
On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> > no, the on is not implicit at all. If you restore into a disabled state
> > then it will go from on to off.
>
> well, for the normal use of it. Okay, I give up, even if the nameing
> looks strange in the beginning is is consistant :)
it does precisely what it says:
irq_off() => turn local IRQs off
irq_on() => turn local IRQs on
irq_save(flags) => save the current IRQ state into flags. The
state can be on or off. (on some
architectures there's even more bits in it.)
irq_save_off(flags) => save the current IRQ state into flags and
disable interrupts.
irq_restore(flags) => restore the IRQ state from flags.
while it's true that 'normally' we save irq-enabled state, it's not at all
sure, eg. when nested irq_save() is done then the first flags will carry
an irqs-on bit, the other nested flags will carry an irqs-off flag - and
the nested irq_restore() will restore to irqs-off state.
this is how it has worked in the past 10 years or so.
but i've added this description to the cli-sti guide :-) Check out the -D7
patch:
http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-D7
Ingo
On Mon, Jul 22, 2002 at 03:01:48PM +0200, Ingo Molnar wrote:
> So what i did in my tree was to introduce the following 5 core means of
> manipulating the local interrupt flags:
>
> irq_off()
> irq_on()
> irq_save(flags)
> irq_save_off(flags)
> irq_restore(flags)
I'd prefer the following:
void irq_off(void);
void irq_on(void);
flags_t irq_save(); /* the old irq_save_off() */
void irq_restore(flags_t);
void __irq_save(void); /* without saveing */
rational: proper function-like API (should be inlines), irq save
without disableing is very uncommon, better make the API symmetric.
On Mon, Jul 22, 2002 at 03:23:40PM +0200, Ingo Molnar wrote:
> i agree mostly, but i do not agree with __irq_save() and irq_save().
> What's wrong with "flags_t irq_save_off()" - the name carries the proper
> meaning, and it also harmonizes with irq_off().
but not with irq_restore :) maybe irq_restore_on() although the on
is implicit.
On Mon, 22 Jul 2002, Marcin Dalecki wrote:
> > i'm hesitant for a number of reasons. Eg. irq_save_off(flags) has to be a
> > macro, otherwise we move the assignment into the irqs-off section.
> > Compare:
> >
> > flags = irq_save_off();
> >
> > with:
> > irq_flags_off(flags);
> >
> > sure, it could be written as:
> >
> > flags = irq_save();
> > irq_off();
> >
> > but then again the macro form is more compact.
>
> By 2 characters. [...]
and a full line ...
> [...] And hiding the side-effect. We don't have the notion of var
> argument passing like in pascal or C++ here.
well, it's a well-known side effect on the other hand.
Ingo
On Mon, 22 Jul 2002, Russell King wrote:
> Actually its to cover the case where you have a floppy drive, and you've
> booted the kernel from a floppy disk, and the kernel doesn't have the
> floppy driver built in. It turns the floppy drive off, cause there's
> nothing else to do that.
this should then be done by the floppy boot code?
Ingo
we abstracted simple types such as pte_t, pmd_t and pgd_t for one good
reason: it's 3 *distinct* entities that can be confused quite easily,
causing subtle VM bugs (eg. for quite some time the x86 arch had 2-level
paging only, so to have a proper 3-level paging VM we needed these type
checks).
i dont sense the same type of urge (and danger of mixup) wrt. the
interrupt flags.
Ingo
On Mon, Jul 22, 2002 at 02:31:16AM +0200, Ingo Molnar wrote:
> +drivers that want to disable local interrupts (interrupts on the
> +current CPU), can use the following four macros:
> +
> + __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
Last mail before zzz (hopefully) - what about
local_irq_{enable,disable,save,restore} ?
With the exception of local_irq_save() which is actually
local_irq_save_disable(), I find these to be more "descriptive" of
their function.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-B0
Looks good. Merged into my BK tree now, so please do future patches
against this one..
Linus
On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> yupp, you're right. could you take the prototype changes anyway?
i'm hesitant for a number of reasons. Eg. irq_save_off(flags) has to be a
macro, otherwise we move the assignment into the irqs-off section.
Compare:
flags = irq_save_off();
with:
irq_flags_off(flags);
sure, it could be written as:
flags = irq_save();
irq_off();
but then again the macro form is more compact.
Ingo
On Mon, 22 Jul 2002, Russell King wrote:
> On Mon, Jul 22, 2002 at 02:31:16AM +0200, Ingo Molnar wrote:
> > +drivers that want to disable local interrupts (interrupts on the
> > +current CPU), can use the following four macros:
> > +
> > + __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
>
> Last mail before zzz (hopefully) - what about
> local_irq_{enable,disable,save,restore} ?
>
> With the exception of local_irq_save() which is actually
> local_irq_save_disable(), I find these to be more "descriptive" of their
> function.
i actually had something much more drastic in mind! :-) Now that the
global IRQ lock is no more, there's no 'local' and 'global' distinction
anymore between various irq disabling mechanizms.
So what i did in my tree was to introduce the following 5 core means of
manipulating the local interrupt flags:
irq_off()
irq_on()
irq_save(flags)
irq_save_off(flags)
irq_restore(flags)
the equivalencies are:
local_irq_save(flags) == irq_save_off(flags)
local_irq_restore(flags) == irq_restore(flags)
local_irq_disable() == irq_off()
local_irq_enable() == irq_on()
__cli() == irq_off()
__sti() == irq_on()
__save_flags(flags) == irq_save(flags)
__restore_flags(flags) == irq_restore(flags)
__save_and_cli(flags) == irq_save_off(flags)
the advantages this rename has:
- consistency - no dual naming for the same thing, we had 9 names for 5
entities.
- __cli, __sti are based on similarly named x86 instruction names which
are often named differently on other architectures. 'irq_off()' and
'irq_on()' on the other hand is a generic description. Also, cli and sti
are more cryptic than need to be.
- the names are actually shorter, more compact, making the source easier
to read and understand.
- there's no conceptual confusion between local_irq_disable() and
irq_disable(), or local_irq_enable() and irq_enable(), which are a
completely separate set of APIs.
- there's no local_ prefix either - we know that interrupts can only be
disabled locally.
- the confusion wrt. local_irq_save() is fixed too. local_irq_save() used
to disable interrupt, although intuition tells that it saves flags only.
- the 5 new names do not create any namespace collision either.
some of these advantages are pretty 'small' in scope - but they all add
up. So in my opinion this is something we should do.
to ease the decision further, here's a patch against Linus' latest BK tree
that does all of this, in the whole kernel tree, for every architecture
and driver:
http://redhat.com/~mingo/remove-irqlock-patches/cli-sti-cleanup-2.5.27-A2
(the patch also cleans up every architecture's include/asm/system.h's irq
macros.)
the patch shaves off a total of 4 KB from the kernel source code size. It
compiles, boots & works just fine on x86 UP and SMP.
Ingo
Ingo Molnar wrote:
> On Mon, 22 Jul 2002, Christoph Hellwig wrote:
>
>
>>yupp, you're right. could you take the prototype changes anyway?
>
>
> i'm hesitant for a number of reasons. Eg. irq_save_off(flags) has to be a
> macro, otherwise we move the assignment into the irqs-off section.
> Compare:
>
> flags = irq_save_off();
>
> with:
> irq_flags_off(flags);
>
> sure, it could be written as:
>
> flags = irq_save();
> irq_off();
>
> but then again the macro form is more compact.
By 2 characters. And hiding the side-effect. We don't have the notion of
var argument passing like in pascal or C++ here.
On Mon, 22 Jul 2002, Ingo Molnar wrote:
>
> So what i did in my tree was to introduce the following 5 core means of
> manipulating the local interrupt flags:
>
> irq_off()
> irq_on()
> irq_save(flags)
> irq_save_off(flags)
> irq_restore(flags)
Ugh.
I'd much rather keep the current "local_xxx" versions, since they clearly
say that it's local to the CPU. Let's face it, people SHOULD NOT USE
THESE!
You should use "spin_lock_irq()" and friends, since those are the only
sane interfaces for doing real irq-safe locking.
So what's wrong with just keeping the things that we've advocated for a
long while, and not try to break source compatibility "just because".
Keeping the old names will make it a lot easier to maintain drivers that
do want to use them, and it means not having to change old drivers that do
the right thing.
So I vote for
local_irq_save(flags) - save and disable
local_irq_restore(flags) - restore
local_irq_disable() - disable
local_irq_enable() - enable
and that's it. Yes, the "calling convention" for local_irq_save() is
strange, but it makes it easier for some architectures, and other
architectures can just always make it
#define local_irq_save(flags) \
do { (flags) = arch_irq_save(); } while (0)
and it's not worth breaking existing practices over (besides, that's the
calling convention that "read_lock_irqsave()" also has, and I do _not_
want to change all of that _too_).
As to needing to do a save without a disable, show me where that really
matters..
I agree that we should get rid of __cli / __sti / __restore_flags /
__save_flags and company, but that is no excuse for breaking backwards
compatibility for stuff that has used the new interfaces
I really think that "local_" prefix is worth it. It makes people who are
used to, and work exclusively with, UP think twice about what the thing
actually does.
Linus
On Mon, 22 Jul 2002, Linus Torvalds wrote:
> I'd much rather keep the current "local_xxx" versions, since they
> clearly say that it's local to the CPU. Let's face it, people SHOULD NOT
> USE THESE!
okay, agreed.
> So I vote for
>
> local_irq_save(flags) - save and disable
> local_irq_restore(flags) - restore
> local_irq_disable() - disable
> local_irq_enable() - enable
i've added this one as well:
local_save_flags(flags) - save
a fair number of places want (and need) to use __save_flags(flags)-type of
functionality, without the irq-disabling side-effect.
But also, a number of places now do:
local_save_flags(flags);
local_irq_disable();
which should be:
local_irq_save(flags);
(these places will be simplified in an upcoming patch.)
The new __cli/__sti cleanup patch is at:
http://redhat.com/~mingo/remove-irqlock-patches/cli-sti-cleanup-2.5.27-B2
compiles, boots & works just fine on x86 UP and SMP.
Ingo
On Mon, 22 Jul 2002, Linus Torvalds wrote:
> As to needing to do a save without a disable, show me where that really
> matters..
Heres an interesting one;
void setup_APIC_timer(void * data)
{
[...]
__save_flags(flags);
__sti();
[...]
}
Regards,
Zwane
--
function.linuxpower.ca
Hi!
> > > It's not that we confuse flags with some other flag all that
> > > frequently that would necessiate some structure-based more abstract
> > > protection of these variables.
>
> are you sure type-checking is really needed? Sure people can mess up the
> flags variable, but 64-bit archs could do a sizeof at compile-time.
Yes, it is needed; type-checking can be easily implemented as
{
unsigned long foo;
(&foo == &arg);
}
-- that gives warning when arg is not unsigned long.
Pavel
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?