389-ds-base/SOURCES/0006-Ticket-51131-improve-mutex-alloc-in-conntable.patch

From 1085823bf5586d55103cfba249fdf212e9afcb7c Mon Sep 17 00:00:00 2001
From: William Brown <william@blackhats.net.au>
Date: Thu, 4 Jun 2020 11:51:53 +1000
Subject: [PATCH] Ticket 51131 - improve mutex alloc in conntable

Bug Description: We previously did delayed allocation
of mutexs, which @tbordaz noted can lead to high usage
of the pthread mutex init routines. This was done under
the conntable lock, as well as cleaning the connection

Fix Description: rather than delayed allocation, we
initialise everything at start up instead, which means
that while startup may have a delay, at run time we have
a smaller and lighter connection allocation routine,
that is able to release the CT lock sooner.

https://pagure.io/389-ds-base/issue/51131

Author: William Brown <william@blackhats.net.au>

Review by: ???
---
 ldap/servers/slapd/conntable.c | 86 +++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 39 deletions(-)

diff --git a/ldap/servers/slapd/conntable.c b/ldap/servers/slapd/conntable.c
index b23dc3435..feb9c0d75 100644
--- a/ldap/servers/slapd/conntable.c
+++ b/ldap/servers/slapd/conntable.c
@@ -138,10 +138,21 @@ connection_table_new(int table_size)
     ct->conn_next_offset = 1;
     ct->conn_free_offset = 1;
 
+    pthread_mutexattr_t monitor_attr = {0};
+    pthread_mutexattr_init(&monitor_attr);
+    pthread_mutexattr_settype(&monitor_attr, PTHREAD_MUTEX_RECURSIVE);
+
     /* We rely on the fact that we called calloc, which zeros the block, so we don't
      * init any structure element unless a zero value is troublesome later
      */
     for (i = 0; i < table_size; i++) {
+        /*
+         * Technically this is a no-op due to calloc, but we should always be
+         * careful with things like this ....
+         */
+        ct->c[i].c_state = CONN_STATE_FREE;
+        /* Start the conn setup. */
+
         LBER_SOCKET invalid_socket;
         /* DBDB---move this out of here once everything works */
         ct->c[i].c_sb = ber_sockbuf_alloc();
@@ -161,11 +172,20 @@ connection_table_new(int table_size)
         ct->c[i].c_prev = NULL;
         ct->c[i].c_ci = i;
         ct->c[i].c_fdi = SLAPD_INVALID_SOCKET_INDEX;
-        /*
-         * Technically this is a no-op due to calloc, but we should always be
-         * careful with things like this ....
-         */
-        ct->c[i].c_state = CONN_STATE_FREE;
+
+        if (pthread_mutex_init(&(ct->c[i].c_mutex), &monitor_attr) != 0) {
+            slapi_log_err(SLAPI_LOG_ERR, "connection_table_get_connection", "pthread_mutex_init failed\n");
+            exit(1);
+        }
+
+        ct->c[i].c_pdumutex = PR_NewLock();
+        if (ct->c[i].c_pdumutex == NULL) {
+            slapi_log_err(SLAPI_LOG_ERR, "connection_table_get_connection", "PR_NewLock failed\n");
+            exit(1);
+        }
+
+        /* Ready to rock, mark as such. */
+        ct->c[i].c_state = CONN_STATE_INIT;
         /* Prepare the connection into the freelist. */
         ct->c_freelist[i] = &(ct->c[i]);
     }
@@ -241,44 +261,32 @@ connection_table_get_connection(Connection_Table *ct, int sd)
             /* Never use slot 0 */
             ct->conn_next_offset += 1;
         }
-        /* Now prep the slot for usage. */
-        PR_ASSERT(c->c_next == NULL);
-        PR_ASSERT(c->c_prev == NULL);
-        PR_ASSERT(c->c_extension == NULL);
-
-        if (c->c_state == CONN_STATE_FREE) {
-
-            c->c_state = CONN_STATE_INIT;
-
-            pthread_mutexattr_t monitor_attr = {0};
-            pthread_mutexattr_init(&monitor_attr);
-            pthread_mutexattr_settype(&monitor_attr, PTHREAD_MUTEX_RECURSIVE);
-            if (pthread_mutex_init(&(c->c_mutex), &monitor_attr) != 0) {
-                slapi_log_err(SLAPI_LOG_ERR, "connection_table_get_connection", "pthread_mutex_init failed\n");
-                exit(1);
-            }
-
-            c->c_pdumutex = PR_NewLock();
-            if (c->c_pdumutex == NULL) {
-                c->c_pdumutex = NULL;
-                slapi_log_err(SLAPI_LOG_ERR, "connection_table_get_connection", "PR_NewLock failed\n");
-                exit(1);
-            }
-        }
-        /* Let's make sure there's no cruft left on there from the last time this connection was used. */
-        /* Note: no need to lock c->c_mutex because this function is only
-         * called by one thread (the slapd_daemon thread), and if we got this
-         * far then `c' is not being used by any operation threads, etc.
-         */
-        connection_cleanup(c);
-        c->c_ct = ct; /* pointer to connection table that owns this connection */
+        PR_Unlock(ct->table_mutex);
     } else {
-        /* couldn't find a Connection */
+        /* couldn't find a Connection, table must be full */
         slapi_log_err(SLAPI_LOG_CONNS, "connection_table_get_connection", "Max open connections reached\n");
+        PR_Unlock(ct->table_mutex);
+        return NULL;
     }
 
-    /* We could move this to before the c alloc as there is no point to remain here. */
-    PR_Unlock(ct->table_mutex);
+    /* Now prep the slot for usage. */
+    PR_ASSERT(c != NULL);
+    PR_ASSERT(c->c_next == NULL);
+    PR_ASSERT(c->c_prev == NULL);
+    PR_ASSERT(c->c_extension == NULL);
+    PR_ASSERT(c->c_state == CONN_STATE_INIT);
+    /* Let's make sure there's no cruft left on there from the last time this connection was used. */
+
+    /*
+     * Note: no need to lock c->c_mutex because this function is only
+     * called by one thread (the slapd_daemon thread), and if we got this
+     * far then `c' is not being used by any operation threads, etc. The
+     * memory ordering will be provided by the work queue sending c to a
+     * thread.
+     */
+    connection_cleanup(c);
+    /* pointer to connection table that owns this connection */
+    c->c_ct = ct;
 
     return c;
 }
-- 
2.26.2