On Sat, 8 Nov 2003, Morten Johansen wrote:
Scott Long wrote:
Bruce Evans wrote:
[... possibly too much trimmed]
The problem here is that the keyboard controller driver tries to be too smart. If it detects that the hardware FIFO is full, it'll drain it into a per-softc, per-function ring buffer. So having psm(4) just directly read the hardware is insufficient in this scheme.
What is the per-function part? (I'm not very familar with psm, but once understood simpler versions of the keyboard driver.) Several layers of buffering might not be too bad for slow devices. The i/o times tend to dominate unless you do silly things like a context switch to move each character from one buffer to other, and even that can be fast enough (I believe it is normal for interactive input on ptys; then there's often a remote IPC or two per character as well).
- it sometimes calls the DELAY() function, which is not permitted in fast interrupt handlers since apart from locking issues, fast interrupt handlers are not permitted to busy-wait.
Again, the keyboard controller driver is too smart for its own good. To summarize:
read_aux_data_no_wait() { Does softc->aux ring buffer contain data? return ring buffer data
Check the status register Is the keyboard fifo full? DELAY(7us) read keyboard fifo into softc->kbd ring buffer Check the status register
Is the aux fifo full? DELAY(7us) return aux fifo data }
So you can wind up stalling for 14us in there, presumably because you cannot read the status and data registers back-to-back without a delay. I don't have the atkbd spec handy so I'm not sure how to optimize this. Do you really need to check the status register before reading the data register?
At least it's a bounded delay. I believe such delays are required for some layers of the keyboard. Perhaps only for the keyboard (old hardware only?) and not for the keyboard controller or the mouse.
Many of the complications for fast interrupt handlers shouldn't be needed in psm. Just make psmintr() INTR_MPSAFE.
I believe that the previous poster actually tried making it INTR_MPSAFE, but didn't see a measurable benefit because the latency of scheduling the ithread is still unacceptable.
That is 100% correct. In the meantime I have taken your's and Bruce's advice and rearranged the interrupt handler to look like this:
mtx_lock(&sc->input_mtx);
Er, this is reasonable for INTR_MPSAFE but not for INTR_FAST. mtx_lock() is a "sleep" lock so it cannot be used in fast interrupt handlers. mtx_lock_spin() must be used. (My version doesn't permit use of mtx_lock_spin() either; more primitive locking must be used.)
while((c = read_aux_data_no_wait(sc->kbdc)) != -1) {
This is probably INTR_FAST-safe enough in practice.
sc->input_queue.buf[sc->input_queue.tail] = c; if ((++ sc->input_queue.tail) >= PSM_BUFSIZE) sc->input_queue.tail = 0; count = (++ sc->input_queue.count); } mtx_unlock(&sc->input_mtx);
The locking for the queue seems to be correct except this should operate on a spinlock too.
if (count >= sc->mode.packetsize) taskqueue_enqueue(taskqueue_swi_giant, &sc->psm_task);
taskqueue_enqueue() can only be used in non-fast interrupt handlers. taskqueue_enqueue_fast() must be used in fast interrupt handlers (except in my version, it is not permitted so it shouldn't exist). Note that the spinlock/fast versions can be used for normal interrupt handlers too, so not much more code is needed to support handlers whose fastness is dynamically configured.
Yes, I have submitted a PR (kern/59067), with an INTR_FAST version that uses spinlocks and taskqueue_fast.
You can find it here if you have time to look at it:
http://dsm.oslonett.no/patch-psm-fast
I would appreciate comments on it's correctness.
And it works, but having it INTR_MPSAFE still does NOT help my problem. It looks to me like data is getting lost because the interrupt handler is unable to read it before it's gone, and the driver gets out of sync, and has to reset itself. However it now takes a few more tries to provoke the problem, so something seems to have improved somewhere.
This is a bit surprising. There are still so few INTR_MPSAFE handlers that there aren't many system activities that get in the way of running the INTR_MPSAFE ones. Shared interrupts prevent running of a handler while other handlers on the same interrupt are running, and the mouse interrupt is often shared, but if it is shared then it couldn't be fast until recently and still can't be fast unless all the other handlers on it are fast.
Bruce
It seems odd that it should be necessary to have it INTR_FAST. But somehow it is, on my system.
Morten
_______________________________________________ [EMAIL PROTECTED] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "[EMAIL PROTECTED]"
