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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/11/2024 02:18, Sergey Ryazanov wrote:
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.

Thanks and welcome back!

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.

I'll go through them all and judge what's meaningful to add to v12 and what can be postponed for later improvements.


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.

True, but to be honest at this point I am fine with sticking to RTNL, also because we will soon introduce the ability to create 'persistent' ifaces, which a user should be able to create before starting openvpn.

Going through RTNL for this is the best choice IMHO, therefore we have an extra use case in favour of this approach (next to what Jiri already mentioned).


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.

I see this was already discussed with Sabrina under another patch and I have the same opinion.

To me the switch/case approach looks cleaner and I truly like it, especially when enums are involved.

ops/callbacks are fine when they can be redefined at runtime (i.e. a proto that can be registered by another module), but this is not the case here. I also feel that with ops it's not easy to understand what call is truly being made by just looking at the caller context and reading can be harder.

So I truly prefer to stick to this schema.

Thanks a lot for sharing your point though.

Regards,

--
Antonio Quartulli
OpenVPN Inc.





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux