[Openvpn-devel,v3] Remove an indention layer from multi_process_incoming_link

Message ID 20260628150235.13113-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v3] Remove an indention layer from multi_process_incoming_link |

Commit Message

Gert Doering June 28, 2026, 3:02 p.m. UTC
  From: Arne Schwabe <arne@rfc2549.org>

This also move the gc area acquire release to the small parts where they
are actually used.

Change-Id: I401aab94993b62bf18e66561532085f99e62f745
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1721
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1721
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <frank@lichtenheld.com>
  

Comments

Gert Doering June 28, 2026, 8:58 p.m. UTC | #1
Yeah, reasonable change "this is the new style how we do early exits" :-)
- and "git show -w" confirms that the difference is indeed "indent,
early exit, and localized GCs" - which makes sense to not have the GC
malloc()/free() in the fast path.

Only lightly tested by me and BB.

Your patch has been applied to the master branch.

commit ce9c3a265b44ec0546f7ae51beac88e839618d59
Author: Arne Schwabe
Date:   Sun Jun 28 17:02:30 2026 +0200

     Remove an indention layer from multi_process_incoming_link

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1721
     Message-Id: <20260628150235.13113-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37341.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
  

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index b4dc13d..f19433b 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3311,8 +3311,6 @@ 
 multi_process_incoming_link(struct multi_context *m, struct multi_instance *instance,
                             const unsigned int mpp_flags, struct link_socket *sock)
 {
-    struct gc_arena gc = gc_new();
-
     struct context *c;
     struct mroute_addr src, dest;
     unsigned int mroute_flags;
@@ -3337,167 +3335,171 @@ 
         multi_set_pending(m, instance);
     }
 
-    if (m->pending)
+    if (!m->pending)
     {
-        set_prefix(m->pending);
+        return true;
+    }
+    set_prefix(m->pending);
 
-        /* get instance context */
-        c = &m->pending->context;
+    /* get instance context */
+    c = &m->pending->context;
 
-        if (!instance)
+    if (!instance)
+    {
+        /* transfer packet pointer from top-level context buffer to instance */
+        c->c2.buf = m->top.c2.buf;
+
+        /* transfer from-addr from top-level context buffer to instance */
+        if (!floated)
         {
-            /* transfer packet pointer from top-level context buffer to instance */
-            c->c2.buf = m->top.c2.buf;
+            c->c2.from = m->top.c2.from;
+        }
+    }
 
