[Openvpn-devel] [S] Change in openvpn[master]: Optimise performance of iterating over all client by remembering high...

[Openvpn-devel] [S] Change in openvpn[master]: Optimise performance of iterating over all client by remembering high...

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5558 bytes --]

plaisthos has uploaded this change for review. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )


Change subject: Optimise performance of iterating over all client by remembering highest peer id
......................................................................

Optimise performance of iterating over all client by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 24 insertions(+), 9 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/57/1557/1

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..5a7bc82 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
 
@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4099,6 +4106,11 @@
         }
     }

+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
+
     /* should not really end up here, since multi_create_instance returns null
      * if amount of clients exceeds max_clients */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..17d850b 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< currently highest allocated peerid and
+                                        * maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>

[-- Attachment #2: Type: text/html, Size: 9619 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise performance of iterating over all client by remembering high...

[flichtenheld (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )

Change subject: Optimise performance of iterating over all client by remembering highest peer id
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

File src/openvpn/multi.c:

http://gerrit.openvpn.net/c/openvpn/+/1557/comment/d4a87d22_1c3297f5?usp=email :
PS1, Line 4109:     if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
I would suggest moving this code below the ASSERT. In the end it makes no difference. But I think it is easier to read if the code that checks that the loop failed is directly below the loop.

As an aside I think the ASSERT is a bit unclear already. I think what it actually should check is against MAX_PEER_ID, not max_clients. From this code it is not clear that it assumes that peer_id was initialized to MAX_PEER_ID. So I think it would be better to have an ASSERT at the top of the function that checks that peer_id is indeed MAX_PEER_ID and then an assert below that checks that peer_id is not MAX_PEER_ID. Much clearer. But not directly the job of this patch.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Comment-Date: Fri, 06 Mar 2026 11:39:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

[-- Attachment #2: Type: text/html, Size: 3189 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 6057 bytes --]

Attention is currently required from: flichtenheld, plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email

to look at the new patch set (#2).

The following approvals got outdated and were removed:
Code-Review-1 by flichtenheld


Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................

Optimise iterating over all clients by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 26 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/57/1557/2

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..653c7e0 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];

@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }

     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }

 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..17d850b 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< currently highest allocated peerid and
+                                        * maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 2
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Attention: flichtenheld <frank@...2641...>

[-- Attachment #2: Type: text/html, Size: 10384 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1394 bytes --]

Attention is currently required from: flichtenheld.

plaisthos has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )

Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................


Patch Set 1:

(1 comment)

File src/openvpn/multi.c:

http://gerrit.openvpn.net/c/openvpn/+/1557/comment/54c3fb30_19036b99?usp=email :
PS1, Line 4109:     if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
> I would suggest moving this code below the ASSERT. In the end it makes no difference. […]
done



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>
Gerrit-Comment-Date: Fri, 06 Mar 2026 13:36:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld <frank@...2641...>

[-- Attachment #2: Type: text/html, Size: 2591 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5908 bytes --]

Attention is currently required from: flichtenheld.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email

to look at the new patch set (#3).


Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................

Optimise iterating over all clients by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 26 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/57/1557/3

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..c03e821 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];

@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }

     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }

 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..17d850b 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< currently highest allocated peerid and
+                                        * maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>

[-- Attachment #2: Type: text/html, Size: 10183 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[flichtenheld (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

Attention is currently required from: plaisthos.

flichtenheld has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )

Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................


Patch Set 3: Code-Review+2

(2 comments)

File src/openvpn/multi.h:

