net: a frame wakes the stack, instead of waiting for the clock
The network thread waited on the timer and nothing else, so a frame that landed just after a poll sat in the ring for up to 10 ms -- the largest fixed latency in the system. That was deliberate when the stack was new (a receive path woken by an interrupt is much harder to debug than one that is asked) and there is now a working stack to compare against, which is exactly when it becomes safe to do. The frame is still *pulled* by the thread through .receive. It has to be: a call into lwIP can reach the driver, the driver can sleep on the buffer cache, and an interrupt handler may do neither. netdev_rx_signal() delivers nothing and reads no ring -- it only ends the wait early. The wait stays on the timer tick rather than moving to a channel of its own, and that is the design. 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. rx_signal lives under tickslock for the same reason it gets no 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. Calling it from interrupt context is safe on the same terms the timer handler already relies on -- acquire() disables interrupts, so a CPU holding tickslock cannot be interrupted into it. The e1000 signals on RXT0 (a frame or burst has landed) and RXDMT0 (the ring has fallen below its threshold, which is when waiting for the clock hurts most). Measured rather than asserted: /proc/netif reports wake_rx and wake_tick, and across three pings rx_frames and wake_rx both went 2 -> 6 -- every received frame woke the thread through the interrupt. wake_tick keeps advancing as the idle floor, which is what lwIP's timers need. socktest still passes and ping is still 0% loss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2
This commit is contained in:
parent
e80f7b287b
commit
3034efd300
7 changed files with 138 additions and 17 deletions
|
|
@ -108,8 +108,30 @@ received frames and runs its timers.
|
||||||
|
|
||||||
**A thread rather than the interrupt handler, and the reason is the lock:** a
|
**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
|
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
|
handler cannot. So the frame is always *pulled*, by this thread, through
|
||||||
cause register — and this thread polls.
|
`.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
|
`netinit()` runs after `sinit()` because `kthread_create` needs a usable process
|
||||||
table, and it returns immediately: everything happens in that thread, which
|
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
|
- **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
|
nothing exposes it, so `ping` takes a dotted quad. `localhost` is special-cased
|
||||||
in the program rather than resolved.
|
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
|
- **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
|
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.
|
rule rather than an accident, and it is not routing.
|
||||||
|
|
|
||||||
|
|
@ -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
|
`text.c` and `ttf.c` both exist and work. The console still indexes
|
||||||
`font_data[256][16]`.
|
`font_data[256][16]`.
|
||||||
- [ ] **Load a font from disk** and point the console at it.
|
- [ ] **Load a font from disk** and point the console at it.
|
||||||
- [ ] **Interrupt-driven receive.** The network thread polls the NIC every timer
|
- [x] ~~**Interrupt-driven receive.**~~ The e1000's handler calls
|
||||||
tick, so a frame waits up to 10 ms. Waking it from the e1000 interrupt was
|
`netdev_rx_signal()`, which ends the network thread's wait early; the frame
|
||||||
deliberately not the first version; it is time.
|
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:**
|
**Structural, because everything after this compounds on it:**
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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
|
- **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
|
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.
|
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
|
`setsockopt` returning 0 for everything is in
|
||||||
[foundations.md](foundations.md) instead, because the Wayland work needs it.
|
[foundations.md](foundations.md) instead, because the Wayland work needs it.
|
||||||
|
|
|
||||||
|
|
@ -526,6 +526,7 @@ int diskwrite(struct inode*, uint64, char*, int);
|
||||||
void netinit(void);
|
void netinit(void);
|
||||||
int net_is_up(void);
|
int net_is_up(void);
|
||||||
int net_is_ready(void);
|
int net_is_ready(void);
|
||||||
|
void net_wake_counts(uint64 *rx, uint64 *tick);
|
||||||
int net_info(char *buf, int cap);
|
int net_info(char *buf, int cap);
|
||||||
|
|
||||||
// net/lwip/ -- the port layer and the Ethernet interfaces, called only from
|
// net/lwip/ -- the port layer and the Ethernet interfaces, called only from
|
||||||
|
|
|
||||||
|
|
@ -52,6 +52,17 @@ int netdev_receive(struct device *, void *out, int cap);
|
||||||
const unsigned char *netdev_mac(struct device *);
|
const unsigned char *netdev_mac(struct device *);
|
||||||
int netdev_link_up(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`.
|
// 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
|
// It proves the whole driver path at once -- descriptor formats, the DD
|
||||||
|
|
|
||||||
|
|
@ -250,7 +250,18 @@ e1000_intr(void *arg)
|
||||||
// Reading ICR clears it. Not reading it leaves the cause asserted, and with a
|
// 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.
|
// level-triggered PCI interrupt that means the handler is re-entered forever.
|
||||||
uint32 cause = rd(e, E1000_ICR);
|
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
|
static int
|
||||||
|
|
|
||||||
|
|
@ -49,6 +49,73 @@ wait_tick(void)
|
||||||
release(&tickslock);
|
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;
|
static int net_started;
|
||||||
|
|
||||||
// Set once DHCP has bound, so `ifconfig`-ish output and /proc/net have
|
// 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
|
// Busy while there is traffic, idle otherwise. Sleeping even 1 ms with
|
||||||
// frames waiting would cap throughput at one burst per tick.
|
// frames waiting would cap throughput at one burst per tick.
|
||||||
if (moved == 0)
|
if (moved == 0)
|
||||||
wait_tick();
|
wait_work();
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -312,6 +379,21 @@ net_info(char *buf, int cap)
|
||||||
buf[len++] = '\n';
|
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;
|
return len;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue