Messages in this thread | | | Date | Fri, 20 Apr 2001 16:45:32 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: x86 rwsem in 2.4.4pre[234] are still buggy [was Re: rwsem benchmarks [Re: generic rwsem [Re: Alpha "process table hang"]]] |
| |
On Fri, 20 Apr 2001, Andrea Arcangeli wrote: > > While dropping the list_empty check to speed up the fast path I faced the same > complexity of the 2.4.4pre4 lib/rwsem.c and so before reinventing the wheel I > read how the problem was solved in 2.4.4pre4.
I would suggest the following:
- the generic semaphores should use the lock that already exists in the wait-queue as the semaphore spinlock.
- the generic semaphores should _not_ drop the lock. Right now it drops the semaphore lock when it goes into the slow path, only to re-aquire it. This is due to bad interfacing with the generic slow-path routines.
I suspect that this lock-drop is why Andrea sees problems with the generic semaphores. The changes to "count" and "sleeper" aren't actually atomic, because we don't hold the lock over them all. And re-using the lock means that we don't need the two levels of spinlocking for adding ourselves to the wait queue. Easily done by just moving the locking _out_ of the wait-queue helper functions, no?
- the generic semaphores are entirely out-of-line, and are just declared universally as regular FASTCALL() functions.
The fast-path x86 code looks ok to me. The debugging stuff makes it less readable than it should be, I suspect, and is probably not worth it at this stage. The users of rw-semaphores are so well-defined (and so well debugged) that the debugging code only makes the code harder to follow right now.
Comments? Andrea? Your patches have looked ok, but I absoutely refuse to see the non-inlined fast-path for reasonable x86 hardware..
Linus
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |