From 5b674dc28c88e7839281039846257fc3ba044a38 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 17 Aug 2025 16:09:57 -0400 Subject: [PATCH] atomic remove --- esphome/core/scheduler.cpp | 37 +++++++++++++++++++++++--- esphome/core/scheduler.h | 53 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/esphome/core/scheduler.cpp b/esphome/core/scheduler.cpp index 6269a66543..c3ade260ac 100644 --- a/esphome/core/scheduler.cpp +++ b/esphome/core/scheduler.cpp @@ -82,7 +82,13 @@ void HOT Scheduler::set_timer_common_(Component *component, SchedulerItem::Type item->set_name(name_cstr, !is_static_string); item->type = type; item->callback = std::move(func); + // Initialize remove to false (though it should already be from constructor) + // Not using mark_item_removed_ helper since we're setting to false, not true +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + item->remove.store(false, std::memory_order_relaxed); +#else item->remove = false; +#endif item->is_retry = is_retry; #ifndef ESPHOME_THREAD_SINGLE @@ -398,6 +404,31 @@ void HOT Scheduler::call(uint32_t now) { this->pop_raw_(); continue; } + + // Check if item is marked for removal + // This handles two cases: + // 1. Item was marked for removal after cleanup_() but before we got here + // 2. Item is marked for removal but wasn't at the front of the heap during cleanup_() +#ifdef ESPHOME_THREAD_MULTI_NO_ATOMICS + // Multi-threaded platforms without atomics: must take lock to safely read remove flag + { + LockGuard guard{this->lock_}; + if (is_item_removed_(item.get())) { + this->pop_raw_(); + this->to_remove_--; + continue; + } + } +#else + // Single-threaded or multi-threaded with atomics: can check without lock + if (is_item_removed_(item.get())) { + LockGuard guard{this->lock_}; + this->pop_raw_(); + this->to_remove_--; + continue; + } +#endif + #ifdef ESPHOME_DEBUG_SCHEDULER const char *item_name = item->get_name(); ESP_LOGV(TAG, "Running %s '%s/%s' with interval=%" PRIu32 " next_execution=%" PRIu64 " (now=%" PRIu64 ")", @@ -518,7 +549,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c if (type == SchedulerItem::TIMEOUT) { for (auto &item : this->defer_queue_) { if (this->matches_item_(item, component, name_cstr, type, match_retry)) { - item->remove = true; + this->mark_item_removed_(item.get()); total_cancelled++; } } @@ -528,7 +559,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Cancel items in the main heap for (auto &item : this->items_) { if (this->matches_item_(item, component, name_cstr, type, match_retry)) { - item->remove = true; + this->mark_item_removed_(item.get()); total_cancelled++; this->to_remove_++; // Track removals for heap items } @@ -537,7 +568,7 @@ bool HOT Scheduler::cancel_item_locked_(Component *component, const char *name_c // Cancel items in to_add_ for (auto &item : this->to_add_) { if (this->matches_item_(item, component, name_cstr, type, match_retry)) { - item->remove = true; + this->mark_item_removed_(item.get()); total_cancelled++; // Don't track removals for to_add_ items } diff --git a/esphome/core/scheduler.h b/esphome/core/scheduler.h index a6092e1b1e..f187549fb2 100644 --- a/esphome/core/scheduler.h +++ b/esphome/core/scheduler.h @@ -97,22 +97,42 @@ class Scheduler { std::function callback; - // Bit-packed fields to minimize padding +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + // Multi-threaded with atomics: use atomic for lock-free access + // Place atomic separately since it can't be packed with bit fields + std::atomic remove{false}; + + // Bit-packed fields (3 bits used, 5 bits padding in 1 byte) + enum Type : uint8_t { TIMEOUT, INTERVAL } type : 1; + bool name_is_dynamic : 1; // True if name was dynamically allocated (needs delete[]) + bool is_retry : 1; // True if this is a retry timeout + // 5 bits padding +#else + // Single-threaded or multi-threaded without atomics: can pack all fields together + // Bit-packed fields (4 bits used, 4 bits padding in 1 byte) enum Type : uint8_t { TIMEOUT, INTERVAL } type : 1; bool remove : 1; bool name_is_dynamic : 1; // True if name was dynamically allocated (needs delete[]) bool is_retry : 1; // True if this is a retry timeout - // 4 bits padding + // 4 bits padding +#endif // Constructor SchedulerItem() : component(nullptr), interval(0), next_execution_(0), +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + // remove is initialized in the member declaration as std::atomic{false} + type(TIMEOUT), + name_is_dynamic(false), + is_retry(false) { +#else type(TIMEOUT), remove(false), name_is_dynamic(false), is_retry(false) { +#endif name_.static_name = nullptr; } @@ -219,6 +239,35 @@ class Scheduler { return item->remove || (item->component != nullptr && item->component->is_failed()); } + // Helper to check if item is marked for removal (platform-specific) + // Returns true if item should be skipped, handles platform-specific synchronization + // NOTE: For ESPHOME_THREAD_MULTI_NO_ATOMICS, caller must hold lock! + bool is_item_removed_(SchedulerItem *item) const { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + // Multi-threaded with atomics: use atomic load for lock-free access + return item->remove.load(std::memory_order_acquire); +#else + // Single-threaded (ESPHOME_THREAD_SINGLE) or + // multi-threaded without atomics (ESPHOME_THREAD_MULTI_NO_ATOMICS): direct read + // For ESPHOME_THREAD_MULTI_NO_ATOMICS, caller MUST hold lock! + return item->remove; +#endif + } + + // Helper to mark item for removal (platform-specific) + // NOTE: For ESPHOME_THREAD_MULTI_NO_ATOMICS, caller must hold lock! + void mark_item_removed_(SchedulerItem *item) { +#ifdef ESPHOME_THREAD_MULTI_ATOMICS + // Multi-threaded with atomics: use atomic store + item->remove.store(true, std::memory_order_release); +#else + // Single-threaded (ESPHOME_THREAD_SINGLE) or + // multi-threaded without atomics (ESPHOME_THREAD_MULTI_NO_ATOMICS): direct write + // For ESPHOME_THREAD_MULTI_NO_ATOMICS, caller MUST hold lock! + item->remove = true; +#endif + } + // Template helper to check if any item in a container matches our criteria template bool has_cancelled_timeout_in_container_(const Container &container, Component *component, const char *name_cstr,