diff mbox series

[Openvpn-devel,v4] dco-freebsd: use m->instances[] instead of m->hash

Message ID 20230323080341.51624-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v4] dco-freebsd: use m->instances[] instead of m->hash | expand

Commit Message

Gert Doering March 23, 2023, 8:03 a.m. UTC 
From: Antonio Quartulli <a@unstable.cc>

When retrieving the multi_instance of a specific peer,
there is no need to peform a linear search across the
whole m->hash list. We can directly access the needed
object via m->instances[peer-id] in constant time (and
just one line of code).

Adapt the dco-freebsd code to do so.

v4: use "peerid" everywhere as that's what FreeBSD does, change message text

Cc: Kristof Provost <kp@FreeBSD.org>
Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/dco_freebsd.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

Comments

Antonio Quartulli March 23, 2023, 8:26 a.m. UTC | #1 
Hi,

On 23/03/2023 09:03, Gert Doering wrote:
> From: Antonio Quartulli <a@unstable.cc>
> 
> When retrieving the multi_instance of a specific peer,
> there is no need to peform a linear search across the
> whole m->hash list. We can directly access the needed
> object via m->instances[peer-id] in constant time (and
> just one line of code).
> 
> Adapt the dco-freebsd code to do so.
> 
> v4: use "peerid" everywhere as that's what FreeBSD does, change message text
> 
> Cc: Kristof Provost <kp@FreeBSD.org>
> Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>   src/openvpn/dco_freebsd.c | 22 ++++++----------------
>   1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
> index 225b3cf8..a334d5d2 100644
> --- a/src/openvpn/dco_freebsd.c
> +++ b/src/openvpn/dco_freebsd.c
> @@ -674,27 +674,17 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
>   static void
>   dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl)
>   {
> -    struct hash_element *he;
> -    struct hash_iterator hi;
>   
> -    hash_iterator_init(m->hash, &hi);
> -
> -    while ((he = hash_iterator_next(&hi)))
> +    if (peerid >= m->max_clients || !m->instances[peerid])
>       {
> -        struct multi_instance *mi = (struct multi_instance *) he->value;
> -
> -        if (mi->context.c2.tls_multi->peer_id != peerid)
> -        {
> -            continue;
> -        }
> -
> -        mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
> -        mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
> -
> +        msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by kernel", peerid);

I'd prefer to use __func__ here, but it is noted that dco_freebsd.c 
seldomly uses this approach.
For this reason I think hardcoding the function name is fine for now.

Later on we can do a full cleanup.

>           return;
>       }
>   
> -    msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
> +    struct multi_instance *mi = m->instances[peerid];
> +
> +    mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
> +    mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
>   }
>   
>   int

This patch is basically my v3 with the peerid fixed and the message changed.

I can't ACK since this patch bears my own signature, but I am fine with 
the applied modification.

Cheers,
Arne Schwabe March 23, 2023, 8:30 a.m. UTC | #2 
Am 23.03.23 um 09:03 schrieb Gert Doering:
> From: Antonio Quartulli <a@unstable.cc>
> 
> When retrieving the multi_instance of a specific peer,
> there is no need to peform a linear search across the
> whole m->hash list. We can directly access the needed
> object via m->instances[peer-id] in constant time (and
> just one line of code).
> 
> Adapt the dco-freebsd code to do so.
> 
> v4: use "peerid" everywhere as that's what FreeBSD does, change message text
> 
> Cc: Kristof Provost <kp@FreeBSD.org>
> Change-Id: I8d8af6f872146604a9710edf443db65df48ac3cb
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>   src/openvpn/dco_freebsd.c | 22 ++++++----------------
>   1 file changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
> index 225b3cf8..a334d5d2 100644
> --- a/src/openvpn/dco_freebsd.c
> +++ b/src/openvpn/dco_freebsd.c
> @@ -674,27 +674,17 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
>   static void
>   dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl)
>   {
> -    struct hash_element *he;
> -    struct hash_iterator hi;
>   
> -    hash_iterator_init(m->hash, &hi);
> -
> -    while ((he = hash_iterator_next(&hi)))
> +    if (peerid >= m->max_clients || !m->instances[peerid])
>       {
> -        struct multi_instance *mi = (struct multi_instance *) he->value;
> -
> -        if (mi->context.c2.tls_multi->peer_id != peerid)
> -        {
> -            continue;
> -        }
> -
> -        mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
> -        mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
> -
> +        msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by kernel", peerid);
>           return;
>       }
>   
> -    msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
> +    struct multi_instance *mi = m->instances[peerid];
> +
> +    mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
> +    mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
>   }
>   
>   int

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering March 23, 2023, 8:39 a.m. UTC | #3 
As this patch has a bit of mixed history "who wrote it, who ACKed it,
what happened afterwards" I decided to record the ACK from Arne and 
Kristof.

v4 has been tested on FreeBSD with DCO enabled, p2mp udp server, one client
being connected all the time and the other client reconnecting (moving
between peer-id 1 and 2), packets going back and forth, looking at the
resulting counters.  Everything looked as expected.

(This is basically an optimization that was discussed in November
already but nobody followed up on it - and with the upcoming Linux DCO
counter patches, the topic "why use iterator when you have an array?"
resurfaced - thanks, Antonio, for covering FreeBSD too)

Your patch has been applied to the master and release/2.6 branch.

commit 03145f223236df90b35d1db444319fd3f785792b (master)
commit 5acefd944c6a8a4338b5105ffe6b9ffce6bad330 (release/2.6)
Author: Antonio Quartulli
Date:   Thu Mar 23 09:03:41 2023 +0100

     dco-freebsd: use m->instances[] instead of m->hash

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Kristof Provost <kp@freebsd.org>
     Message-Id: <20230323080341.51624-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/search?l=mid&q=20230323080341.51624-1-gert@greenie.muc.de
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index 225b3cf8..a334d5d2 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -674,27 +674,17 @@  dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
 static void
 dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl)
 {
-    struct hash_element *he;
-    struct hash_iterator hi;
 
-    hash_iterator_init(m->hash, &hi);
-
-    while ((he = hash_iterator_next(&hi)))
+    if (peerid >= m->max_clients || !m->instances[peerid])
     {
-        struct multi_instance *mi = (struct multi_instance *) he->value;
-
-        if (mi->context.c2.tls_multi->peer_id != peerid)
-        {
-            continue;
-        }
-
-        mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
-        mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
-
+        msg(M_WARN, "dco_update_peer_stat: invalid peer ID %d returned by kernel", peerid);
         return;
     }
 
-    msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
+    struct multi_instance *mi = m->instances[peerid];
+
+    mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
+    mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
 }
 
 int