Andrey Savochkin wrote:
>
> Hello,
Hi, Andrey.
> On Thu, May 25, 2000 at 01:40:45PM +0100, Alan Cox wrote:
> > There is a really dumb fix perhaps ?
> >
> > wait_on_timers();
> >
> > That would map to a bh wait on 2.2 and the saner timer wait on 2.3.x
>
> Andrew, there are a lot of possible timer races,
Over a hundred, many of them fatal. Cheery, aren't I?
321 files use timers. I've performed a randomish audit of 35. Eight of
these are correct. http://www.uow.edu.au/~andrewm/linux/timers.txt
[ eepro100 should be using del_timer_sync() in speedo_close(),
and doesn't need to wrap del_timer/del_timer_sync in
#ifdef CONFIG_SMP in speedo_tx_timeout(), BTW :) ]
> but most of them can be very
> easy solved by the timer user. For example, setting some flag before
> del_timer call so that the handler will not reinsert the timer solves several
> kinds of the races.
Yes, it solves the timer-running-continuously-after-module-unload
problem, but not the handler-still-running-once-after-close problem. A
number of files do some clever spinlocking and trylocking to get around
this.
> BTW, this flag seems to be necessary with del_timer_sync
> anyway.
The present del_timer_sync takes care of that - it will detatch the
timer _after_ its handler has completed.
But del_timer_sync has a few problems, as I mentioned. Plus: if the
handler kfree's the timer (I've found a couple of these, BTW) then
del_timer_sync() will hang if slab poisoning is turned on.
> I like the Alan's proposal.
> Actually, with wait_on_timers() and reinsert protection flag we do not need
> del_timer_sync for most drivers. wait_on_timers() has larger overhead
> because it waits for all timers, but, I think, it's acceptable for device
> close call.
Sure. But del_timer() + wait_on_timers() + dont-re-add-flag ==
del_timer_sync()?
It helps a bit; we also need to do something about the fact that the
handler can be executing after the del_timer, even if it's the last time
it'll run. Only a tiny percentage of the clients of the kernel timers
are designed to cope with this.
I have completed the audit which Alan suggested (deadlocks caused by
making del_timer synchronous). I reviewed the 92 files which contain
the strings "del_timer|add_timer|mod_timer" and "spin". I patched the
following:
include/net/tcp.h
net/ipv4/tcp_timer.c
net/ipv4/igmp.c
net/ipv4/ip_fragment.c
net/decnet/dn_route.c
net/ipv6/addrconf.c
net/ipv6/reassembly.c
net/sunrpc/sched.c
net/sched/sch_generic.c
net/sched/estimator.c
drivers/net/slip.c
drivers/net/wan/syncppp.c
drivers/net/via-rhine.c
drivers/char/rtc.c
drivers/char/synclink.c
drivers/scsi/pci2220i.c
drivers/scsi/scsi_error.c
drivers/sound/sonicvibes.c
drivers/sound/cmpci.c
drivers/sound/esssolo1.c
drivers/video/tdfxfb.c
drivers/video/matrox/matroxfb_DAC1064.c
drivers/usb/uhci.c
drivers/ide/ide.c
drivers/ide/ide-disk.c
Detailed analysis at http://www.uow.edu.au/~andrewm/linux/spintimers.txt
Patch at http://www.uow.edu.au/~andrewm/linux/timer-deadlocks-7.patch
The problem is, in many of the above cases I simply replaced del_timer
with del_timer_async, so the races which some of the above files have
will remain in place. But the potential problems are identified and I
can now bug people :).
I expect the reason why these races haven't caused general disaster is
that they only affect SMP, and a lot of them are in code which doesn't
tend to be used on SMP machines: sound, old NICs, oddball devices, ISDN,
framebuffer,
etc:
- IDE is solid
- IPv4 is solid (I think - need to revisit)
- SCSI is solid (except for pci2220i.c)
- The popular NIC drivers are solid (except for close())
- IPv6 has problems...
We have a choice of:
1: Give del_timer synchronous semantics and patch the
above 25 files or
2: Fix del_timer_sync and then use it in ~250 files or
3: Throw up our hands and run away, as appeared to
happen in 2.1.x
I think you're implying that it should be option 2. That's OK - it's a
lot more work but at least I get to read most of the kernel source, fix
other bugs as they pop up and get to meet lots of maintainers :)
But it's a lot more work and options 1 and 2 are equivalent. The only
difference is in the names of the functions!
-- -akpm-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Wed May 31 2000 - 21:00:16 EST