#18365: Fined-grain timer implementation to support per-connection or per-circuit timers -------------------------------------------------+------------------------- Reporter: nickm | Owner: nickm Type: enhancement | Status: Priority: Medium | needs_revision Component: Core Tor/Tor | Milestone: Tor: Severity: Normal | 0.2.9.x-final Keywords: performance, backend, | Version: TorCoreTeam201604 | Resolution: Parent ID: | Actual Points: medium Reviewer: | Points: medium | Sponsor: -------------------------------------------------+------------------------- Changes (by andrea):
* status: needs_review => needs_revision Comment: Begin code review for nickm's timeouts_v2 branch: 32e80ea3d32d5fd8207d16f9e5b26defa0d98a7c: - No detailed review of this commit as this is an import of external code - One potential concern: we need to be pretty clear whether the times we use here are meant to be monotonic times or real-time clock times; it doesn't look like timeout.c makes any direct time syscalls itself, just lets the caller supply the current time as a uint64_t, which gives us flexibility there, but it will be necessary to be consistent within any given struct timeouts. Possibly some admonitory documentation is in order somewhere. - Okay, in d0638fe760363f9c040256ac2884234ddad1d384 it looks like we're committing to monotonic time in libevent_timer_reschedule(). Consider this concern resolved. 05499b6ded25b5cbc8b16916fa9c0a39407ab10f: - Straightforward makefile changes; this looks fine 9d6c530015e4eefd7b333885eaeca1f9fcbc6578: - Stop compiler warnings for timeout.c; looks okay, but are we sure we got everything for every possible case we end up building? - Same for cbf47612b737a6ad31f17084ef5c36f5ebe33a76; some nervousness about all these bitmasks and casts. c77cf8825a33d902c5827f0b4f0a71cec97a3a85: - This looks pretty straightforward and unobjectionable. d0638fe760363f9c040256ac2884234ddad1d384: - Is using a single pool of timeouts for both absolute and relative values with tor_gettimeofday_cached_monotonic() entirely wise? - From the comment for that function: {{{ * In future versions of Tor, this may return a time does not have its * origin at the Unix epoch. }}} This, of course, is intended to allow for a future change to clock_gettime(CLOCK_MONOTONIC, ...), which will provide smoother behavior in the case the clock actually does run backwards than the current remember-and-compare implementation, but is not guaranteed to have any particular origin. Using that where available is clearly the most reliable way to handle relative timeouts, but wrong for absolute ones, and the function we're using is defined to allow us room to make that change in the future. - Do we actually have any examples of needing to fire a particular callback exactly once at or after a particular wall clock time as these absolute timeouts provide? - If we do need absolute timeouts, we should think about splitting things up into two timeouts objects rather than just a unified global_timeouts; if relative timeouts are monotonic and absolute timeouts are based on clock time, then resetting time potentially changes the order of future timeouts without any timeout having occurred. Then we're free to use the appropriate definition of time separately for each. f7a6f142c67a3d256d522d1cfa66e525d16d6ab7: - These tests look just fine. cbf47612b737a6ad31f17084ef5c36f5ebe33a76: - See comments for 9d6c530015e4eefd7b333885eaeca1f9fcbc6578 - This particular cast in timeouts_del(), for example, will break on a machine with 64-bit pointers and 32-bit ints if WHEEL_LEN is very large. - timeout.c does document size constraints on these parameters such that this should work on any platform with integer sizes consistent with ISO C, though. b9e0f7c91623e5ebde774bab61030f04b808c024: - More tests; looks okay. d3a2b9e836cfed39f64963b171be152a7ae8ff4b: - Changes file looks fine In conclusion, this needs more thought about relative vs. absolute timeouts I think. Should I review the imported timeout.c code itself separately, or is this something solid and widely used enough we aren't overly worried about it? -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18365#comment:8> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online _______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs