Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 30 Mar 2022 15:40:55 +0200 (CEST)
From:   Anna-Maria Behnsen <anna-maria@linutronix.de>
To:     Artem Savkov <asavkov@redhat.com>
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
In-Reply-To: <20220330082046.3512424-2-asavkov@redhat.com>
Message-ID: <alpine.DEB.2.21.2203301514570.4409@somnus>
References: <87zglcfmcv.ffs@tglx> <20220330082046.3512424-1-asavkov@redhat.com> <20220330082046.3512424-2-asavkov@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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 .

Thanks,

	Anna-Maria