Message-ID: <a7009e7e-a1f9-4aa5-ad41-2befc64b5d3e@openvpn.net>
Date: Thu, 14 Nov 2024 16:33:01 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, sd@...asysnail.net,
 Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 steffen.klassert@...unet.com, antony.antony@...unet.com,
 Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH net-next v11 00/23] Introducing OpenVPN Data Channel
 Offload

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@...nvpn.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. 

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.

Powered by blists - more mailing lists