Re: [PATCH net-next v11 00/23] Introducing OpenVPN Data Channel Offload

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

Hi Antonio,

On 29.10.2024 12:47, Antonio Quartulli wrote:
> Notable changes from v10:
> * extended commit message of 23/23 with brief description of the output
> * Link to v10: https://lore.kernel.org/r/20241025-b4-ovpn-v10-0-b87530777be7@openvpn.net
> 
> Please note that some patches were already reviewed by Andre Lunn,
> Donald Hunter and Shuah Khan. They have retained the Reviewed-by tag
> since no major code modification has happened since the review.
> 
> The latest code can also be found at:
> 
> https://github.com/OpenVPN/linux-kernel-ovpn

As I promised many months ago I am starting publishing some nit picks 
regarding the series. The review was started when series was V3 
"rebasing" the review to every next version to publish it at once. But I 
lost this race to the new version releasing velocity :) So, I am going 
to publish it patch-by-patch.

Anyway you and all participants have done a great progress toward making 
accelerator part of the kernel. Most of considerable things already 
resolved so do not wait me please to finish picking every nit.

Regarding "big" topics I have only two concerns: link creation using 
RTNL and a switch statement usage. In the corresponding thread, I asked 
Jiri to clarify that "should" regarding .newlink implementation. Hope he 
will have a chance to find a time to reply.

For the 'switch' statement, I see a repeating pattern of handling 
mode-or family-specific cases like this:

int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer)
{
   switch (ovpn->mode) {
   case OVPN_MODE_MP:
     return ovpn_peer_add_mp(ovpn, peer);
   case OVPN_MODE_P2P:
     return ovpn_peer_add_p2p(ovpn, peer);
   default:
     return -EOPNOTSUPP;
   }
}

or

void ovpn_encrypt_post(void *data, int ret)
{
   ...
   switch (peer->sock->sock->sk->sk_protocol) {
   case IPPROTO_UDP:
     ovpn_udp_send_skb(peer->ovpn, peer, skb);
     break;
   case IPPROTO_TCP:
     ovpn_tcp_send_skb(peer, skb);
     break;
   default:
     /* no transport configured yet */
     goto err;
   }
   ...
}

or

void ovpn_peer_keepalive_work(...)
{
   ...
   switch (ovpn->mode) {
   case OVPN_MODE_MP:
     next_run = ovpn_peer_keepalive_work_mp(ovpn, now);
     break;
   case OVPN_MODE_P2P:
     next_run = ovpn_peer_keepalive_work_p2p(ovpn, now);
     break;
   }
   ...
}

Did you consider to implement mode specific operations as a set of 
operations like this:

ovpn_ops {
   int (*peer_add)(struct ovpn_struct *ovpn, struct ovpn_peer *peer);
   int (*peer_del)(struct ovpn_peer *peer, enum ovpn_del_peer_reason 
reason);
   void (*send_skb)(struct ovpn_peer *peer, struct sk_buff *skb);
   time64_t (*keepalive_work)(...);
};

Initialize them during the interface creation and invoke these 
operations indirectly. E.g.

int ovpn_peer_add(struct ovpn_struct *ovpn, struct ovpn_peer *peer)
{
   return ovpn->ops->peer_add(ovpn, peer);
}

void ovpn_encrypt_post(void *data, int ret)
{
   ...
   ovpn->ops->send_skb(peer, skb);
   ...
}

void ovpn_peer_keepalive_work(...)
{
   ...
   next_run = ovpn->ops->keepalive_work(ovpn, now);
   ...
}

Anyway the module has all these option values in advance during the 
network interface creation phase and I believe replacing 'switch' 
statements with indirect calls can make code easy to read.

--
Sergey