From 7ce49089a09b931918b374d871b30bda4a228111 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 20:39:01 -1000 Subject: [PATCH] [core] Remove pre-sleep socket scan from fast select path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-sleep scan in Application::yield_with_select_() walks monitored_sockets_ on every loop iteration, issuing a volatile cross-thread read on each socket's lwip_sock::rcvevent to preserve select() semantics when the FreeRTOS task notification counter had been consumed but a socket still had unread data. That scenario only existed because of a Socket::ready() contract violation: callers could stop reading with rcvevent > 0, leaving data behind with no pending notification. That contract is now documented and enforced (#15590), and #15589 (the first failure that reverted the earlier removal attempt #14475) has been fixed. With the contract honoured, every rcvevent > 0 is paired with a pending xTaskNotifyGive from the lwip event_callback wrapper (see lwip_fast_select.c). ulTaskNotifyTake either returns immediately (counter non-zero) or wakes the moment the notify lands — the scan has nothing left to rescue. Evidence: https://github.com/esphome/esphome/pull/15638 — an instrumentation PR ran across 5 devices (ESP32 rev1/rev3.1/C3 on Ethernet and WiFi, plus LibreTiny RTL8720CF) through Home Assistant disconnect/reconnect cycles, multi-client API logger bursts, and BLE GATT connect storms. Across ~275,000 scans and 4 observed load-bearing candidates, every hit was in the 2–14µs range — the instruction-level window between the lwip callback writing rcvevent and calling xTaskNotifyGive a few instructions later. Zero hits exceeded 100µs. No hit came anywhere near loop_interval (16ms), which is the latency scale the scan was added to prevent. In addition to being unused, the scan is actively harmful on the hot path: N volatile 16-bit loads against cache-cold cross-thread lwip_sock structures on every main-loop iteration, just to reproduce a microsecond-scale ordering artifact the notification path is already handling authoritatively. This also removes the now-unused monitored_sockets_ vector and Application::{register,unregister}_socket() on the fast-select path. Socket implementations now call esphome_lwip_hook_socket() directly to install the netconn event callback wrapper. --- .../components/socket/bsd_sockets_impl.cpp | 16 +++++---- .../components/socket/lwip_sockets_impl.cpp | 16 +++++---- esphome/core/application.cpp | 27 +-------------- esphome/core/application.h | 33 +++++-------------- 4 files changed, 27 insertions(+), 65 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index aea7c776c62..1db905daea7 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -14,9 +14,13 @@ BSDSocketImpl::BSDSocketImpl(int fd, bool monitor_loop) { if (!monitor_loop || this->fd_ < 0) return; #ifdef USE_LWIP_FAST_SELECT - // Cache lwip_sock pointer and register for monitoring (hooks callback internally) + // Cache lwip_sock pointer (used by ready() for direct rcvevent reads) and hook the + // netconn event callback so the main loop is notified via FreeRTOS task notifications. this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - this->loop_monitored_ = App.register_socket(this->cached_sock_); + if (this->cached_sock_ != nullptr) { + esphome_lwip_hook_socket(this->cached_sock_); + this->loop_monitored_ = true; + } #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif @@ -30,12 +34,10 @@ BSDSocketImpl::~BSDSocketImpl() { int BSDSocketImpl::close() { if (!this->closed_) { - // Unregister before closing to avoid dangling pointer in monitored set #ifdef USE_LWIP_FAST_SELECT - if (this->loop_monitored_) { - App.unregister_socket(this->cached_sock_); - this->cached_sock_ = nullptr; - } + // All LwIP sockets share the same static event_callback, so there is no per-socket + // unhook needed — just drop the cached pointer before the socket is destroyed. + this->cached_sock_ = nullptr; #else if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index 2fad429e0f7..e628a22d8de 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -14,9 +14,13 @@ LwIPSocketImpl::LwIPSocketImpl(int fd, bool monitor_loop) { if (!monitor_loop || this->fd_ < 0) return; #ifdef USE_LWIP_FAST_SELECT - // Cache lwip_sock pointer and register for monitoring (hooks callback internally) + // Cache lwip_sock pointer (used by ready() for direct rcvevent reads) and hook the + // netconn event callback so the main loop is notified via FreeRTOS task notifications. this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - this->loop_monitored_ = App.register_socket(this->cached_sock_); + if (this->cached_sock_ != nullptr) { + esphome_lwip_hook_socket(this->cached_sock_); + this->loop_monitored_ = true; + } #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif @@ -30,12 +34,10 @@ LwIPSocketImpl::~LwIPSocketImpl() { int LwIPSocketImpl::close() { if (!this->closed_) { - // Unregister before closing to avoid dangling pointer in monitored set #ifdef USE_LWIP_FAST_SELECT - if (this->loop_monitored_) { - App.unregister_socket(this->cached_sock_); - this->cached_sock_ = nullptr; - } + // All LwIP sockets share the same static event_callback, so there is no per-socket + // unhook needed — just drop the cached pointer before the socket is destroyed. + this->cached_sock_ = nullptr; #else if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index cd758598801..510e3ec1900 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -449,32 +449,7 @@ void Application::enable_pending_loops_() { } } -#ifdef USE_LWIP_FAST_SELECT -bool Application::register_socket(struct lwip_sock *sock) { - // It modifies monitored_sockets_ without locking — must only be called from the main loop. - if (sock == nullptr) - return false; - esphome_lwip_hook_socket(sock); - this->monitored_sockets_.push_back(sock); - return true; -} - -void Application::unregister_socket(struct lwip_sock *sock) { - // It modifies monitored_sockets_ without locking — must only be called from the main loop. - for (size_t i = 0; i < this->monitored_sockets_.size(); i++) { - if (this->monitored_sockets_[i] != sock) - continue; - - // Swap with last element and pop - O(1) removal since order doesn't matter. - // No need to unhook the netconn callback — all LwIP sockets share the same - // static event_callback, and the socket will be closed by the caller. - if (i < this->monitored_sockets_.size() - 1) - this->monitored_sockets_[i] = this->monitored_sockets_.back(); - this->monitored_sockets_.pop_back(); - return; - } -} -#elif defined(USE_HOST) +#ifdef USE_HOST bool Application::register_socket_fd(int fd) { // WARNING: This function is NOT thread-safe and must only be called from the main loop // It modifies socket_fds_ and related variables without locking diff --git a/esphome/core/application.h b/esphome/core/application.h index 6b2969b4907..122f9b39c7b 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -534,12 +534,7 @@ class Application { /// Register/unregister a socket to be monitored for read events. /// WARNING: These functions are NOT thread-safe. They must only be called from the main loop. -#ifdef USE_LWIP_FAST_SELECT - /// Fast select path: hooks netconn callback and registers for monitoring. - /// @return true if registration was successful, false if sock is null - bool register_socket(struct lwip_sock *sock); - void unregister_socket(struct lwip_sock *sock); -#elif defined(USE_HOST) +#ifdef USE_HOST /// Fallback select() path: monitors file descriptors. /// NOTE: File descriptors >= FD_SETSIZE (typically 10 on ESP) will be rejected with an error. /// @return true if registration was successful, false if fd exceeds limits @@ -653,9 +648,7 @@ class Application { // and active_end_ is incremented // - This eliminates branch mispredictions from flag checking in the hot loop FixedVector looping_components_{}; -#ifdef USE_LWIP_FAST_SELECT - std::vector monitored_sockets_; // Cached lwip_sock pointers for direct rcvevent read -#elif defined(USE_HOST) +#ifdef USE_HOST std::vector socket_fds_; // Vector of all monitored socket file descriptors #endif #ifdef USE_HOST @@ -898,26 +891,16 @@ inline void ESPHOME_ALWAYS_INLINE Application::loop() { #ifndef USE_HOST inline void ESPHOME_ALWAYS_INLINE Application::yield_with_select_(uint32_t delay_ms) { #ifdef USE_LWIP_FAST_SELECT - // Fast path (ESP32/LibreTiny): reads rcvevent directly from cached lwip_sock pointers. - // Safe because this runs on the main loop which owns socket lifetime (create, read, close). + // Fast path (ESP32/LibreTiny): FreeRTOS task notifications posted by the lwip + // event_callback wrapper (see lwip_fast_select.c) are the single source of truth for + // socket wake-ups. Every NETCONN_EVT_RCVPLUS posts an xTaskNotifyGive, so any notification + // that lands between wakes keeps the counter non-zero (next ulTaskNotifyTake returns + // immediately) or wakes a blocked Take directly. Also woken by wake_loop_threadsafe() + // from background tasks, or timeout. if (delay_ms == 0) [[unlikely]] { yield(); return; } - - // Check if any socket already has pending data before sleeping. - // If a socket still has unread data (rcvevent > 0) but the task notification was already - // consumed, ulTaskNotifyTake would block until timeout — adding up to delay_ms latency. - // This scan preserves select() semantics: return immediately when any fd is ready. - for (struct lwip_sock *sock : this->monitored_sockets_) { - if (esphome_lwip_socket_has_data(sock)) { - yield(); - return; - } - } - - // Sleep with instant wake via FreeRTOS task notification. - // Woken by: callback wrapper (socket data), wake_loop_threadsafe() (background tasks), or timeout. #endif esphome::internal::wakeable_delay(delay_ms); }