diff --git a/include/defs.h b/include/defs.h index 8968f9f..1079024 100644 --- a/include/defs.h +++ b/include/defs.h @@ -387,6 +387,7 @@ int sock_domain(struct socket*); // net/inet.c -- AF_INET on lwIP's raw API. Called only from net/unix.c, which // is where the per-domain dispatch lives. +void inet_init(void); void* inet_new(int type); int inet_bind(void *h, uint addr, ushort port); int inet_listen(void *h, int backlog); diff --git a/kernel/net/inet.c b/kernel/net/inet.c index 19a332c..67233ff 100644 --- a/kernel/net/inet.c +++ b/kernel/net/inet.c @@ -18,19 +18,34 @@ // because there is no second writer. // // --------------------------------------------------------------------------- -// Blocking, and what it costs today +// Blocking, and how a waiter is woken // -// A blocking read waits by polling on the timer tick rather than by sleeping on -// a channel a callback wakes. The reason is a lock mismatch: sleep() wants a -// spinlock, the lwIP lock is a sleeplock (it must be, because a call into lwIP -// can reach the driver and the driver can sleep), and interleaving the two -// correctly is a genuinely error-prone piece of code. +// A blocking call sleeps until an lwIP callback says something changed. It used +// to wake on every timer tick and look, which cost a wakeup per socket per +// 10 ms and answered up to 10 ms late even when the data was already there. // -// The cost is up to 10 ms of latency on an operation that has to wait -- and -// none at all on one that does not, which is the case that matters for -// throughput: a bulk transfer only blocks when the buffer is genuinely empty. -// Replacing this with a proper sleep/wakeup is worth doing and is deliberately -// not the first version. +// The obstacle was a lock mismatch, and it is worth writing down because it is +// the reason this is not simply sleep(&socket, &lwip_lock): sleep() wants a +// spinlock, and the lwIP lock is a sleeplock -- it must be, because a call into +// lwIP can reach the driver and the driver can sleep. A waiter cannot hold the +// lwIP lock while it sleeps either, or the network thread could never deliver +// the very thing it is waiting for. +// +// So the wait is on a counter with a spinlock of its own: +// +// reader: lwip_lock; check condition; s = net_seq(); lwip_unlock; +// net_wait(s) +// waker: (in a callback, lwIP lock held) net_wake() +// +// No wakeup can be lost. `s` is read while the reader still holds the lwIP +// lock, so anything that changes the condition must do so after that -- and +// therefore bumps the counter after the read. net_wait re-checks the counter +// under its spinlock before sleeping, sees it has moved, and does not sleep. +// +// One counter and one channel for every inet socket, rather than one each. At +// sixteen sockets the cost of waking all of them is a handful of instructions +// in the ones with nothing to do, and a per-socket channel would have to be +// torn down carefully in close() against callbacks that can still arrive. #include "types.h" #include "defs.h" @@ -79,6 +94,55 @@ struct inet_sock { static struct inet_sock itab[NINET]; +/* ------------------------------------------------------------------ */ +/* Waiting. See the note on blocking at the top of this file. */ +/* ------------------------------------------------------------------ */ + +static struct spinlock netwait_lock; +static uint netwait_seq; + +void +inet_init(void) +{ + initlock(&netwait_lock, "netwait"); +} + +// The current wakeup count. Read by a caller that has just checked its +// condition and is still holding the lwIP lock. +static uint +net_seq(void) +{ + uint s; + + acquire(&netwait_lock); + s = netwait_seq; + release(&netwait_lock); + return s; +} + +// Sleep until the count moves past `seen`, or this process is killed. kill() +// makes a sleeping process runnable, so the loop re-checks and leaves. +static void +net_wait(uint seen) +{ + acquire(&netwait_lock); + while (netwait_seq == seen && !myproc()->killed) + sleep(&netwait_seq, &netwait_lock); + release(&netwait_lock); +} + +// Something a waiter might care about has happened. Called from the lwIP +// callbacks, which run in the network thread with the lwIP lock held -- taking +// a spinlock there is fine, since nothing between acquire and release sleeps. +static void +net_wake(void) +{ + acquire(&netwait_lock); + netwait_seq++; + release(&netwait_lock); + wakeup(&netwait_seq); +} + /* ------------------------------------------------------------------ */ /* The receive ring. Callers hold the lwIP lock. */ /* ------------------------------------------------------------------ */ @@ -129,6 +193,9 @@ tcp_fail(struct inet_sock *is) // The pcb is already freed by lwIP when err_cb fires; keeping the pointer // would leave a dangling handle that close() would then free a second time. is->tpcb = 0; + // A failure is as much of a wakeup as data: whoever is blocked in connect, + // read or write is now waiting for something that will never arrive. + net_wake(); } static void @@ -156,6 +223,7 @@ inet_recv_cb(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err) // until the buffer drains, and only then does read() report end of file. if (p == NULL) { is->peer_closed = 1; + net_wake(); return ERR_OK; } @@ -171,6 +239,17 @@ inet_recv_cb(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err) // Only now is the data ours, so only now may the window reopen. tcp_recved(pcb, p->tot_len); pbuf_free(p); + net_wake(); + return ERR_OK; +} + +// An acknowledgement freed space in the send buffer. Nothing reads `len`: the +// only waiter is a write that could not fit, and it re-reads tcp_sndbuf itself. +static err_t +inet_sent_cb(void *arg, struct tcp_pcb *pcb, u16_t len) +{ + (void)arg; (void)pcb; (void)len; + net_wake(); return ERR_OK; } @@ -188,6 +267,7 @@ inet_connected_cb(void *arg, struct tcp_pcb *pcb, err_t err) } is->connecting = 0; is->connected = 1; + net_wake(); return ERR_OK; } @@ -212,8 +292,10 @@ inet_accept_cb(void *arg, struct tcp_pcb *newpcb, err_t err) tcp_arg(newpcb, is); tcp_recv(newpcb, inet_recv_cb); tcp_err(newpcb, inet_err_cb); + tcp_sent(newpcb, inet_sent_cb); ls->acceptq[ls->naccept++] = is; + net_wake(); return ERR_OK; } @@ -235,6 +317,7 @@ inet_udp_recv_cb(void *arg, struct udp_pcb *pcb, struct pbuf *p, for (struct pbuf *q = p; q != NULL; q = q->next) rx_put(is, (const char *)q->payload, q->len); pbuf_free(p); + net_wake(); } /* ------------------------------------------------------------------ */ @@ -255,17 +338,6 @@ inet_alloc(int type) return 0; } -// Wait one timer tick. See the note at the top on why waiting is a poll. -static void -inet_wait(void) -{ - acquire(&tickslock); - uint start = ticks; - while (ticks == start) - sleep(&ticks, &tickslock); - release(&tickslock); -} - /* ------------------------------------------------------------------ */ /* The interface kernel/net/unix.c dispatches to */ /* ------------------------------------------------------------------ */ @@ -295,6 +367,7 @@ inet_new(int type) tcp_arg(is->tpcb, is); tcp_err(is->tpcb, inet_err_cb); tcp_recv(is->tpcb, inet_recv_cb); + tcp_sent(is->tpcb, inet_sent_cb); } else { is->upcb = udp_new(); if (is->upcb == 0) { @@ -373,11 +446,14 @@ inet_accept(void *h) lwip_sys_unlock(); return is; } + // Taken while the lwIP lock is still held, so no connection can arrive + // between the check above and the sleep below without moving it. + uint seen = net_seq(); lwip_sys_unlock(); if (myproc()->killed) return 0; - inet_wait(); + net_wait(seen); } } @@ -409,7 +485,8 @@ inet_connect(void *h, uint32 addr, uint16 port) for (;;) { lwip_sys_lock(); - int done = is->connected, failed = is->conn_failed; + int done = is->connected, failed = is->conn_failed; + uint seen = net_seq(); lwip_sys_unlock(); if (done) return 0; @@ -417,7 +494,7 @@ inet_connect(void *h, uint32 addr, uint16 port) return -ECONNREFUSED; if (myproc()->killed) return -EINTR; - inet_wait(); + net_wait(seen); } } @@ -431,10 +508,9 @@ inet_read(void *h, char *buf, int n) for (;;) { lwip_sys_lock(); - uint got = rx_take(is, buf, (uint)n); - int eof = is->peer_closed; - if (got > 0 && is->type == SOCK_STREAM && is->tpcb) - ; // window already reopened in the receive callback + uint got = rx_take(is, buf, (uint)n); + int eof = is->peer_closed; + uint seen = net_seq(); lwip_sys_unlock(); if (got > 0) @@ -443,7 +519,7 @@ inet_read(void *h, char *buf, int n) return 0; // end of file, not an error if (myproc()->killed) return -EINTR; - inet_wait(); + net_wait(seen); } } @@ -472,6 +548,8 @@ inet_write(void *h, char *buf, int n) // TCP: the send buffer is finite, so a large write becomes several passes. while (done < n) { + uint seen; + lwip_sys_lock(); if (is->peer_closed || is->tpcb == 0) { lwip_sys_unlock(); @@ -499,12 +577,16 @@ inet_write(void *h, char *buf, int n) return done > 0 ? done : -EIO; } } + seen = net_seq(); lwip_sys_unlock(); if (done < n) { if (myproc()->killed) return done > 0 ? done : -EINTR; - inet_wait(); // wait for the window or the buffer + // Woken by inet_sent_cb when an acknowledgement frees send buffer, or by + // a failure. Without tcp_sent registered this would be a sleep with + // nobody left to end it, which is why that callback exists. + net_wait(seen); } } return done; diff --git a/kernel/net/net.c b/kernel/net/net.c index 821def8..6020481 100644 --- a/kernel/net/net.c +++ b/kernel/net/net.c @@ -252,6 +252,12 @@ netinit(void) { if (net_started) return; + // Before the check below, not after: the lock it initialises has to be + // valid on a machine with no NIC too. Nothing can reach a wait there -- + // inet_new refuses while the stack is down -- but a lock whose validity + // depends on that reasoning holding forever is not worth the two lines + // saved. + inet_init(); if (!e1000_present()) { // Silent: a machine without an e1000 is not misconfigured, and this // runs on every boot.