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

[Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>]

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@xxxxxxxxxxx
>
> 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