Re: [PATCH v3 1/2] timer: add a function to adjust timeouts to be upper bound

From: Artem Savkov <asavkov@redhat.com>
To: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: netdev@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	davem@davemloft.net, yoshfuji@linux-ipv6.org, dsahern@kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/2] timer: add a function to adjust timeouts to be upper bound
Date: Sat, 2 Apr 2022 08:55:33 +0200	[thread overview]
Message-ID: <YkfzZWs+Nj3hCvnE@sparkplug.usersys.redhat.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2203301514570.4409@somnus>

On Wed, Mar 30, 2022 at 03:40:55PM +0200, Anna-Maria Behnsen wrote:
> On Wed, 30 Mar 2022, Artem Savkov wrote:
> 
> > Current timer wheel implementation is optimized for performance and
> > energy usage but lacks in precision. This, normally, is not a problem as
> > most timers that use timer wheel are used for timeouts and thus rarely
> > expire, instead they often get canceled or modified before expiration.
> > Even when they don't, expiring a bit late is not an issue for timeout
> > timers.
> > 
> > TCP keepalive timer is a special case, it's aim is to prevent timeouts,
> > so triggering earlier rather than later is desired behavior. In a
> > reported case the user had a 3600s keepalive timer for preventing firewall
> > disconnects (on a 3650s interval). They observed keepalive timers coming
> > in up to four minutes late, causing unexpected disconnects.
> > 
> > This commit adds upper_bound_timeout() function that takes a relative
> > timeout and adjusts it based on timer wheel granularity so that supplied
> > value effectively becomes an upper bound for the timer.
> > 
> 
> I think there is a problem with this approach. Please correct me, if I'm
> wrong. The timer wheel index and level calculation depends on
> timer_base::clk. The timeout/delta which is used for this calculation is
> relative to timer_base::clk (delta = expires - base::clk). timer_base::clk
> is not updated in sync with jiffies. It is forwarded before a new timer is
> queued. It is possible, that timer_base::clk is behind jiffies after
> forwarding because of a not yet expired timer.
> 
> When calculating the level/index with a relative timeout, there is no
> guarantee that the result is the same when actual enqueueing the timer with
> expiry = jiffies + timeout .

Yes, you are correct. This especially is a problem for timeouts placed
just before LVL_START(x), which is a good chunk of cases. I don't think
it is possible to get to timer_base clock without meddling with the
hot-path.

Is it possible to determine the upper limit of error margin here? My
assumption is it shouldn't be very big, so maybe it would be enough to
account for this when adjusting timeout at the edge of a level.
I know this doesn't sound good but I am running out of ideas here.

-- 
 Artem