ns8250_drain: Drain without DELAY first

[cperciva]

Summary

In virtual machines with virtual UARTs which have fictitious baud
rates, it may be possible to drain the receive queue very quickly,
without needing to DELAY after each character. Attempt to read
(and discard) the receive queue as fast as possible, stopping for
a DELAY only when LSR_RXRDY is no longer asserted; assume that we
have finished draining the queue when LSR_RXRDY is asserted both
before and after a DELAY.

This speeds up the boot process in FreeBSD/Firecracker by 27 ms.

[jrtc27]

sys/dev/uart/uart_dev_ns8250.c

165–167

Perhaps this should be restyled to look more like the updated receiver case?

187–188

I think you want a limit > 0 in here?

188

No space, though style(9) says "Do not use ! for tests unless it is a boolean" so better to compare against 0 anyway

192

You need another uart_barrier here I believe?

[cperciva]

Fix missing loop condition and style issues.

[cperciva]

sys/dev/uart/uart_dev_ns8250.c

165–167

My general inclination is to avoid making style changes at the same time as functional changes, but I"m happy to rewrite this code if you'd prefer that I do everything.

187–188

Thanks, fixed.

188

Thanks, fixed.

192

Do we need one? If LSR_RXRDY is set, I think we should be able to read data without needing another uart_barrier first?

[cperciva]

@jrtc27 Is this version ok?

[jrtc27]

Hm, doesn't this change behaviour in the case LSR_RXRDY isn't asserted the first time round the loop? Previously we would skip the entire loop, but now you go through the delay-and-check-it's-really-zero path.

sys/dev/uart/uart_dev_ns8250.c

184

I think you mean is asserted?

[cperciva]

Fix comment re asserted vs non-asserted.

[cperciva]

In D36184#822609, @jrtc27 wrote:

Hm, doesn't this change behaviour in the case LSR_RXRDY isn't asserted the first time round the loop? Previously we would skip the entire loop, but now you go through the delay-and-check-it's-really-zero path.

Good catch. Does it matter? I can change this but it makes the code uglier...

sys/dev/uart/uart_dev_ns8250.c

184

Thanks, fixed (and also reworded "!LSR_RXRDY is asserted" to avoid the double negative).

[jrtc27]

In D36184#823279, @cperciva wrote:

In D36184#822609, @jrtc27 wrote:

Hm, doesn't this change behaviour in the case LSR_RXRDY isn't asserted the first time round the loop? Previously we would skip the entire loop, but now you go through the delay-and-check-it's-really-zero path.

Good catch. Does it matter? I can change this but it makes the code uglier...

It needlessly slows down the common case (note that we drain the receiver during probe, so every 8250 will pass through here at least once).

[cperciva]

Completely rework the change. Rather than "only count !LSR_RXRDY
as real if we see it before and after a DELAY", the new code goes
with "if we decide to read a character, keep on reading without
delays as long as LSR_RXRDY".

This restores the previous behaviour of "if !LSR_RXRDY when we
start, we don't do any DELAYs" and actually reduces the size of
the diff.

[cperciva]

Add uart_barrier which I accidentally removed.

[cperciva]

It needlessly slows down the common case (note that we drain the receiver during probe, so every 8250 will pass through here at least once).

Good catch. I've reworked the patch to fix this.