http://gerrit.openvpn.net/c/openvpn/+/1557/comment/cefa47db_0826c6d4?usp=email :
PS3, Line 166:     uint32_t max_peerid;               /**< currently highest allocated peerid and
"highest currently"


File src/openvpn/multi.c:

http://gerrit.openvpn.net/c/openvpn/+/1557/comment/aa64aaec_78ffb9f4?usp=email :
PS1, Line 4109:     if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
> done
Done



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 3
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>
Gerrit-Comment-Date: Fri, 06 Mar 2026 14:01:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: plaisthos <arne-openvpn@...1227...>
Comment-In-Reply-To: flichtenheld <frank@...2641...>

[-- Attachment #2: Type: text/html, Size: 3323 bytes --]

[Openvpn-devel] [PATCH v3] Optimise iterating over all clients by remembering highest peer id

[Gert Doering]

From: Arne Schwabe <arne@...1227...>

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Signed-off-by: Arne Schwabe <arne-openvpn@...1227...>
Acked-by: Frank Lichtenheld <frank@...2641...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1557
---

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/+/1557
This mail reflects revision 3 of this Change.

Signed-off-by line for the author was added as per our policy.

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

        
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..c03e821 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }
 
         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;
 
-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);
 
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
 
@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;
 
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }
 
     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }
 
 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..17d850b 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< currently highest allocated peerid and
+                                        * maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@
 
     int count = 0;
 
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];
 

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5992 bytes --]

Attention is currently required from: plaisthos.

Hello flichtenheld,

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email

to look at the new patch set (#4).

The change is no longer submittable: checks~ChecksSubmitRule is unsatisfied now.


Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................

Optimise iterating over all clients by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 26 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/57/1557/4

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..c03e821 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];

@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }

     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }

 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..17d850b 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< currently highest allocated peerid and
+                                        * maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 4
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-openvpn@...1227...>

[-- Attachment #2: Type: text/html, Size: 10274 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

plaisthos has posted comments on this change by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )

Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................


Patch Set 6:

(1 comment)

File src/openvpn/multi.h:

http://gerrit.openvpn.net/c/openvpn/+/1557/comment/6ce2d185_dc6b762e?usp=email :
PS3, Line 166:     uint32_t max_peerid;               /**< currently highest allocated peerid and
> "highest currently"
I will update this if I have to update the patch again as this has been already sent out as mail.



--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 6
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Comment-Date: Thu, 19 Mar 2026 11:27:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld <frank@...2641...>

[-- Attachment #2: Type: text/html, Size: 2478 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[plaisthos (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 6146 bytes --]

Attention is currently required from: flichtenheld.

Hello flichtenheld, 

I'd like you to reexamine a change. Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email

to look at the new patch set (#7).

The following approvals got outdated and were removed:
Code-Review+2 by flichtenheld

The change is no longer submittable: Code-Review and checks~ChecksSubmitRule are unsatisfied now.


Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................

Optimise iterating over all clients by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Signed-off-by: Arne Schwabe <arne@...1227...>
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 26 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/57/1557/7

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..c03e821 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];

@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }

     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }

 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..4acb364 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< highest currently allocated peer-id
+                                        * and maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 7
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: flichtenheld <frank@...2641...>

[-- Attachment #2: Type: text/html, Size: 10435 bytes --]

[Openvpn-devel] [PATCH v7] Optimise iterating over all clients by remembering highest peer id

[Gert Doering]

From: Arne Schwabe <arne@...1227...>

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Signed-off-by: Arne Schwabe <arne@...1227...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1557
---

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/+/1557
This mail reflects revision 7 of this Change.

Acked-by according to Gerrit (reflected above):

        
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 9d4ea49..c03e821 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }
 
         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;
 
-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);
 
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
 
@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;
 
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }
 
     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }
 
 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 498409d..4acb364 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< highest currently allocated peer-id
+                                        * and maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index 6456554..529cc39 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@
 
     int count = 0;
 
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];
 

[Openvpn-devel] [PATCH applied] Re: Optimise iterating over all clients by remembering highest peer id

[Gert Doering]

Stared at code, all makes sense :-) - plus ACK from Frank, plus successful
passing of the t_server tests...

Technically, there is one wart, though:

+    uint32_t max_peerid;               /**< highest currently allocated peer-id
+                                        * and maximum allocated/valid index in
+                                        * instances */

we start with "0", and that is not technically correct for "no peers
are allocated, no valid index in the table".  In practice, this does
not really matter - since the array can have holes, every walk always
verifies that the elements are valid, so the "there is nothing here"
situation leads to a single look at m->instances[0], which is not 
active, so nothing happens.  It's consistent, though, as the decreasing
algorithm also stops at max_peerid == 0 (no underruns).

Your patch has been applied to the master branch.

commit ab3ba0cab7c38699c38898457f403b9b9a40eb3f
Author: Arne Schwabe
Date:   Sun Apr 12 14:53:50 2026 +0200

     Optimise iterating over all clients by remembering highest peer id

     Signed-off-by: Arne Schwabe <arne@...1227...>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1557
     Message-Id: <20260412125356.32261-1-gert@...1296...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg36577.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[cron2 (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 6066 bytes --]

cron2 has uploaded a new patch set (#8) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )


Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................

Optimise iterating over all clients by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Signed-off-by: Arne Schwabe <arne@...1227...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1557
Message-Id: <20260412125356.32261-1-gert@...1296...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg36577.html
Signed-off-by: Gert Doering <gert@...1296...>
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 26 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/57/1557/8

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 0b03ff1..c7a91ab 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];

@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }

     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }

 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 3f4eefe..935bda1 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< highest currently allocated peer-id
+                                        * and maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index fe48091..f57f54d 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 8
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>

