net: a blocked socket sleeps until something happens, rather than every 10 ms

The AF_INET blocking paths -- connect, accept, read and write -- woke on
every timer tick and looked to see whether anything had changed. That is
a wakeup per blocked socket per 10 ms whether or not the network did
anything, and it answers late even when the data was already sitting in
the buffer.

What stood in the way was a lock mismatch, and it is written down in the
file now because it is the reason this is not simply sleep on the socket:
sleep() wants a spinlock, and the lwIP lock is a sleeplock -- it must be,
since a call into lwIP can reach the driver and the driver can sleep.
Nor can a waiter hold the lwIP lock while sleeping, or the network thread
could never deliver the thing it is waiting for.

So the wait is on a counter with a spinlock of its own. A waiter reads
the counter while it still holds the lwIP lock, right after checking its
condition, then releases and sleeps only if the counter has not moved.
The lwIP callbacks bump it and wake the channel. No wakeup can be lost:
anything that changes a condition does so after the reader's snapshot,
and therefore bumps the counter after it too, and the sleeper re-checks
under the spinlock before committing.

One counter and one channel for all sixteen sockets rather than one
each. Waking a socket with nothing to do costs a few instructions, and a
per-socket channel would have to be torn down in close() against
callbacks that can still arrive -- which is a use-after-free waiting to
be written.

tcp_sent is registered now, on both connected and accepted pcbs. It was
not needed while writes woke on the clock; with this change a write
blocked on a full send buffer would otherwise sleep with nobody left to
wake it, because an acknowledgement is the only thing that frees space.

Verified: bootcheck -Net passes, 38 [ok], no failures. It exercises the
path that matters -- nettest blocks in read() until the host answers, and
the host answers only after reading what the guest sent, so the wakeup
comes from a callback and not from a tick that would have fired anyway.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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);

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 blocking call sleeps until an lwIP callback says something changed. It used
-// a channel a callback wakes. The reason is a lock mismatch: sleep() wants a
+// to wake on every timer tick and look, which cost a wakeup per socket per
-// spinlock, the lwIP lock is a sleeplock (it must be, because a call into lwIP
+// 10 ms and answered up to 10 ms late even when the data was already there.
-// can reach the driver and the driver can sleep), and interleaving the two
-// correctly is a genuinely error-prone piece of code.
 //
-// The cost is up to 10 ms of latency on an operation that has to wait -- and
+// The obstacle was a lock mismatch, and it is worth writing down because it is
-// none at all on one that does not, which is the case that matters for
+// the reason this is not simply sleep(&socket, &lwip_lock): sleep() wants a
-// throughput: a bulk transfer only blocks when the buffer is genuinely empty.
+// spinlock, and the lwIP lock is a sleeplock -- it must be, because a call into
-// Replacing this with a proper sleep/wakeup is worth doing and is deliberately
+// lwIP can reach the driver and the driver can sleep. A waiter cannot hold the
-// not the first version.
+// 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);
   }
 }
 
@@ -410,6 +486,7 @@ inet_connect(void *h, uint32 addr, uint16 port)
   for (;;) {
     lwip_sys_lock();
     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);
   }
 }
 
@@ -433,8 +510,7 @@ inet_read(void *h, char *buf, int n)
     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)
+    uint seen = net_seq();
-      ;  // window already reopened in the receive callback
     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;

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.