-            /* transfer from-addr from top-level context buffer to instance */
-            if (!floated)
+    if (BLEN(&c->c2.buf) > 0)
+    {
+        struct link_socket_info *lsi;
+        const uint8_t *orig_buf;
+
+        /* decrypt in instance context */
+
+        lsi = &sock->info;
+        orig_buf = c->c2.buf.data;
+        if (process_incoming_link_part1(c, lsi, floated))
+        {
+            /* nonzero length means that we have a valid, decrypted packed */
+            if (floated && c->c2.buf.len > 0)
             {
-                c->c2.from = m->top.c2.from;
+                multi_process_float(m, m->pending, sock);
             }
+
+            process_incoming_link_part2(c, lsi, orig_buf);
         }
 
-        if (BLEN(&c->c2.buf) > 0)
+        if (TUNNEL_TYPE(m->top.c1.tuntap) == DEV_TYPE_TUN)
         {
-            struct link_socket_info *lsi;
-            const uint8_t *orig_buf;
+            /* extract packet source and dest addresses */
+            mroute_flags =
+                mroute_extract_addr_from_packet(&src, &dest, 0, &c->c2.to_tun, DEV_TYPE_TUN);
 
-            /* decrypt in instance context */
-
-            lsi = &sock->info;
-            orig_buf = c->c2.buf.data;
-            if (process_incoming_link_part1(c, lsi, floated))
+            /* drop packet if extract failed */
+            if (!(mroute_flags & MROUTE_EXTRACT_SUCCEEDED))
             {
-                /* nonzero length means that we have a valid, decrypted packed */
-                if (floated && c->c2.buf.len > 0)
-                {
-                    multi_process_float(m, m->pending, sock);
-                }
-
-                process_incoming_link_part2(c, lsi, orig_buf);
+                c->c2.to_tun.len = 0;
             }
-
-            if (TUNNEL_TYPE(m->top.c1.tuntap) == DEV_TYPE_TUN)
+            /* make sure that source address is associated with this client */
+            else if (multi_get_instance_by_virtual_addr(m, &src, true) != m->pending)
             {
-                /* extract packet source and dest addresses */
-                mroute_flags =
-                    mroute_extract_addr_from_packet(&src, &dest, 0, &c->c2.to_tun, DEV_TYPE_TUN);
-
-                /* drop packet if extract failed */
-                if (!(mroute_flags & MROUTE_EXTRACT_SUCCEEDED))
+                /* IPv6 link-local address (fe80::xxx)? */
+                if ((src.type & MR_ADDR_MASK) == MR_ADDR_IPV6
+                    && IN6_IS_ADDR_LINKLOCAL(&src.v6.addr))
                 {
-                    c->c2.to_tun.len = 0;
+                    /* do nothing, for now.  TODO: add address learning */
                 }
-                /* make sure that source address is associated with this client */
-                else if (multi_get_instance_by_virtual_addr(m, &src, true) != m->pending)
+                else
                 {
-                    /* IPv6 link-local address (fe80::xxx)? */
-                    if ((src.type & MR_ADDR_MASK) == MR_ADDR_IPV6
-                        && IN6_IS_ADDR_LINKLOCAL(&src.v6.addr))
-                    {
-                        /* do nothing, for now.  TODO: add address learning */
-                    }
-                    else
-                    {
-                        msg(D_MULTI_DROPPED,
-                            "MULTI: bad source address from client [%s], packet dropped",
-                            mroute_addr_print(&src, &gc));
-                    }
-                    c->c2.to_tun.len = 0;
+                    struct gc_arena gc = gc_new();
+                    msg(D_MULTI_DROPPED,
+                        "MULTI: bad source address from client [%s], packet dropped",
+                        mroute_addr_print(&src, &gc));
+                    gc_free(&gc);
                 }
-                /* client-to-client communication enabled? */
-                else if (m->enable_c2c)
+                c->c2.to_tun.len = 0;
+            }
+            /* client-to-client communication enabled? */
+            else if (m->enable_c2c)
+            {
+                /* multicast? */
+                if (mroute_flags & MROUTE_EXTRACT_MCAST)
                 {
-                    /* multicast? */
-                    if (mroute_flags & MROUTE_EXTRACT_MCAST)
-                    {
-                        /* for now, treat multicast as broadcast */
-                        multi_bcast(m, &c->c2.to_tun, m->pending, 0);
-                    }
-                    else /* possible client to client routing */
-                    {
-                        ASSERT(!(mroute_flags & MROUTE_EXTRACT_BCAST));
-                        mi = multi_get_instance_by_virtual_addr(m, &dest, true);
+                    /* for now, treat multicast as broadcast */
+                    multi_bcast(m, &c->c2.to_tun, m->pending, 0);
+                }
+                else /* possible client to client routing */
+                {
+                    ASSERT(!(mroute_flags & MROUTE_EXTRACT_BCAST));
+                    mi = multi_get_instance_by_virtual_addr(m, &dest, true);
 
-                        /* if dest addr is a known client, route to it */
-                        if (mi)
+                    /* if dest addr is a known client, route to it */
+                    if (mi)
+                    {
                         {
+                            multi_unicast(m, &c->c2.to_tun, mi);
+                            register_activity(c, BLEN(&c->c2.to_tun));
+                        }
+                        c->c2.to_tun.len = 0;
+                    }
+                }
+            }
+        }
+        else if (TUNNEL_TYPE(m->top.c1.tuntap) == DEV_TYPE_TAP)
+        {
+            uint16_t vid = 0;
+
+            if (m->top.options.vlan_tagging)
+            {
+                if (vlan_is_tagged(&c->c2.to_tun))
+                {
+                    /* Drop VLAN-tagged frame. */
+                    msg(D_VLAN_DEBUG, "dropping incoming VLAN-tagged frame");
+                    c->c2.to_tun.len = 0;
+                }
+                else
+                {
+                    vid = c->options.vlan_pvid;
+                }
+            }
+            /* extract packet source and dest addresses */
+            mroute_flags =
+                mroute_extract_addr_from_packet(&src, &dest, vid, &c->c2.to_tun, DEV_TYPE_TAP);
+
+            if (mroute_flags & MROUTE_EXTRACT_SUCCEEDED)
+            {
+                if (multi_learn_addr(m, m->pending, &src, 0) == m->pending)
+                {
+                    /* check for broadcast */
+                    if (m->enable_c2c)
+                    {
+                        if (mroute_flags & (MROUTE_EXTRACT_BCAST | MROUTE_EXTRACT_MCAST))
+                        {
+                            multi_bcast(m, &c->c2.to_tun, m->pending, vid);
+                        }
+                        else /* try client-to-client routing */
+                        {
+                            mi = multi_get_instance_by_virtual_addr(m, &dest, false);
+
+                            /* if dest addr is a known client, route to it */
+                            if (mi)
                             {
                                 multi_unicast(m, &c->c2.to_tun, mi);
                                 register_activity(c, BLEN(&c->c2.to_tun));
-                            }
-                            c->c2.to_tun.len = 0;
-                        }
-                    }
-                }
-            }
-            else if (TUNNEL_TYPE(m->top.c1.tuntap) == DEV_TYPE_TAP)
-            {
-                uint16_t vid = 0;
-
-                if (m->top.options.vlan_tagging)
-                {
-                    if (vlan_is_tagged(&c->c2.to_tun))
-                    {
-                        /* Drop VLAN-tagged frame. */
-                        msg(D_VLAN_DEBUG, "dropping incoming VLAN-tagged frame");
-                        c->c2.to_tun.len = 0;
-                    }
-                    else
-                    {
-                        vid = c->options.vlan_pvid;
-                    }
-                }
-                /* extract packet source and dest addresses */
-                mroute_flags =
-                    mroute_extract_addr_from_packet(&src, &dest, vid, &c->c2.to_tun, DEV_TYPE_TAP);
-
-                if (mroute_flags & MROUTE_EXTRACT_SUCCEEDED)
-                {
-                    if (multi_learn_addr(m, m->pending, &src, 0) == m->pending)
-                    {
-                        /* check for broadcast */
-                        if (m->enable_c2c)
-                        {
-                            if (mroute_flags & (MROUTE_EXTRACT_BCAST | MROUTE_EXTRACT_MCAST))
-                            {
-                                multi_bcast(m, &c->c2.to_tun, m->pending, vid);
-                            }
-                            else /* try client-to-client routing */
-                            {
-                                mi = multi_get_instance_by_virtual_addr(m, &dest, false);
-
-                                /* if dest addr is a known client, route to it */
-                                if (mi)
-                                {
-                                    multi_unicast(m, &c->c2.to_tun, mi);
-                                    register_activity(c, BLEN(&c->c2.to_tun));
-                                    c->c2.to_tun.len = 0;
-                                }
+                                c->c2.to_tun.len = 0;
                             }
                         }
                     }
-                    else
-                    {
-                        msg(D_MULTI_DROPPED,
-                            "MULTI: bad source address from client [%s], packet dropped",
-                            mroute_addr_print(&src, &gc));
-                        c->c2.to_tun.len = 0;
-                    }
                 }
                 else
                 {
+                    struct gc_arena gc = gc_new();
+                    msg(D_MULTI_DROPPED,
+                        "MULTI: bad source address from client [%s], packet dropped",
+                        mroute_addr_print(&src, &gc));
                     c->c2.to_tun.len = 0;
+                    gc_free(&gc);
                 }
             }
+            else
+            {
+                c->c2.to_tun.len = 0;
+            }
         }
-
-        /* postprocess and set wakeup */
-        ret = multi_process_post(m, m->pending, mpp_flags);
-
-        clear_prefix();
     }
 
-    gc_free(&gc);
+    /* postprocess and set wakeup */
+    ret = multi_process_post(m, m->pending, mpp_flags);
+
+    clear_prefix();
+
     return ret;
 }
 
@@ -3539,7 +3541,6 @@ 
          * Route an incoming tun/tap packet to
          * the appropriate multi_instance object.
          */
-
         mroute_flags = mroute_extract_addr_from_packet(&src, &dest, vid, &m->top.c2.buf, dev_type);
 
         if (mroute_flags & MROUTE_EXTRACT_SUCCEEDED)