[-- Attachment #2: Type: text/html, Size: 10371 bytes --]

[Openvpn-devel] [S] Change in openvpn[master]: Optimise iterating over all clients by remembering highest peer id

[cron2 (Code Review)]

[-- Attachment #1: Type: text/plain, Size: 5933 bytes --]

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email )

Change subject: Optimise iterating over all clients by remembering highest peer id
......................................................................

Optimise iterating over all clients by remembering highest peer id

This keeps track of the highest peer id that is currently allocated to avoid
iterating over the empty tail of the m->instances array.

Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Signed-off-by: Arne Schwabe <arne@...1227...>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1557
Message-Id: <20260412125356.32261-1-gert@...1296...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg36577.html
Signed-off-by: Gert Doering <gert@...1296...>
---
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/push_util.c
3 files changed, 26 insertions(+), 10 deletions(-)




diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 0b03ff1..c7a91ab 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -601,6 +601,13 @@
         if (mi->context.c2.tls_multi->peer_id != MAX_PEER_ID)
         {
             m->instances[mi->context.c2.tls_multi->peer_id] = NULL;
+
+            /* Adjust the max_peerid as this might have been the highest
+             * peer id instance */
+            while (m->max_peerid > 0 && m->instances[m->max_peerid] == NULL)
+            {
+                m->max_peerid--;
+            }
         }

         schedule_remove_entry(m->schedule, (struct schedule_entry *)mi);
@@ -652,7 +659,7 @@
 {
     if (m->hash)
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi)
@@ -1326,7 +1333,7 @@
         {
             int count = 0;

-            for (int i = 0; i < m->max_clients; i++)
+            for (uint32_t i = 0; i <= m->max_peerid; i++)
             {
                 struct multi_instance *mi = m->instances[i];
                 if (mi && mi != new_mi && !mi->halt)
@@ -2885,7 +2892,7 @@
 #endif
         mb = mbuf_alloc_buf(buf);

-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];

@@ -3794,7 +3801,7 @@
 multi_push_restart_schedule_exit(struct multi_context *m, bool next_server)
 {
     /* tell all clients to restart */
-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt && proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
@@ -3876,7 +3883,7 @@
     struct multi_context *m = (struct multi_context *)arg;
     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *mi = m->instances[i];
         if (mi && !mi->halt)
@@ -3907,7 +3914,7 @@
     maddr.proto = proto;
     if (mroute_extract_openvpn_sockaddr(&maddr, &saddr, true))
     {
-        for (int i = 0; i < m->max_clients; i++)
+        for (uint32_t i = 0; i <= m->max_peerid; i++)
         {
             struct multi_instance *mi = m->instances[i];
             if (mi && !mi->halt && mroute_addr_equal(&maddr, &mi->real))
@@ -4100,8 +4107,14 @@
     }

     /* should not really end up here, since multi_create_instance returns null
-     * if amount of clients exceeds max_clients */
+     * if amount of clients exceeds max_clients and this method would then
+     * also not have been called */
     ASSERT(mi->context.c2.tls_multi->peer_id < m->max_clients);
+
+    if (mi->context.c2.tls_multi->peer_id > m->max_peerid)
+    {
+        m->max_peerid = mi->context.c2.tls_multi->peer_id;
+    }
 }

 #if defined(__GNUC__) || defined(__clang__)
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 3f4eefe..935bda1 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -160,9 +160,12 @@
  */
 struct multi_context
 {
-    struct multi_instance **instances; /**< Array of multi_instances. An instance can be
+    struct multi_instance **instances; /**< Array of multi_instances with the size of
+                                        *  max_clients.  An instance can be
                                         * accessed using peer-id as an index. */
-
+    uint32_t max_peerid;               /**< highest currently allocated peer-id
+                                        * and maximum allocated/valid index in
+                                        * instances */
     struct hash *hash;                 /**< VPN tunnel instances indexed by real
                                         *   address of the remote peer. */
     struct hash *vhash;                /**< VPN tunnel instances indexed by
diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c
index fe48091..f57f54d 100644
--- a/src/openvpn/push_util.c
+++ b/src/openvpn/push_util.c
@@ -317,7 +317,7 @@

     int count = 0;

-    for (int i = 0; i < m->max_clients; i++)
+    for (uint32_t i = 0; i <= m->max_peerid; i++)
     {
         struct multi_instance *curr_mi = m->instances[i];


--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1557?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: If797f3fe178fba3f43fb12898e5484bfb38f05c3
Gerrit-Change-Number: 1557
Gerrit-PatchSet: 8
Gerrit-Owner: plaisthos <arne-openvpn@...1227...>
Gerrit-Reviewer: flichtenheld <frank@...2641...>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>

[-- Attachment #2: Type: text/html, Size: 10201 bytes --]