FreeRTOS: wake the poll task when there is work instead of waiting for the timer - #180
Conversation
|
@wolfSSL-Fenrir-bot review balanced |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #180
Scan targets checked: wolfip-src, wolfip-bugs
Coverage: 1 of 2 in-scope changed file(s) opened by the reviewer; not opened: src/port/freeRTOS/bsd_socket.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Balanced
danielinux
left a comment
There was a problem hiding this comment.
A more generic approach to cover all bsd ports would be preferred. Not including in the current release, as it would require a bit of rework at this stage. Discussing internally.
0c18995 to
3cf8384
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The implementation diverges materially from the stated design and contains callback, synchronization, and API correctness issues.
Review effort: Balanced
Findings: 10
Open (10)
Advertised FreeRTOS poll wake test target is missing · New Poll task implementation does not use promised task notifications · New getsockopt does not return stored SO_RCVTIMEO value · New Server startup wait can hang indefinitely on thread or socket failure · New Server startup wait can hang indefinitely on thread or socket failure · New Server startup wait can hang indefinitely on thread or socket failure · New Server startup wait can hang indefinitely on thread or socket failure · New Binary semaphore mock incorrectly permits counts above one · New Packet socket callbacks incorrectly nested under raw socket guard · New wolfIP_poll return contract changes from status to delay · New
What changed in this PR
Introduces wake-driven polling for FreeRTOS and POSIX integrations, alongside deadline-aware wolfIP_poll() behavior.
Changes:
- Adds poll deadlines and wake callbacks to the core.
- Adds FreeRTOS ISR wake support and POSIX descriptor-based wakeups.
- Updates tests, mocks, CI, and documentation.
| File | Description |
|---|---|
wolfip.h |
Declares polling and wake APIs. |
src/wolfip.c |
Implements deadlines and wake signaling. |
src/test/unit/unit.c |
Registers new poll tests. |
src/test/unit/unit_tests_poll_dispatcher.c |
Tests deadlines and wake callbacks. |
src/test/unit/unit_tests_multicast.c |
Updates poll expectations. |
src/test/unit/unit_tests_branches.c |
Updates poll return assertions. |
src/test/test_native_wolfssl.c |
Caps test polling delay. |
src/test/test_httpd.c |
Caps test polling delay. |
src/test/test_freertos_close_last_ack.c |
Extends FreeRTOS mocks and wake checks. |
src/test/test_eventloop.c |
Coordinates server startup. |
src/test/test_eventloop_tun.c |
Coordinates TUN server startup. |
src/test/test_dhcp_dns.c |
Coordinates DHCP/DNS test server startup. |
src/test/ipfilter_logger.c |
Caps polling delay. |
src/test/freertos_mocks/wolfip.h |
Mocks the wake API. |
src/test/freertos_mocks/semphr.h |
Adds ISR semaphore mock. |
src/test/freertos_mocks/FreeRTOS.h |
Adds tick and ISR macros. |
src/test/esp/test_esp.c |
Adjusts ESP test polling and startup. |
src/port/vde2/vde_device.h |
Exposes the VDE descriptor. |
src/port/vde2/vde_device.c |
Implements VDE descriptor access. |
src/port/posix/utun_darwin.c |
Makes polling nonblocking and exposes its FD. |
src/port/posix/tap_linux.c |
Makes polling nonblocking and exposes its FD. |
src/port/posix/tap_freebsd.c |
Makes polling nonblocking and exposes its FD. |
src/port/posix/bsd_socket.c |
Adds wake-pipe polling and receive timeouts. |
src/port/freeRTOS/README.md |
Documents semaphore-driven polling. |
src/port/freeRTOS/bsd_socket.h |
Declares the ISR wake API. |
src/port/freeRTOS/bsd_socket.c |
Implements semaphore wakeups and tick conversion. |
docs/porting_guide.md |
Updates polling integration guidance. |
docs/migrating_from_lwIP.md |
Updates FreeRTOS migration guidance. |
docs/migrating_from_lwIP_JP.md |
Updates Japanese migration guidance. |
docs/http_server_howto.md |
Updates poll-loop example. |
docs/API.md |
Documents polling and wake contracts. |
.github/workflows/linux.yml |
Runs FreeRTOS wrapper tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The descriptor table carries rcv_timeout_ms, and the blocking receive
calls wait on it, but setsockopt() forwarded SO_RCVTIMEO to
wolfIP_sock_setsockopt() and never stored it, so every blocking receive
waited for ever. A wait that did expire returned ETIMEDOUT, where Linux
reports an expired SO_RCVTIMEO as EAGAIN.
setsockopt() now records SO_RCVTIMEO for wolfIP descriptors and
getsockopt() reports it back, and an expired wait returns EAGAIN. The
values follow Linux: {0, 0} or one too large to store waits for ever, a
negative tv_sec does not wait, and a tv_usec outside [0, 1000000) fails
with EDOM.
iputils ping depends on this: once replies come back quickly it blocks
in recvmsg() and relies on SO_RCVTIMEO to return in time to send the
next request. Under LD_PRELOAD it hung after the first reply in most
runs; the CI ping step passed on timing alone.
wolfIP_poll() always returned 0; the comment above it carried a TODO to return the number of milliseconds to wait before calling it again. It now does: the earliest expiry in the timer heap, the next allowed ARP request while a frame waits for resolution, or 0 when work is left over (a driver that returned EAGAIN on send, an RX budget that ran out, queued loopback frames, or socket events the TX flush raised after this poll had already run the callbacks). With nothing pending it returns WOLFIP_POLL_MAX_WAIT_MS, 1000 by default, defined in wolfip.h so callers can size their own clamps. The value cannot account for frames still sitting in the driver, so a loop that sleeps for it must also wake on receive, or cap the sleep. The POSIX stack thread and the host test loops (including test_esp), which poll a tap device, cap it at 1 ms for now, as test_wolfguard_interop already does at 10 ms. Bare-metal ports ignore the return value and are unaffected.
An OS port that sleeps until wolfIP_poll()'s deadline has to be woken when a socket call queues something, or a send waits out the deadline. wolfIP_set_wake_cb() registers a callback the core calls when that happens: after each TX FIFO push (TCP data and control segments, UDP, ICMP, raw and packet sockets), when accept() or a failed ACK arms an ACK retry, when an IGMP join arms its report timer, when a frame is queued on the loopback interface, when a socket callback is registered while events are already pending, when a partial TCP read raises CB_EVENT_READABLE again on a socket with a callback, and after wolfIP_recv() or wolfIP_recv_ex() hands in a frame from a driver's own context. It is never called while wolfIP_poll() runs, since anything queued there is flushed before it returns. Because the core only calls it when a frame really was queued, a window update, FIN or SYN-ACK wakes the poller exactly when one exists, and every OS port gets the same behaviour instead of each wrapper guessing which of its calls might have queued something. A port that sleeps until the deadline also leaves last_tick stale, by up to WOLFIP_POLL_MAX_WAIT_MS, and a socket call arms its timers from it. A connect() 900 ms into an idle sleep armed its SYN retransmit 100 ms after the SYN left; the early retransmit counted as a control timeout, and the connection then started with a 3 s RTO. The FIN from close(), the SYN-ACK from accept() and the DNS, DHCP and IGMP timers were early in the same way. While a wake callback is set, a timer armed between polls is now marked, and the next wolfIP_poll() moves it forward by the time since last_tick, so it starts when its frame is sent; the wake brings that poll right after the call. Without a wake callback the caller polls on its own schedule, and the timer keeps last_tick as its start, as before.
The poll task clamped wolfIP_poll()'s return value between WOLFIP_FREERTOS_POLL_MIN_MS and _MAX_MS, but that value was always 0, so the task slept exactly MIN every cycle and everything a socket queued waited for it. It now clamps the real deadline and waits on a binary semaphore instead of vTaskDelay(). The wake callback registered with wolfIP_set_wake_cb() gives the semaphore, so transmits leave at once. The defaults become MIN 1 ms and MAX 5 ms. MAX now bounds only how long a received frame waits on a driver without an RX interrupt, which is what the old fixed 5 ms sleep gave; MIN keeps the task from spinning when wolfIP_poll() reports work still pending. The return value is read as an int, so an error no longer turns into the longest sleep. now_ms is derived from configTICK_RATE_HZ. portTICK_PERIOD_MS is 1000 / configTICK_RATE_HZ in integer arithmetic, so it is 0 above 1000 Hz, which stopped every wolfIP timer, and truncates for rates that do not divide 1000. The semaphore is never deleted once created: a link driver may enable its RX interrupt before init, so a failed init keeps it and the next init reuses it. The Linux workflow now runs the FreeRTOS wrapper tests; they were built there but never run. The close test now also checks that init registers a wake callback that gives the semaphore, and that a failed init leaves neither the callback nor the lock behind.
wolfip_freertos_notify_from_isr() wakes the poll task from an interrupt, so a driver with a receive interrupt can have an arriving frame serviced at once instead of waiting out WOLFIP_FREERTOS_POLL_MAX_MS. On a platform whose driver polls, that wait is the dominant latency: every frame in a transfer costs up to a full interval. It is xSemaphoreGiveFromISR on the semaphore the poll task already waits on, reading the handle once, so it collapses with the wakes from the core. It does nothing before wolfip_freertos_socket_init() has run; the FreeRTOS test checks both cases.
The socket callback printed from the poll task on every close and every 32nd event. During a bulk transfer that is hundreds of lines of console output inside the data path, enough to move the throughput it would be used to diagnose: a 1 MiB TLS echo varied by 30% run to run with it on. Off by default; build with -DWOLFIP_BSD_DEBUG_CALLBACK=1 to get it back.
3cf8384 to
66a7851
Compare
On hosts running systemd-udevd, MACAddressPolicy=persistent in 99-default.link gives a new interface a name-derived MAC shortly after it appears. tap_init() read the kernel's random MAC and moved on, so when the stack resolved the host side over ARP before udev ran, it kept sending to the old address and the host dropped the frames as meant for another machine. The CI raw_ping and LD_PRELOAD ping steps hung this way once the stack thread stopped holding the lock for 2 ms per poll, which had delayed the first ARP request past udev. It never shows in a network namespace of its own, where udev does not act. Writing the address back with SIOCSIFHWADDR marks it user-assigned, which udev's MAC policy leaves alone.
The stack thread called wolfIP_poll() with usleep(0) between cycles, and each tap driver's poll blocked in poll(2) for 2 ms, inside wolfIP_poll() and so with wolfIP_mutex held. The lock was held most of the time, and a socket call waited for the stack thread to let go of it. The tap drivers now poll without blocking and export their descriptor (tap_get_fd(), vde_get_fd()). The stack thread waits in poll(2) on that descriptor and on a pipe the wake callback writes to, for at most wolfIP_poll()'s deadline, with the mutex released. The callback writes through the real write(), since it runs under wolfIP_mutex, and both pipe ends are close-on-exec like the per-socket pipes. Without a pipe or an RX descriptor the thread falls back to polling every 1 ms, and it drops a descriptor that reports an error or hang-up rather than spin on it. test_eventloop, test_eventloop_tun, test_dhcp_dns and test_esp called wolfIP_sock_connect() before the host server thread was listening. The 2 ms block in tap_poll() delayed the SYN enough to hide it; without it the SYN is reset, never retried, and the test hangs. They now start the server and wait for it to listen before connecting.
66a7851 to
de3b3f7
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #180
Scan targets checked: wolfip-src, wolfip-bugs
Coverage: 4 of 9 in-scope changed file(s) opened by the reviewer; not opened: src/port/freeRTOS/bsd_socket.h, src/port/posix/tap_freebsd.c, src/port/posix/utun_darwin.c, src/port/vde2/vde_device.c, src/port/vde2/vde_device.h
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
| /* Lowers the deadline the current wolfIP_poll() returns. */ | ||
| static void wolfIP_poll_by(struct wolfIP *s, uint64_t when) | ||
| { | ||
| if (when < s->poll_next_at) |
There was a problem hiding this comment.
wolfIP_poll_by compares deadlines as raw uint64 while wait/expiry use 32-bit wrapped ticks · Logic errors
After timers_heap_rebase() truncates pending timers to 32-bit values (backward clock step on POSIX epoch-ms now), or when timers were armed from last_tick 0, the raw when < poll_next_at check picks the stale small next_tmr over poll_by(now)/ARP/MAX deadlines. wolfIP_poll() then returns the timer's full remaining time, not 0, and can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread sleeps with TX backpressure, loopback frames or undelivered events pending.
Suggested fix: Compare in the same wrapped domain as tick_expired(), e.g. (int32_t)((uint32_t)when - (uint32_t)s->poll_next_at) < 0.
wolfIP_poll_by() kept the earliest deadline with a raw 64-bit compare, while tick_expired() and the wait wolfIP_poll() returns use the signed difference of the low 32 bits. The two disagree once pending timers hold 32-bit values in a 64-bit tick domain: timers_heap_rebase() truncates them after the clock steps back (a POSIX epoch-ms now), and timers armed before the first poll start from last_tick 0. Such a timer is small as a raw value, so it replaced the deadline set by wolfIP_poll_by(s, now) for pending events, loopback frames or TX backpressure, and wolfIP_poll() returned the timer's whole remaining time instead of 0. That can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread slept with work pending. Compare in the same wrapped domain as tick_expired(). Every deadline then orders the way the timers fire, and none can lie beyond the now + WOLFIP_POLL_MAX_WAIT_MS that each poll starts from. Reported in the review of wolfSSL#180. The new tests step a 64-bit clock back under a pending timer and check that the poll still returns at most WOLFIP_POLL_MAX_WAIT_MS, and that a rebased timer does not override a deadline at now.
wolfIP_poll_by() kept the earliest deadline with a raw 64-bit compare, while tick_expired() and the wait wolfIP_poll() returns use the signed difference of the low 32 bits. The two disagree once pending timers hold 32-bit values in a 64-bit tick domain: timers_heap_rebase() truncates them after the clock steps back (a POSIX epoch-ms now), and timers armed before the first poll start from last_tick 0. Such a timer is small as a raw value, so it replaced the deadline set by wolfIP_poll_by(s, now) for pending events, loopback frames or TX backpressure, and wolfIP_poll() returned the timer's whole remaining time instead of 0. That can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread slept with work pending. Compare in the same wrapped domain as tick_expired(). Every deadline then orders the way the timers fire, and none can lie beyond the now + WOLFIP_POLL_MAX_WAIT_MS that each poll starts from. Reported in the review of wolfSSL#180. The new tests step a 64-bit clock back under a pending timer and check that the poll still returns at most WOLFIP_POLL_MAX_WAIT_MS, and that a rebased timer does not override a deadline at now.
wolfIP_poll_by() kept the earliest deadline with a raw 64-bit compare, while tick_expired() and the wait wolfIP_poll() returns use the signed difference of the low 32 bits. The two disagree once pending timers hold 32-bit values in a 64-bit tick domain: timers_heap_rebase() truncates them after the clock steps back (a POSIX epoch-ms now), and timers armed before the first poll start from last_tick 0. Such a timer is small as a raw value, so it replaced the deadline set by wolfIP_poll_by(s, now) for pending events, loopback frames or TX backpressure, and wolfIP_poll() returned the timer's whole remaining time instead of 0. That can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread slept with work pending. Compare in the same wrapped domain as tick_expired(). Every deadline then orders the way the timers fire, and none can lie beyond the now + WOLFIP_POLL_MAX_WAIT_MS that each poll starts from. Reported in the review of wolfSSL#180. The new tests step a 64-bit clock back under a pending timer and check that the poll still returns at most WOLFIP_POLL_MAX_WAIT_MS, and that a rebased timer does not override a deadline at now.
wolfIP_poll_by() kept the earliest deadline with a raw 64-bit compare, while tick_expired() and the wait wolfIP_poll() returns use the signed difference of the low 32 bits. The two disagree once pending timers hold 32-bit values in a 64-bit tick domain: timers_heap_rebase() truncates them after the clock steps back (a POSIX epoch-ms now), and timers armed before the first poll start from last_tick 0. Such a timer is small as a raw value, so it replaced the deadline set by wolfIP_poll_by(s, now) for pending events, loopback frames or TX backpressure, and wolfIP_poll() returned the timer's whole remaining time instead of 0. That can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread slept with work pending. Compare in the same wrapped domain as tick_expired(). Every deadline then orders the way the timers fire, and none can lie beyond the now + WOLFIP_POLL_MAX_WAIT_MS that each poll starts from. Reported in the review of wolfSSL#180. The new tests step a 64-bit clock back under a pending timer and check that the poll still returns at most WOLFIP_POLL_MAX_WAIT_MS, and that a rebased timer does not override a deadline at now.
wolfIP_poll_by() kept the earliest deadline with a raw 64-bit compare, while tick_expired() and the wait wolfIP_poll() returns use the signed difference of the low 32 bits. The two disagree once pending timers hold 32-bit values in a 64-bit tick domain: timers_heap_rebase() truncates them after the clock steps back (a POSIX epoch-ms now), and timers armed before the first poll start from last_tick 0. Such a timer is small as a raw value, so it replaced the deadline set by wolfIP_poll_by(s, now) for pending events, loopback frames or TX backpressure, and wolfIP_poll() returned the timer's whole remaining time instead of 0. That can exceed WOLFIP_POLL_MAX_WAIT_MS, so the POSIX stack thread slept with work pending. Compare in the same wrapped domain as tick_expired(). Every deadline then orders the way the timers fire, and none can lie beyond the now + WOLFIP_POLL_MAX_WAIT_MS that each poll starts from. Reported in the review of #180. The new tests step a 64-bit clock back under a pending timer and check that the poll still returns at most WOLFIP_POLL_MAX_WAIT_MS, and that a rebased timer does not override a deadline at now.

Summary
Reworked after review. The first version woke the FreeRTOS poll task from each socket call in the FreeRTOS wrapper; that was the wrong layer, and the POSIX layer had the same problem. The wake now lives in the core, where frames are actually queued, and
wolfIP_poll()returns the time to its next deadline (the old TODO above it), so an OS port can sleep until then and be woken early when there is work. Both in-tree OS layers use it: FreeRTOS and the POSIXLD_PRELOADlayer.wolfIP_poll()returns the milliseconds until its next deadline: the earliest timer, the next allowed ARP request while a frame waits for resolution, or 0 when work is left over (driver TX backpressure, an exhausted RX budget, queued loopback frames, socket events raised by the TX flush). With nothing pending it returnsWOLFIP_POLL_MAX_WAIT_MS(default 1000). Frames still in the driver are not covered, so a loop that sleeps for the value must also wake on receive or cap the sleep.wolfIP_set_wake_cb(): the core calls the callback when a socket call leaves work for the nextwolfIP_poll(): after each TX FIFO push (TCP data and control segments, UDP, ICMP, raw, packet), when an ACK retry or IGMP report timer is armed, when a frame is queued on loopback, when a callback is registered with events pending, and afterwolfIP_recv()/wolfIP_recv_ex()from a driver's own context. It is never called from insidewolfIP_poll(). Window updates, FINs and SYN-ACKs wake the poller only when one was really queued.WOLFIP_FREERTOS_POLL_MIN_MS/_MAX_MSclamp, which now applies to a real deadline, and waits on a binary semaphore instead ofvTaskDelay(). The wake callback gives the semaphore, so no task notification is reserved. Defaults change to MIN 1 ms / MAX 5 ms: MAX bounds how long a received frame waits on a driver without an RX interrupt (what the fixed 5 ms sleep gave before), MIN keeps the task from spinning.now_msis derived fromconfigTICK_RATE_HZ, sinceportTICK_PERIOD_MSis 0 above 1000 Hz.wolfip_freertos_notify_from_isr()lets a link driver's RX interrupt wake the poll task at once. The semaphore is never deleted once created, so an interrupt enabled before init is safe.usleep(0)while each tap driver blocked for 2 ms insidewolfIP_poll()withwolfIP_mutexheld. The tap drivers now poll without blocking and export their descriptor (tap_get_fd(),vde_get_fd()); the thread waits inpoll(2)on that descriptor and on a wake pipe, for at most the returned deadline, with the mutex released.SO_RCVTIMEO:setsockopt()never stored it, so blocking receives ignored it, and an expired wait returnedETIMEDOUTinstead ofEAGAIN. iputilspingdepends on it once replies are fast, and the CILD_PRELOADping step only passed on timing.WOLFIP_BSD_DEBUG_CALLBACK(default off).Behaviour and API changes
wolfIP_set_wake_cb()andwolfIP_wake_cbinwolfip.h;WOLFIP_POLL_MAX_WAIT_MS;wolfip_freertos_notify_from_isr();WOLFIP_BSD_DEBUG_CALLBACK;tap_get_fd()andvde_get_fd()for the POSIX host drivers.wolfIP_poll()returns a deadline instead of always 0; bare-metal ports ignore the value and are unaffected. FreeRTOS defaultsWOLFIP_FREERTOS_POLL_MIN_MS5 -> 1 and_MAX_MS20 -> 5. POSIX: an expired blocking wait reportsEAGAIN; the tap drivers'pollno longer blocks.wolfIP_poll()and the socket calls. On FreeRTOS,wolfip_freertos_notify_from_isr()is only safe from interrupts at or belowconfigMAX_SYSCALL_INTERRUPT_PRIORITY; to avoid a receive livelock, mask the RX interrupt in the ISR and re-enable it fromll->pollonce the ring is drained.Testing
wolfIP_poll() == 0now check the returned value. Also clean under ASan, UBSan, and the multicast and VLAN variants.test-evloop,test-evloop-tun,test-wolfssl, the forwarding and TTL tests,test-espin both modes,raw_ping,packet_pingand theLD_PRELOADping. Four host tests (test_eventloop,test_eventloop_tun,test_dhcp_dns,test_esp) calledconnect()before their host server listened; the 2 ms tap block hid it, and they now wait for the listener. Their loops cap the returned sleep at 1 ms.wolfip_freertos_notify_from_isr()): the demo's network suite passes 10/10 including X25519MLKEM768 + ML-DSA-44, and its stress harness passes. TLS echo round trips are unchanged against the first version of this PR (16 KiB about 12 ms, 1 MiB bulk echo about 0.8 s): both wake on transmit and on receive, but the poll task now sleeps until its next deadline when idle.Docs
docs/API.md: thewolfIP_poll()return value andwolfIP_set_wake_cb().docs/porting_guide.md§6.1/§6.2 and the tap driver example;docs/migrating_from_lwIP.mdanddocs/migrating_from_lwIP_JP.md;docs/http_server_howto.md.src/port/freeRTOS/README.md: the deadline-driven poll task, the new API and knobs.Follow-up
The FreeRTOS wrapper's blocking-semantics PR (
SO_RCVTIMEO/SO_SNDTIMEO, anaccept()that returns only established connections, a boundedclose()) builds on this one and on #178, and will be rebased onto this version.