Messages in this thread |  | | Date | Thu, 7 Sep 2000 09:00:29 -0700 | From | "Leonard N. Zubkoff" <> | Subject | Re: DAC960 SMP deadlock fix |
| |
Date: Thu, 7 Sep 2000 16:50:51 +0200 (CEST) From: Andrea Arcangeli <andrea@suse.de> X-Sender: andrea@inspiron.random cc: linux-kernel@vger.kernel.org, Alan Cox <alan@lxorguk.ukuu.org.uk> X-GnuPG-Key-URL: http://e-mind.com/~andrea/aa.gnupg.asc
Hi Leonard,
this night I (hopefully) finally spotted and fixed a longstanding deadlock that was hitting us on heavily loaded server running the DAC960.
The bug is present also in the earlier 2.2.x and 2.4.x and it's _not_ been introduced with the DAC960 updates in 2.2.17.
In 2.4.x the SMP deadlock can't trigger because of two reasons:
1) the waitqueue uses a per-queue spinlock and the driver always wakeup the waitqueue with the io_request_lock acquired 2) the waitqueue lock in 2.4.x at the moment is a spinlock_t (thus there isn't a __cli-less read_lock (no-irqsave) wake_up).
The bug triggers in 2.2.x because the waitqueue_lock is shard by all waitqueues and the wake_up uses a read_lock (that can be interrupted by irq handlers that can later spin on the io_request_lock).
The SMP locking rule that was not enforced by the driver and that was causing the deadlock is:
"never run add_wait_queue with the io_request_lock acquired"
The reason of the rule is that the irq_request_lock can be acquired by interrupt handlers as well.
The patch with a description of the problem is here:
ftp://ftp.us.kernel.org/pub/linux/kernel/people/andrea/patches/v2.2/2.2.18pre2/patches/v2.2/2.2.18pre2/DAC960-SMP-lock-inversion-1
I recommend adding the above fix to 2.2.18pre3.
Andrea
I tried retrieving that file but was unsuccessful; is that the correct URL?
As for the comments above, I expect you are right. I had originally used a much simpler sleep_on implementation until Manfred Spraul pointed out an SMP problem with that and suggested the alternative sequence the driver now uses. Looking at DAC960_WaitForRequest, we have:
WaitQueue_T WaitQueueEntry = { current, NULL }; add_wait_queue(&Controller->CommandWaitQueue, &WaitQueueEntry); current->state = TASK_UNINTERRUPTIBLE; spin_unlock(&io_request_lock); schedule(); current->state = TASK_RUNNING; remove_wait_queue(&Controller->CommandWaitQueue, &WaitQueueEntry); spin_lock_irq(&io_request_lock);
Is the fix simply moving the spin_unlock right before the call to add_wait_queue?
I should be able to release a new driver very shortly with such a fix; I just have one or two other minor nits I'm finishing up.
Leonard - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org Please read the FAQ at http://www.tux.org/lkml/
|  |