Copilot commented on code in PR #13821: URL: https://github.com/apache/trafficserver/pull/13821#discussion_r4261251740
########## include/tscore/TimerWheel.h: ########## @@ -0,0 +1,246 @@ +/** @file + + A timer wheel: O(1) amortized scheduling and expiry of per-element + deadlines, used to replace O(N)-per-tick scans of open connections. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#pragma once + +#include <algorithm> +#include <cstdint> +#include <limits> +#include <type_traits> + +#include "tscore/List.h" +#include "tscore/ink_assert.h" +#include "tscore/ink_hrtime.h" + +// Per-element linkage for TimerWheel: the deadline the element was last +// scheduled with, which bucket (slot) it currently lives in (-1 if not in +// the wheel), and which wheel instance owns it. +// +// An element must never be scheduled on more than one wheel at a time. +// Moving an element between wheels (e.g. a NetVConnection migrating between +// per-thread wheels) requires calling cancel() on the old wheel before +// calling schedule() on the new one; owner is asserted in cancel() and +// schedule() to catch a missed hand-off, which would otherwise splice two +// unrelated rings together. +// +// Destroying an element while it is still scheduled is a use-after-free: +// the element's destructor has no way to reach the wheel and unlink it. +// Callers must cancel() before destruction. +struct TimerWheelHook { + ink_hrtime deadline = 0; + int32_t slot = -1; + void const *owner = nullptr; +}; + +// The delegate expire() drives: it reports each visited element's current true +// deadline and fires the ones that are due. +// +// deadline_of() returning 0 means "no timeout", so the element is dropped from +// the wheel. operator() is called once per element whose true deadline has +// passed, after the element has already been removed from the wheel; it may +// call schedule()/cancel() on the wheel, including re-scheduling the very +// element being fired, and the wheel guards against that landing back in the +// bucket currently being drained. +// +// Both are invoked through a non-const F &, so a delegate may carry mutable +// state shared across one pass - a visit count, for instance. +// +// Spelled with std::is_convertible_v rather than std::convertible_to so that +// toolchains whose standard library predates <concepts> can still build it. +template <class F, class C> +concept TimerWheelDelegate = requires(F &f, C *e) { + requires std::is_convertible_v<decltype(f.deadline_of(e)), ink_hrtime>; + f(e); +}; + +// An intrusive ring of DLL buckets, one per second (TICK), used to find +// elements whose deadline has passed without scanning every element. +// +// Not thread safe: each instance is owned and driven by a single thread. +// +// The scheduled deadline may be earlier than an element's eventual true +// deadline (it will simply be rearmed in place, lazily, at expire time) +// but must never be later, or the timeout fires late. +// +// expire() stops early once it has fired @c budget elements or examined +// @c max_visits of them, whichever comes first; either means the caller is +// behind and should call again promptly. The visit cap matters because a +// lazy rearm fires nothing, so a bucket of elements whose deadlines were all +// extended would otherwise be drained in one unbounded pass. The return value +// is the fired count, so a caller that sets a visit cap must count visits +// itself (via deadline_of(), which runs once per element examined) to tell +// whether that cap was the one hit. If the driving thread lags by more than +// N_BUCKETS ticks (e.g. init() was never called, or a long stall), the +// cursor is fast-forwarded rather than replaying every intervening empty +// tick; elements that would have been visited during that lag lose strict +// firing-order fidelity relative to each other, but none fire early or are +// lost. +// N_BUCKETS default: sized from src/tscore/unit_tests/test_TimerWheel.cc's +// "TimerWheel bucket-count sizing sweep" benchmark. At the 30s/120s keepalive +// timeouts (the dominant real case) 256 through 4096 are all already +// steady-state (~1.0 visits per element per timeout period); the difference +// only shows up for timeouts beyond the ring's range (e.g. a multi-hour +// tunnel active timeout), which get revisited every Buckets-1 ticks until +// due. Measured steady state for a 4h timeout: 1024 buckets costs ~14.9 +// revisits per element per period, 4096 costs ~4. The memory difference +// (8 KiB vs 32 KiB per wheel, one wheel per ET_NET thread) is negligible +// either way, so 4096 is strictly better and is the default. +template <class C, int32_t Buckets = 4096, class L = typename C::Link_timer_link> class TimerWheel +{ +public: + static constexpr int32_t N_BUCKETS = Buckets; + static constexpr ink_hrtime TICK = HRTIME_SECOND; + static constexpr int32_t MAX_TICKS_AHEAD = N_BUCKETS - 1; + + static_assert((N_BUCKETS & (N_BUCKETS - 1)) == 0, "N_BUCKETS must be a power of two"); + + void + init(ink_hrtime now) + { + _cursor = now / TICK; + } + + void + schedule(C *e, ink_hrtime deadline) + { + cancel(e); + e->timer_hook.owner = this; + e->timer_hook.deadline = deadline; + // Floor past both the last fully-drained tick and, if we are being + // called reentrantly from inside expire()'s fire callback, past the + // bucket currently being drained - otherwise a reschedule from within + // the callback could land back in that bucket and be popped again in + // the same pass. + _insert(e, std::max(_cursor + 1, _draining_tick + 1)); + } + + void + cancel(C *e) + { + int32_t const slot = e->timer_hook.slot; + + if (slot >= 0) { + ink_assert(e->timer_hook.owner == this); + _buckets[slot].remove(e); + e->timer_hook.slot = -1; + } + } + + bool + is_scheduled(C const *e) const + { + return e->timer_hook.slot >= 0; + } + + template <TimerWheelDelegate<C> F> Review Comment: `TimerWheelDelegate` is defined as a two-parameter concept (`TimerWheelDelegate<F, C>`), but `expire()` constrains `F` using `TimerWheelDelegate<C>` (wrong number/ordering of concept arguments). This should be rewritten as `template <class F> requires TimerWheelDelegate<F, C>` (or refactor the concept to be single-parameter within `TimerWheel`) so the code compiles and the constraint matches the intended contract. ########## include/tscore/TimerWheel.h: ########## @@ -0,0 +1,246 @@ +/** @file + + A timer wheel: O(1) amortized scheduling and expiry of per-element + deadlines, used to replace O(N)-per-tick scans of open connections. + + @section license License + + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + */ + +#pragma once + +#include <algorithm> +#include <cstdint> +#include <limits> +#include <type_traits> + +#include "tscore/List.h" +#include "tscore/ink_assert.h" +#include "tscore/ink_hrtime.h" + +// Per-element linkage for TimerWheel: the deadline the element was last +// scheduled with, which bucket (slot) it currently lives in (-1 if not in +// the wheel), and which wheel instance owns it. +// +// An element must never be scheduled on more than one wheel at a time. +// Moving an element between wheels (e.g. a NetVConnection migrating between +// per-thread wheels) requires calling cancel() on the old wheel before +// calling schedule() on the new one; owner is asserted in cancel() and +// schedule() to catch a missed hand-off, which would otherwise splice two +// unrelated rings together. +// +// Destroying an element while it is still scheduled is a use-after-free: +// the element's destructor has no way to reach the wheel and unlink it. +// Callers must cancel() before destruction. +struct TimerWheelHook { + ink_hrtime deadline = 0; + int32_t slot = -1; + void const *owner = nullptr; +}; + +// The delegate expire() drives: it reports each visited element's current true +// deadline and fires the ones that are due. +// +// deadline_of() returning 0 means "no timeout", so the element is dropped from +// the wheel. operator() is called once per element whose true deadline has +// passed, after the element has already been removed from the wheel; it may +// call schedule()/cancel() on the wheel, including re-scheduling the very +// element being fired, and the wheel guards against that landing back in the +// bucket currently being drained. +// +// Both are invoked through a non-const F &, so a delegate may carry mutable +// state shared across one pass - a visit count, for instance. +// +// Spelled with std::is_convertible_v rather than std::convertible_to so that +// toolchains whose standard library predates <concepts> can still build it. +template <class F, class C> +concept TimerWheelDelegate = requires(F &f, C *e) { + requires std::is_convertible_v<decltype(f.deadline_of(e)), ink_hrtime>; + f(e); +}; + +// An intrusive ring of DLL buckets, one per second (TICK), used to find +// elements whose deadline has passed without scanning every element. +// +// Not thread safe: each instance is owned and driven by a single thread. +// +// The scheduled deadline may be earlier than an element's eventual true +// deadline (it will simply be rearmed in place, lazily, at expire time) +// but must never be later, or the timeout fires late. +// +// expire() stops early once it has fired @c budget elements or examined +// @c max_visits of them, whichever comes first; either means the caller is +// behind and should call again promptly. The visit cap matters because a +// lazy rearm fires nothing, so a bucket of elements whose deadlines were all +// extended would otherwise be drained in one unbounded pass. The return value +// is the fired count, so a caller that sets a visit cap must count visits +// itself (via deadline_of(), which runs once per element examined) to tell +// whether that cap was the one hit. If the driving thread lags by more than +// N_BUCKETS ticks (e.g. init() was never called, or a long stall), the +// cursor is fast-forwarded rather than replaying every intervening empty +// tick; elements that would have been visited during that lag lose strict +// firing-order fidelity relative to each other, but none fire early or are +// lost. +// N_BUCKETS default: sized from src/tscore/unit_tests/test_TimerWheel.cc's +// "TimerWheel bucket-count sizing sweep" benchmark. At the 30s/120s keepalive +// timeouts (the dominant real case) 256 through 4096 are all already +// steady-state (~1.0 visits per element per timeout period); the difference +// only shows up for timeouts beyond the ring's range (e.g. a multi-hour +// tunnel active timeout), which get revisited every Buckets-1 ticks until +// due. Measured steady state for a 4h timeout: 1024 buckets costs ~14.9 +// revisits per element per period, 4096 costs ~4. The memory difference +// (8 KiB vs 32 KiB per wheel, one wheel per ET_NET thread) is negligible +// either way, so 4096 is strictly better and is the default. +template <class C, int32_t Buckets = 4096, class L = typename C::Link_timer_link> class TimerWheel +{ +public: + static constexpr int32_t N_BUCKETS = Buckets; + static constexpr ink_hrtime TICK = HRTIME_SECOND; + static constexpr int32_t MAX_TICKS_AHEAD = N_BUCKETS - 1; + + static_assert((N_BUCKETS & (N_BUCKETS - 1)) == 0, "N_BUCKETS must be a power of two"); + + void + init(ink_hrtime now) + { + _cursor = now / TICK; + } + + void + schedule(C *e, ink_hrtime deadline) + { + cancel(e); + e->timer_hook.owner = this; + e->timer_hook.deadline = deadline; + // Floor past both the last fully-drained tick and, if we are being + // called reentrantly from inside expire()'s fire callback, past the + // bucket currently being drained - otherwise a reschedule from within + // the callback could land back in that bucket and be popped again in + // the same pass. + _insert(e, std::max(_cursor + 1, _draining_tick + 1)); + } + + void + cancel(C *e) + { + int32_t const slot = e->timer_hook.slot; + + if (slot >= 0) { + ink_assert(e->timer_hook.owner == this); + _buckets[slot].remove(e); + e->timer_hook.slot = -1; + } + } + + bool + is_scheduled(C const *e) const + { + return e->timer_hook.slot >= 0; + } + + template <TimerWheelDelegate<C> F> + int + expire(ink_hrtime now, int budget, F &f, int max_visits = std::numeric_limits<int>::max()) + { + if (budget <= 0 || max_visits <= 0) { + return 0; // nothing to do; wheel state is left untouched + } + + int64_t const now_tick = now / TICK; + + // A cursor lagging by more than a full ring has already had every + // bucket aliased over at least once; fast-forward instead of replaying + // N_BUCKETS empty ticks (this also covers expire() being called before + // init(), where _cursor starts at 0). + if (now_tick - _cursor > N_BUCKETS) { + _cursor = now_tick - N_BUCKETS; + } + + int fired = 0; + int visits = 0; + + while (_cursor < now_tick) { + int64_t const tick = _cursor + 1; + DLL<C, L> &bucket = _buckets[tick & (N_BUCKETS - 1)]; + + { + DrainGuard const guard(_draining_tick, tick); + + while (C *e = bucket.pop()) { + e->timer_hook.slot = -1; + ++visits; + + ink_hrtime const deadline = f.deadline_of(e); + + if (deadline == 0) { + // no timeout: drop out of the wheel + } else if (deadline > now) { + e->timer_hook.deadline = deadline; + _insert(e, tick + 1); // lazy rearm; floor tick+1 avoids re-entering `tick` + } else { + f(e); + ++fired; + } + if (fired >= budget || visits >= max_visits) { + return fired; // guard resets _draining_tick; bucket keeps the rest; _cursor not advanced + } + } + } + _cursor = tick; // only advance once fully drained + } + return fired; + } + +private: + // Marks _draining_tick for the lifetime of one bucket's drain pass so + // that a reentrant schedule() (called from the fire callback) cannot + // land back in that bucket. Resets on every exit, including the budget + // early-return above. + class DrainGuard + { + public: + DrainGuard(int64_t &draining_tick, int64_t tick) : _draining_tick(draining_tick) { _draining_tick = tick; } + ~DrainGuard() { _draining_tick = INT64_MIN; } + + DrainGuard(DrainGuard const &) = delete; + DrainGuard &operator=(DrainGuard const &) = delete; + + private: + int64_t &_draining_tick; + }; + + void + _insert(C *e, int64_t floor_tick) + { + int64_t tick = e->timer_hook.deadline / TICK; + + if (tick < floor_tick) { + tick = floor_tick; + } else if (tick > floor_tick + MAX_TICKS_AHEAD - 1) { + tick = floor_tick + MAX_TICKS_AHEAD - 1; Review Comment: `MAX_TICKS_AHEAD` is defined as `N_BUCKETS - 1`, but the clamp uses `MAX_TICKS_AHEAD - 1`, reducing the usable wheel range by one tick and increasing re-insertion churn for long deadlines. If the intended range is truly `N_BUCKETS - 1` ticks ahead (as described in the header comment), clamp to `floor_tick + MAX_TICKS_AHEAD` instead. ########## src/iocore/net/NetHandler.cc: ########## @@ -93,6 +94,10 @@ NetHandler::stopIO(NetEvent *ne) write_enable_list.remove(ne); ne->write.in_enabled_list = 0; } + if (ne->handoff.load() & NetEvent::HANDOFF_QUEUED) { + handoff_list.remove(ne); + ne->handoff.store(0); + } Review Comment: This clears `handoff` state (including potentially `HANDOFF_CLOSING`) and removes `ne` from `handoff_list`, but `handoff_list` is used for cross-thread handoff (`defer_rearm()` / `close_from_foreign_thread()` can push from other threads). Clearing `HANDOFF_CLOSING` here can break the foreign-close claim invariant (allowing premature free), and removing from an atomic cross-thread list is risky unless `ASLL::remove()` is explicitly designed for concurrent pushers. Prefer leaving already-queued entries to be consumed by `process_handoffs()` and only clearing bits in the owner-thread pop path; if cancellation is required for teardown/migration, keep `HANDOFF_CLOSING` intact and make `process_handoffs()` ignore stale entries (e.g., where `ne->nh != this`) rather than mutating the shared list from `stopIO()`. ########## doc/release-notes/whats-new.en.rst: ########## @@ -20,6 +20,27 @@ .. _whats_new: +What's New in ATS v11.0 +======================= + +Metrics +------- + +* Added ``proxy.process.net.inactivity_cop_visited`` +* Added ``proxy.process.net.inactivity_cop_budget_exhausted`` Review Comment: The PR description and code add additional inactivity-cop metrics (`inactivity_cop_passes`, `_fired`, `_pass_time_us`, `_pass_max_us`), but the v11.0 release notes list only two. Please either list the full set here (or add a short note pointing readers to the monitoring docs section that enumerates them) to avoid under-documenting the newly introduced observability surface. ########## src/iocore/net/NetEvent.cc: ########## @@ -0,0 +1,61 @@ +/** @file + + A brief file description Review Comment: The new source file header still contains the placeholder text \"A brief file description\". Please replace it with a meaningful description of what `NetEvent.cc` provides (e.g., cross-thread-safe timer rearm and default-inactivity deadline rebasing helpers). ########## src/iocore/net/InactivityCop.h: ########## @@ -0,0 +1,365 @@ +/** @file + + A brief file description Review Comment: The new header file banner still contains the placeholder \"A brief file description\". Please update it to describe the timer-wheel-driven `InactivityCop` behavior and its budgeting/audit roles. ########## src/iocore/net/UnixNetVConnection.cc: ########## @@ -455,6 +468,15 @@ UnixNetVConnection::set_enabled(VIO *vio) STATE_FROM_VIO(vio)->enabled = 1; if (!next_inactivity_timeout_at && inactivity_timeout_in) { next_inactivity_timeout_at = ink_get_hrtime() + inactivity_timeout_in; + rearm_timer(); + } else if (!next_inactivity_timeout_at && !inactivity_timeout_in && + default_inactivity_timeout_in.load(std::memory_order_relaxed) > 0) { + // No explicit timeout is set: the wheel only revisits scheduled elements, so + // arm the default here rather than relying on the cop's sweep to notice. + use_default_inactivity_timeout = true; + next_inactivity_timeout_at = ink_get_hrtime() + default_inactivity_timeout_in; Review Comment: Line 473 loads `default_inactivity_timeout_in` with `memory_order_relaxed`, but line 477 reads `default_inactivity_timeout_in` again via the implicit atomic conversion (typically `seq_cst`) and may even observe a different value than the one used in the condition. Consider loading once into a local `default_timeout_in` (relaxed) and using that for both the check and the arithmetic to keep memory ordering consistent and avoid a redundant atomic read. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
