On 17/07/2024 22:40, Sabrina Dubroca wrote:

quoted

quoted

quoted

+/**
+ * ovpn_peer_keepalive_recv_reset - reset keepalive timeout
+ * @peer: peer for which the timeout should be reset
+ *
+ * To be invoked upon reception of an authenticated packet from peer in order
+ * to report valid activity and thus reset the keepalive timeout
+ */
+static inline void ovpn_peer_keepalive_recv_reset(struct ovpn_peer *peer)
+{
+	u32 delta = msecs_to_jiffies(peer->keepalive_timeout * MSEC_PER_SEC);
+
+	if (unlikely(!delta))
+		return;
+
+	mod_timer(&peer->keepalive_recv, jiffies + delta);

This (and ovpn_peer_keepalive_xmit_reset) is going to be called for
each packet. I wonder how well the timer subsystem deals with one
timer getting updated possibly thousands of time per second.

May it even introduce some performance penalty?

That's what I was worried about, yes.

I asked Paolo, he suggested checking that we're actually doing any
change to the timer:

    if (new_timeout_time != old_timeout_time)
        mod_timer(...)

This would reduce the update frequency to one per jiffy, which should
be acceptable.

quoted

Maybe we should get rid of the timer object and introduce a periodic (1s)
worker which checks some last_recv timestamp on every known peer?
What do you think?

That should work, or the workqueue like Eyal is saying.

I will go with Eyal's approach.
This way we also eliminate any timer related operation from the fast path.


Cheers!



-- 
Antonio Quartulli
OpenVPN Inc.