Re: [PATCH net-next v5 17/25] ovpn: implement keepalive mechanism

[Antonio Quartulli <antonio@openvpn.net>]

On 17/07/2024 22:40, Sabrina Dubroca wrote:
>>>> +/**
>>>> + * 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.
> 
>> 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.