Copilot commented on code in PR #13821:
URL: https://github.com/apache/trafficserver/pull/13821#discussion_r4251617270
##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -285,6 +287,12 @@ UnixNetVConnection::do_io_close(int alerrno /* = -1 */)
closed = -1;
}
+ // Only rearm on this NetEvent's own thread; a cross-thread close defers to
+ // whichever path eventually runs do_io_close (or free_netevent) there.
+ if (t == this->thread) {
+ this->rearm_timer();
+ }
Review Comment:
A cross-thread close has no guaranteed owner-thread follow-up here. The
global session pool and connection migration can call `do_io_close()` from a
different ET_NET thread (`HttpSessionManager.cc:515`,
`UnixNetVConnection.cc:1389`); after the full cop sweep is removed, such a
closed VC remains at its old wheel deadline—or forever if it had no
deadline—because no readiness event is guaranteed to revisit it. Queue an
owner-thread rearm/free operation instead of simply skipping the wheel update.
##########
src/records/RecordsConfig.cc:
##########
@@ -832,6 +832,9 @@ static constexpr RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.net.inactivity_check_frequency", RECD_INT, "1",
RECU_RESTART_TS, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
,
+ {RECT_CONFIG, "proxy.config.net.inactivity_cop_budget", RECD_INT, "4096",
RECU_DYNAMIC, RR_NULL, RECC_STR, "^[1-9][0-9]*$",
+ RECA_NULL}
Review Comment:
This accepts values larger than `INT_MAX`, but the value is narrowed into
`uint32_t` and then cast to `int` by `InactivityCop::budget()`. For example,
2147483648 becomes a negative budget on supported 32-bit-int targets:
`expire()` does no work while the cop continually schedules another pass,
effectively disabling all timeouts. Constrain the record to the positive `int`
range.
##########
src/iocore/net/InactivityCop.h:
##########
@@ -0,0 +1,328 @@
+/** @file
+
+ A brief file description
+
+ @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 "P_Net.h"
+#include "iocore/eventsystem/Continuation.h"
+#include "iocore/eventsystem/EThread.h"
+#include "iocore/eventsystem/Event.h"
+#include "iocore/eventsystem/Lock.h"
+#include "iocore/eventsystem/VConnection.h"
+#include "iocore/net/NetEvent.h"
+#include "iocore/net/NetHandler.h"
+#include "tscore/Ptr.h"
+#include "tscore/ink_hrtime.h"
+#include "tsutil/DbgCtl.h"
+#include "tsutil/Metrics.h"
+#include "ts/ats_probe.h"
+
+#include <cinttypes>
+
+// INKqa10496
+// One Inactivity cop runs on each thread once every second and
+// visits the NetEvents whose timer-wheel deadline is due, calling timeouts.
+class InactivityCop : public Continuation
+{
+public:
+ // Fallback budget, used only if the configured value is unusable. A tick's
+ // worth of due elements is normally tiny; the budget is only a guard against
+ // one correlated wave monopolizing the thread. Anything deferred past it is
+ // picked up TIMEOUT_CONTINUE_DELAY later, not on the next tick.
+ static constexpr int TIMEOUT_BUDGET = 4096;
+
+ // Deliberately not schedule_imm(): an immediate event is dispatched inline
by
+ // EThread::process_queue()'s own dequeue loop, so it would re-enter this
pass
+ // before the poll events run and bound nothing. A timed event lands in
+ // EventQueue instead, where dequeue_ready() will not return it this loop
+ // iteration, so the tail handler polls first.
+ static constexpr ink_hrtime TIMEOUT_CONTINUE_DELAY = HRTIME_MSECONDS(1);
+
+ InactivityCop(Ptr<ProxyMutex> const &m, NetHandler &nh) :
Continuation(m.get()), _nh(nh)
+ {
+ SET_HANDLER(&InactivityCop::check_inactivity);
+ }
+
+ /// Fire callback for the timer wheel. Applies the timeout to each NetEvent
+ /// whose true deadline has passed, preserving the semantics of the old
sweep.
+ struct Fire {
+ NetHandler &nh;
+ ink_hrtime now;
+ Event *e;
+ /// The wheel calls deadline_of() exactly once per element it pops, so this
+ /// is an exact count of what the cop examined this run.
+ int visited = 0;
+
+ ink_hrtime
+ deadline_of(NetEvent *ne)
+ {
+ ++visited;
+ // Must agree exactly with rearm_timer(): closed NetEvents are reaped on
+ // the next tick rather than at their original deadline.
+ return ne->closed ? now : nh._earliest_deadline(ne);
+ }
+
+ void
+ operator()(NetEvent *ne) const
+ {
+ // If we cannot get the lock don't stop just keep cleaning. The element
+ // has already been popped from the wheel, so it must be rescheduled or
+ // it will never be visited again.
+ MUTEX_TRY_LOCK(lock, ne->get_mutex(), this_ethread());
+ if (!lock.is_locked()) {
+
Metrics::Counter::increment(net_rsb.inactivity_cop_lock_acquire_failure);
+ nh.rearm_timer(ne);
+ return;
+ }
+
+ if (ne->closed) {
+ nh.free_netevent(ne);
+ return;
+ }
+
+ // set a default inactivity timeout if one is not set
+ // The event `EVENT_INACTIVITY_TIMEOUT` only be triggered if a read
+ // or write I/O operation was set by `do_io_read()` or `do_io_write()`.
+ if (ne->next_inactivity_timeout_at == 0 &&
ne->default_inactivity_timeout_in > 0 && (ne->read.enabled ||
ne->write.enabled)) {
+ Dbg(dbg_ctl_inactivity_cop, "vc: %p inactivity timeout not set,
setting a default of %d", ne,
+ nh.config.default_inactivity_timeout);
+ ne->use_default_inactivity_timeout = true;
+ ne->next_inactivity_timeout_at = now +
ne->default_inactivity_timeout_in;
+ ne->inactivity_timeout_in = 0;
+ ne->rearm_timer();
+
Metrics::Counter::increment(net_rsb.default_inactivity_timeout_applied);
+ }
+
+ if (ne->next_inactivity_timeout_at && ne->next_inactivity_timeout_at <
now) {
+ if (ne->is_default_inactivity_timeout()) {
+ // track the connections that timed out due to default inactivity
+ Dbg(dbg_ctl_inactivity_cop, "vc: %p timed out due to default
inactivity timeout", ne);
+
Metrics::Counter::increment(net_rsb.default_inactivity_timeout_count);
+ }
+ if (nh.keep_alive_queue.in(ne)) {
+ // only stat if the connection is in keep-alive, there can be other
inactivity timeouts
+ ink_hrtime diff = (now - (ne->next_inactivity_timeout_at -
ne->inactivity_timeout_in)) / HRTIME_SECOND;
+ Metrics::Counter::increment(net_rsb.keep_alive_queue_timeout_total,
diff);
+ Metrics::Counter::increment(net_rsb.keep_alive_queue_timeout_count);
+ }
+ Dbg(dbg_ctl_inactivity_cop_verbose, "ne: %p now: %" PRId64 " timeout
at: %" PRId64 " timeout in: %" PRId64, ne,
+ ink_hrtime_to_sec(now), ne->next_inactivity_timeout_at,
ne->inactivity_timeout_in);
+ ATS_PROBE6(net_inactivity_timeout, ne->get_fd(), now,
ne->next_inactivity_timeout_at, ne->inactivity_timeout_in,
+ ne->is_default_inactivity_timeout() ? 1 : 0,
ne->default_inactivity_timeout_in.load());
+ ne->callback(VC_EVENT_INACTIVITY_TIMEOUT, e);
+ } else if (ne->next_activity_timeout_at && ne->next_activity_timeout_at
< now) {
+ Dbg(dbg_ctl_inactivity_cop_verbose, "active ne: %p now: %" PRId64 "
timeout at: %" PRId64 " timeout in: %" PRId64, ne,
+ ink_hrtime_to_sec(now), ne->next_activity_timeout_at,
ne->active_timeout_in);
+ ne->callback(VC_EVENT_ACTIVE_TIMEOUT, e);
+ } else {
+ // Examined but not due. The wheel hands over an element once its
+ // deadline is <= now while both tests above are strict <, so a
deadline
+ // exactly equal to now lands here. The element has already been
popped,
+ // so it has to go back in or nothing will ever visit it again and it
+ // never times out. Re-arming rather than firing also matches the
+ // pre-wheel sweep, which left such an element for the next tick.
+ //
+ // Only safe on this path: the callbacks above may free ne.
+ nh.rearm_timer(ne);
+ }
+ }
+ };
+
+ /// Timeouts one pass may fire, from config, with the fallback applied.
+ ///
+ /// expire() treats a budget of 0 as "do nothing and leave the wheel alone",
so
+ /// a misconfigured 0 here would silently stop every timeout in the process
--
+ /// connections would accumulate with no error and no metric moving. The
records
+ /// schema rejects 0, and this is the second line of defence for a value that
+ /// arrived some other way.
+ int
+ budget() const
+ {
+ uint32_t const configured = _nh.config.inactivity_cop_budget;
+
+ return configured > 0 ? static_cast<int>(configured) : TIMEOUT_BUDGET;
+ }
+
+ int
+ check_inactivity(int /* event */, Event *e)
+ {
+ if (run(ink_get_hrtime(), e) >= budget()) {
+ // Deadlines are still due, so come back shortly instead of waiting out
+ // the rest of the tick. The budget bounds how long one pass can hold the
+ // poll thread; it is not a cap on how much work a tick may retire.
+ // Waiting for the periodic event would be worse than it looks: expire()
+ // does not advance its cursor until a bucket drains, so an over-full
+ // bucket would also delay examining every tick behind it.
+ this_ethread()->schedule_in(this, TIMEOUT_CONTINUE_DELAY);
+ }
+ return 0;
+ }
+
+ /** One timeout pass, with @a now supplied by the caller.
+ *
+ * Split out from the event handler purely so a test can drive the cop with a
+ * controlled clock: the wheel's resolution is one second, so a harness that
+ * advances real time cannot exercise a per-tick cost without sleeping. Every
+ * deadline comparison in a pass uses this one timestamp.
+ *
+ * Scheduling policy stays in the handler, so driving this directly does not
+ * queue events. Returns the number of timeouts fired; reaching the budget
+ * means more are already due.
+ */
+ int
+ run(ink_hrtime now, Event *e)
+ {
+ NetHandler &nh = _nh;
+
+ Dbg(dbg_ctl_inactivity_cop_check, "Checking inactivity on Thread-ID #%d",
this_ethread()->id);
+
+ Fire fire{nh, now, e};
+ int const pass_budget = budget();
+ ink_hrtime const started = ink_get_hrtime();
+ int const fired = nh.timer_wheel.expire(now, pass_budget,
fire);
+ int64_t const elapsed_us = ink_hrtime_to_usec(ink_get_hrtime() -
started);
+
+ // The cop's own cost had no telemetry before the wheel, which is how an
+ // O(N)-per-second sweep regressed unnoticed.
+ //
+ // The timing covers expire() only - the wheel walk plus the inline closes
it
+ // dispatches - and deliberately not the keep-alive sweep below, so that
+ // pass_time_us divided by fired is a per-close figure and nothing else.
Two
+ // clock reads per pass per thread is not worth gating behind a debug tag.
+ Metrics::Counter::increment(net_rsb.inactivity_cop_visited, fire.visited);
+ Metrics::Counter::increment(net_rsb.inactivity_cop_passes);
+ Metrics::Counter::increment(net_rsb.inactivity_cop_fired, fired);
Review Comment:
`TimerWheel::expire()` increments its return count after every delegate
call, including when `Fire` returns on a mutex failure or merely frees an
already-closed event. Consequently this metric overcounts actual timeout
callbacks, despite being documented and used as the denominator for per-timeout
cost. Track callback fires separately from the processed count used for
budgeting.
##########
include/tscore/TimerWheel.h:
##########
@@ -0,0 +1,239 @@
+/** @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 <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()'s return value equal to budget means the caller is behind and
+// should call again promptly. 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)
+ {
+ if (budget <= 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;
+
+ 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;
+
+ ink_hrtime const deadline = f.deadline_of(e);
+
+ if (deadline == 0) {
+ continue; // no timeout: drop out of the wheel
+ }
+ if (deadline > now) {
+ e->timer_hook.deadline = deadline;
+ _insert(e, tick + 1); // lazy rearm; floor tick+1 avoids
re-entering `tick`
Review Comment:
The budget only advances after `f(e)`. Entries whose cached deadline is due
but whose true deadline was extended take the lazy-rearm branch and do not
consume budget, so a large bucket of active connections with a shared original
deadline is still processed in one unbounded pass. This can recreate the O(N)
event-loop pause the budget is intended to prevent; count visits/rearms against
a work budget (or add a separate visit budget) and resume promptly.
--
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]
