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

[Sabrina Dubroca <sd@queasysnail.net>]

2024-07-17, 17:30:17 +0200, Antonio Quartulli wrote:
> Hi,
> 
> On 15/07/2024 16:44, Sabrina Dubroca wrote:
> > 2024-06-27, 15:08:35 +0200, Antonio Quartulli wrote:
> > > +/**
> > > + * ovpn_xmit_special - encrypt and transmit an out-of-band message to peer
> > > + * @peer: peer to send the message to
> > > + * @data: message content
> > > + * @len: message length
> > > + *
> > > + * Assumes that caller holds a reference to peer
> > > + */
> > > +static void ovpn_xmit_special(struct ovpn_peer *peer, const void *data,
> > > +			      const unsigned int len)
> > > +{
> > > +	struct ovpn_struct *ovpn;
> > > +	struct sk_buff *skb;
> > > +
> > > +	ovpn = peer->ovpn;
> > > +	if (unlikely(!ovpn))
> > > +		return;
> > > +
> > > +	skb = alloc_skb(256 + len, GFP_ATOMIC);
> > 
> > Where is that 256 coming from?
> 
> "Reasonable number" which should be enough[tm] to hold the entire packet.

Ok, let's go with that for now, unless someone else wants you to
change it.

> > > +	if (unlikely(!skb))
> > > +		return;
> > 
> > Failure to send a keepalive should probably have a counter, to help
> > users troubleshoot why their connection dropped.
> > (can be done later unless someone insists)
> 
> This will be part of a more sophisticated error counting that I will
> introduce later on.

Cool, thanks.


> > > +/**
> > > + * ovpn_peer_keepalive_set - configure keepalive values for peer
> > > + * @peer: the peer to configure
> > > + * @interval: outgoing keepalive interval
> > > + * @timeout: incoming keepalive timeout
> > > + */
> > > +void ovpn_peer_keepalive_set(struct ovpn_peer *peer, u32 interval, u32 timeout)
> > > +{
> > > +	u32 delta;
> > > +
> > > +	netdev_dbg(peer->ovpn->dev,
> > > +		   "%s: scheduling keepalive for peer %u: interval=%u timeout=%u\n",
> > > +		   __func__, peer->id, interval, timeout);
> > > +
> > > +	peer->keepalive_interval = interval;
> > > +	if (interval > 0) {
> > > +		delta = msecs_to_jiffies(interval * MSEC_PER_SEC);
> > > +		mod_timer(&peer->keepalive_xmit, jiffies + delta);
> > 
> > Maybe something to consider in the future: this could be resetting a
> > timer that was just about to go off to a somewhat distant time in the
> > future. Not sure the peer will be happy about that (and not consider
> > it a timeout).
> 
> Normally this timer is only set upon connection, or maybe upon some future
> parameter exchange. In both cases we can assume the connection is alive, so
> this case should not scare us.
> 
> But thanks for pointing it out

Ok, I was thinking about updates while the connection is fully
established. If it's only done during setup, it shouldn't be a
problem.


> > > +/**
> > > + * 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.

-- 
Sabrina