In kernel 2.4.18 or so, if two Token Ring boards are used in a SMP computer, a lock-up happens. IBM asked me to look at a patch by Denis J. Barlow which was said to fix the lockup. The patch was too big and touched too much code, so I figured what it did to locking and wrote a small patchlet which fixed it. Today I received a mail from Arnaldo (the maintainer of weird networking stacks) which said that someone else did exactly the same thing for 2.5 already, so he is taking the patch and sending it to Marcelo. In a year or so we'll know if it actually worked...
I am a little sad to see all the rest of Denis' patch to fall by wayside, but he is stubbornly refusing to split it into manageable chunks.
Another moral of the story is: Avoid exotic locks, such as spin_lock_bh(). The "my" patch simply replaces spin_lock_bh with spin_lock_irqsave. Why did the author use spin_lock_bh in the first place? I place the blame squarely on premature optimization.
Somewhere in August, Robert Love wrote an article in Linux Journal about Linux locks. It was an honest article, which listed all lock varieties and even tried to explain how to use them. When I read it, I thought it was a wonderful article; the only omission I noticed was that he did not mention that semaphore down() cannot be used in an interrupt. But guess what, about a month ago, Grant Edwards, a comp.os.linux.development.system regular, made a posting where he told he was going to use exotic locking. He wrote that he went through RML's article, and so, everything was going to be ok, because he'll use those correctly. I had to step in to direct him to sane locking. Of course, his code would be correct - in that particular version of the kernel. One step left, one step right, and it bitrots. Just like Token Ring did.
I extend the KISS phylosophy way beyond spin_lock_bh, into the realm of reader-writer locks. They can improve performance of contented locks a lot, but they are rife with pitfalls. In this regard Bill Irwin grieves me especially, because he advocates using reader-writer locks by default while being sharp and respected. He probably corrupted minds of hundreds of newbie driver writers. He does not understand that they are not ready for the sophisticated stuff.
Newbies need something simple instead, which may not be the most scalable, but working robustly, like a set of these "Zaitcev's Commandments":
- If you are in a process context (any syscall) and want to lock other process out, use a semaphore. You can take a semaphore and sleep (copy_from_user or kmalloc(x,GFP_KERNEL)).
- Otherwise (== data can be touched in an interrupt), use spin_lock_irqsave and spin_unlock_irqrestore.
- Avoid holding spinlock for more than 5 lines of code and across any function call (except accessors like readb).
That would be it. Just keep it simple. Any of these rules can be broken under some circomstances, but the problem is that newbies cannot judge adequately. This is why they are "Commandments", to prevent them from confusing themselves.