diff --git a/docs/architecture/networking.md b/docs/architecture/networking.md index 9a1dc12..10417cc 100644 --- a/docs/architecture/networking.md +++ b/docs/architecture/networking.md @@ -108,8 +108,30 @@ received frames and runs its timers. **A thread rather than the interrupt handler, and the reason is the lock:** a call into lwIP can reach the driver, the driver can sleep, and an interrupt -handler cannot. So the interrupt keeps doing what it always did — clear the -cause register — and this thread polls. +handler cannot. So the frame is always *pulled*, by this thread, through +`.receive`. + +The interrupt's only job is to end the thread's wait early. `netdev_rx_signal()` +is what a driver calls from its handler; it delivers no frame and reads no ring. +Without it a frame that arrived just after a poll sat there until the next clock +tick — up to 10 ms, and the largest fixed latency in the system. + +**The wait stays on the timer tick rather than moving to a channel of its own**, +and that is the design rather than an accident. lwIP's timers must still run when +the link is silent, so the clock has to remain a floor under the wait, and +`sleep()` cannot wait on two things at once. Joining the tick's existing wakeup +costs nothing and leaves one wait with one predicate. + +The flag lives under `tickslock` for the same reason it does not get a lock of +its own: `sleep()` releases `tickslock` atomically, so a driver setting the flag +and waking under that lock cannot lose the wakeup by arriving between the +predicate test and the sleep. A private lock would not be held across the sleep, +and that window would be back. + +`/proc/netif` reports `wake_rx` and `wake_tick` so this is observable rather +than assumed — if `wake_rx` stays at zero while frames arrive, the interrupt is +not reaching the stack and the thread is running on the clock exactly as it used +to. Measured across three pings: `rx_frames` and `wake_rx` both went 2 → 6. `netinit()` runs after `sinit()` because `kthread_create` needs a usable process table, and it returns immediately: everything happens in that thread, which @@ -237,11 +259,6 @@ be fixed before `recvfrom` could report a source address at all. - **No resolver reachable from userland.** lwIP's DNS client is compiled but nothing exposes it, so `ping` takes a dotted quad. `localhost` is special-cased in the program rather than resolved. -- **Receive is polled.** The network thread looks at the NIC every timer tick - when the link is idle, so an arriving frame waits up to 10 ms. Waking the - thread from the e1000 interrupt is the real fix, and was deliberately not the - first version: a receive path woken by an interrupt is much harder to debug - than one that is asked. - **No routing table.** With two interfaces up, the first is the default route and the second is reachable only by an address on its own subnet. That is a rule rather than an accident, and it is not routing. diff --git a/docs/plan/1.0.md b/docs/plan/1.0.md index 999639f..ca54fd2 100644 --- a/docs/plan/1.0.md +++ b/docs/plan/1.0.md @@ -101,9 +101,13 @@ These are the galling ones — the replacement exists, is tested, and is unused. `text.c` and `ttf.c` both exist and work. The console still indexes `font_data[256][16]`. - [ ] **Load a font from disk** and point the console at it. -- [ ] **Interrupt-driven receive.** The network thread polls the NIC every timer - tick, so a frame waits up to 10 ms. Waking it from the e1000 interrupt was - deliberately not the first version; it is time. +- [x] ~~**Interrupt-driven receive.**~~ The e1000's handler calls + `netdev_rx_signal()`, which ends the network thread's wait early; the frame + is still *pulled* by the thread, because an interrupt handler can neither + sleep on the buffer cache nor take the lwIP lock. The wait stays on the + tick so lwIP's timers keep their floor. Proven rather than assumed: + `/proc/netif` gained `wake_rx`/`wake_tick`, and across three pings + `rx_frames` and `wake_rx` both went 2 -> 6. **Structural, because everything after this compounds on it:** diff --git a/docs/plan/roadmap.md b/docs/plan/roadmap.md index 39f5e2d..b9a25ed 100644 --- a/docs/plan/roadmap.md +++ b/docs/plan/roadmap.md @@ -51,11 +51,6 @@ Three things the stack owed on the day it landed, all still owed: - **No routing table.** With two interfaces up, the first is the default route and the second is reachable only by an address on its own subnet. That is a rule rather than an accident, and it is not routing. -- **Receive is polled.** The network thread looks at the NIC every timer tick - when the link is idle, so an arriving frame waits up to 10 ms. Waking that - thread from the e1000 interrupt is the real fix, and was deliberately not the - first version: a receive path woken by an interrupt is much harder to debug - than one that is asked. `setsockopt` returning 0 for everything is in [foundations.md](foundations.md) instead, because the Wayland work needs it. diff --git a/include/defs.h b/include/defs.h index b985e0c..879f8bc 100644 --- a/include/defs.h +++ b/include/defs.h @@ -526,6 +526,7 @@ int diskwrite(struct inode*, uint64, char*, int); void netinit(void); int net_is_up(void); int net_is_ready(void); +void net_wake_counts(uint64 *rx, uint64 *tick); int net_info(char *buf, int cap); // net/lwip/ -- the port layer and the Ethernet interfaces, called only from diff --git a/include/netdev.h b/include/netdev.h index c59ad0f..bd041db 100644 --- a/include/netdev.h +++ b/include/netdev.h @@ -52,6 +52,17 @@ int netdev_receive(struct device *, void *out, int cap); const unsigned char *netdev_mac(struct device *); int netdev_link_up(struct device *); +// Tell the network stack a frame has arrived, from a driver's interrupt handler. +// +// The driver does not deliver the frame here and must not: the stack pulls with +// .receive, from a thread that can sleep, and an interrupt handler cannot. This +// only ends the thread's wait early, so the frame is taken now rather than at +// the next clock tick -- which is up to 10 ms away. +// +// Safe to call with interrupts off; see the note above netdev_rx_signal in +// kernel/net/net.c for why the wait is on the tick and not a channel of its own. +void netdev_rx_signal(void); + // Send an ARP request by hand and wait for the reply. `echo n > /proc/sysrq`. // // It proves the whole driver path at once -- descriptor formats, the DD diff --git a/kernel/drivers/net/e1000/e1000.c b/kernel/drivers/net/e1000/e1000.c index ff6b251..b39452e 100644 --- a/kernel/drivers/net/e1000/e1000.c +++ b/kernel/drivers/net/e1000/e1000.c @@ -250,7 +250,18 @@ e1000_intr(void *arg) // Reading ICR clears it. Not reading it leaves the cause asserted, and with a // level-triggered PCI interrupt that means the handler is re-entered forever. uint32 cause = rd(e, E1000_ICR); - (void)cause; + + // Nothing is read from the ring here. The frame is pulled by the network + // thread through .receive, which may sleep on the buffer cache and on the + // lwIP lock; an interrupt handler may do neither. All this does is end that + // thread's wait, so the frame is taken now instead of up to 10 ms later. + // + // RXT0 is the receive timer expiring -- a frame, or a burst, has landed. + // RXDMT0 is the ring falling below its minimum threshold, which means frames + // are arriving faster than they are being taken and is exactly when waiting + // for the clock hurts most. + if (cause & (E1000_ICR_RXT0 | E1000_ICR_RXDMT0)) + netdev_rx_signal(); } static int diff --git a/kernel/net/net.c b/kernel/net/net.c index ef95bad..6008f7b 100644 --- a/kernel/net/net.c +++ b/kernel/net/net.c @@ -49,6 +49,73 @@ wait_tick(void) release(&tickslock); } +// --------------------------------------------------------------------------- +// Waking on a received frame +// +// The thread used to wait for the clock and nothing else, so a frame arriving +// just after a poll sat in the ring for up to 10 ms. That was deliberate at the +// time -- a receive path woken by an interrupt is much harder to debug than one +// that is asked, and the stack had to be proven first -- and it is now the +// largest fixed latency in the system. +// +// The wait stays on `ticks`, rather than moving to a channel of its own, and +// that is the whole design. lwIP's timers must still run when the link is +// silent, so the clock has to remain a floor under the wait; a second channel +// would mean sleeping on two things at once, which sleep() cannot do. Joining +// the tick's existing wakeup costs nothing and keeps one wait, one predicate. +// +// `rx_signal` lives under tickslock for the same reason it is not its own lock: +// sleep() releases tickslock atomically, so a driver that sets the flag and +// wakes under that lock cannot lose the wakeup by arriving in the window +// between the predicate test and the sleep. A private lock would not be held +// across the sleep, and that window would be back. +// +// Called from interrupt context. acquire() disables interrupts, so a CPU +// holding tickslock cannot be interrupted into this; another CPU spins briefly. +// That is the same arrangement the timer handler itself uses. +// --------------------------------------------------------------------------- + +static int rx_signal; + +// Counted so the mechanism is observable rather than assumed: if `rx` stays +// zero while traffic flows, the interrupt is not arriving and the thread is +// running on the clock exactly as it did before. +static uint64 wake_rx, wake_tick; + +void +netdev_rx_signal(void) +{ + acquire(&tickslock); + rx_signal = 1; + wakeup(&ticks); + release(&tickslock); +} + +void +net_wake_counts(uint64 *rx, uint64 *tick) +{ + if (rx) *rx = wake_rx; + if (tick) *tick = wake_tick; +} + +// Sleep until there is something to do: a frame has been signalled, or the +// clock has moved and lwIP's timers are due. +static void +wait_work(void) +{ + acquire(&tickslock); + uint start = ticks; + while (ticks == start && !rx_signal) + sleep(&ticks, &tickslock); + if (rx_signal) { + wake_rx++; + rx_signal = 0; + } else { + wake_tick++; + } + release(&tickslock); +} + static int net_started; // Set once DHCP has bound, so `ifconfig`-ish output and /proc/net have @@ -160,7 +227,7 @@ net_thread(void) // Busy while there is traffic, idle otherwise. Sleeping even 1 ms with // frames waiting would cap throughput at one burst per tick. if (moved == 0) - wait_tick(); + wait_work(); } } @@ -312,6 +379,21 @@ net_info(char *buf, int cap) buf[len++] = '\n'; } + // How the network thread is being woken, for the whole stack rather than + // per interface. `wake_rx` staying at zero while frames arrive means the + // receive interrupt is not reaching us and the thread is running on the + // clock exactly as it did before -- which works, and is 10 ms slower. + { + uint64 wrx, wtick; + net_wake_counts(&wrx, &wtick); + len += put_field(buf + len, "wake_rx"); + len += put_u(buf + len, wrx); + buf[len++] = '\n'; + len += put_field(buf + len, "wake_tick"); + len += put_u(buf + len, wtick); + buf[len++] = '\n'; + } + return len; }