This is an automated email from the git hooks/post-receive script.
tbordaz pushed a commit to branch 389-ds-base-1.4.0
in repository 389-ds-base.
The following commit(s) were added to refs/heads/389-ds-base-1.4.0 by this push:
new 6f87fab Ticket 49873 - Contention on virtual attribute lookup
6f87fab is described below
commit 6f87fab42db4bf084c4d9fa32897e7175ba1ef82
Author: Thierry Bordaz <tbordaz(a)redhat.com>
AuthorDate: Tue Jan 15 11:13:42 2019 +0100
Ticket 49873 - Contention on virtual attribute lookup
Bug Description:
During lookup of the virtual attribute table (filter evaluation and returned
attribute)
the lock is acquired many times in read. For example it is acquired for each
targetfilter aci and for
each evaluated entry.
Unfortunately RW lock is expensive and appears frequently on pstacks.
The lock exists because the table can be updated but update is very rare (addition of
a new service provider).
So it slows down general proceeding for exceptional events.
Fix Description:
The fix is to acquire/release the read lock at the operation level and set a per-cpu
flag, so that later lookup
would just check the flag.
https://pagure.io/389-ds-base/issue/49873
Reviewed by: Ludwig Krispenz, William Brown (thanks !!)
Platforms tested: F27
Flag Day: no
Doc impact: no
---
ldap/servers/slapd/connection.c | 1 +
ldap/servers/slapd/opshared.c | 6 +++
ldap/servers/slapd/proto-slap.h | 3 ++
ldap/servers/slapd/psearch.c | 1 +
ldap/servers/slapd/vattr.c | 82 ++++++++++++++++++++++++++++++++++-------
5 files changed, 80 insertions(+), 13 deletions(-)
diff --git a/ldap/servers/slapd/connection.c b/ldap/servers/slapd/connection.c
index 8b88568..fcc46cd 100644
--- a/ldap/servers/slapd/connection.c
+++ b/ldap/servers/slapd/connection.c
@@ -1509,6 +1509,7 @@ connection_threadmain()
long bypasspollcnt = 0;
enable_nunc_stans = config_get_enable_nunc_stans();
+ vattr_global_lock_init();
#if defined(hpux)
/* Arrange to ignore SIGPIPE signals. */
SIGNAL(SIGPIPE, SIG_IGN);
diff --git a/ldap/servers/slapd/opshared.c b/ldap/servers/slapd/opshared.c
index e791a90..8b895a1 100644
--- a/ldap/servers/slapd/opshared.c
+++ b/ldap/servers/slapd/opshared.c
@@ -243,6 +243,7 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
int pr_idx = -1;
Slapi_DN *orig_sdn = NULL;
int free_sdn = 0;
+ PRBool vattr_lock_acquired = PR_FALSE;
be_list[0] = NULL;
referral_list[0] = NULL;
@@ -528,6 +529,8 @@ op_shared_search(Slapi_PBlock *pb, int send_result)
}
slapi_pblock_set(pb, SLAPI_BACKEND_COUNT, &index);
+ vattr_rdlock();
+ vattr_lock_acquired = PR_TRUE;
if (be) {
slapi_pblock_set(pb, SLAPI_BACKEND, be);
@@ -983,6 +986,9 @@ free_and_return:
} else if (be_single) {
slapi_be_Unlock(be_single);
}
+ if (vattr_lock_acquired) {
+ vattr_unlock();
+ }
free_and_return_nolock:
slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, &rc);
diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h
index ca946fb..2029f41 100644
--- a/ldap/servers/slapd/proto-slap.h
+++ b/ldap/servers/slapd/proto-slap.h
@@ -1419,6 +1419,9 @@ void subentry_create_filter(Slapi_Filter **filter);
* vattr.c
*/
void vattr_init(void);
+void vattr_global_lock_init(void);
+void vattr_rdlock();
+void vattr_unlock();
void vattr_cleanup(void);
/*
diff --git a/ldap/servers/slapd/psearch.c b/ldap/servers/slapd/psearch.c
index 8ad268a..e7b97a7 100644
--- a/ldap/servers/slapd/psearch.c
+++ b/ldap/servers/slapd/psearch.c
@@ -267,6 +267,7 @@ ps_send_results(void *arg)
Operation *pb_op = NULL;
g_incr_active_threadcnt();
+ vattr_global_lock_init();
slapi_pblock_get(ps->ps_pblock, SLAPI_CONNECTION, &pb_conn);
slapi_pblock_get(ps->ps_pblock, SLAPI_OPERATION, &pb_op);
diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c
index f7c473a..155afca 100644
--- a/ldap/servers/slapd/vattr.c
+++ b/ldap/servers/slapd/vattr.c
@@ -102,6 +102,16 @@ int vattr_basic_sp_init();
void **statechange_api;
+struct _vattr_map
+{
+ Slapi_RWLock *lock;
+ PLHashTable *hashtable; /* Hash table */
+};
+typedef struct _vattr_map vattr_map;
+
+static vattr_map *the_map = NULL;
+static PRUintn thread_private_global_vattr_lock;
+
/* Housekeeping Functions, called by server startup/shutdown code */
/* Called on server startup, init all structures etc */
@@ -109,6 +119,7 @@ void
vattr_init()
{
statechange_api = 0;
+ PR_NewThreadPrivateIndex(&thread_private_global_vattr_lock, NULL);
vattr_map_create();
#ifdef VATTR_TEST_CODE
@@ -116,6 +127,60 @@ vattr_init()
#endif
}
+void
+vattr_global_lock_init()
+{
+ if (thread_private_global_vattr_lock) {
+ PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) 0);
+ }
+}
+/* The map lock can be acquired recursively. So only the first rdlock
+ * will acquire the lock.
+ * A optimization acquires it at high level (op_shared_search), so that
+ * later calls during the operation processing will just increase/decrease a counter.
+ */
+void
+vattr_rdlock()
+{
+ if (thread_private_global_vattr_lock) {
+ int nb_acquire = (int) PR_GetThreadPrivate(thread_private_global_vattr_lock);
+
+ if (nb_acquire == 0) {
+ /* The lock was not held just acquire it */
+ slapi_rwlock_rdlock(the_map->lock);
+ }
+ nb_acquire++;
+ PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) nb_acquire);
+ } else {
+ slapi_rwlock_rdlock(the_map->lock);
+ }
+}
+/* The map lock can be acquired recursively. So only the last unlock
+ * will release the lock.
+ * A optimization acquires it at high level (op_shared_search), so that
+ * later calls during the operation processing will just increase/decrease a counter.
+ */
+void
+vattr_unlock()
+{
+ if (thread_private_global_vattr_lock) {
+ int nb_acquire = (int) PR_GetThreadPrivate(thread_private_global_vattr_lock);
+
+ if (nb_acquire >= 1) {
+ nb_acquire--;
+ if (nb_acquire == 0) {
+ slapi_rwlock_unlock(the_map->lock);
+ }
+ PR_SetThreadPrivate(thread_private_global_vattr_lock, (void *) nb_acquire);
+ } else {
+ slapi_log_err(SLAPI_LOG_CRIT,
+ "vattr_unlock", "The lock was not acquire. We should not be
here\n");
+ PR_ASSERT(nb_acquire >= 1);
+ }
+ } else {
+ slapi_rwlock_unlock(the_map->lock);
+ }
+}
/* Called on server shutdown, free all structures, inform service providers that
we're going down etc */
void
vattr_cleanup()
@@ -1811,15 +1876,6 @@ typedef struct _vattr_map_entry vattr_map_entry;
vattr_map_entry test_entry = {NULL};
-struct _vattr_map
-{
- Slapi_RWLock *lock;
- PLHashTable *hashtable; /* Hash table */
-};
-typedef struct _vattr_map vattr_map;
-
-static vattr_map *the_map = NULL;
-
static PRIntn
vattr_hash_compare_keys(const void *v1, const void *v2)
{
@@ -1939,11 +1995,11 @@ vattr_map_lookup(const char *type_to_find, vattr_map_entry
**result)
}
/* Get the reader lock */
- slapi_rwlock_rdlock(the_map->lock);
+ vattr_rdlock();
*result = (vattr_map_entry *)PL_HashTableLookupConst(the_map->hashtable,
(void *)basetype);
/* Release ze lock */
- slapi_rwlock_unlock(the_map->lock);
+ vattr_unlock();
if (tmp) {
slapi_ch_free_string(&tmp);
@@ -2131,7 +2187,7 @@ slapi_vattr_schema_check_type(Slapi_Entry *e, char *type)
objAttrValue *obj;
if (0 == vattr_map_lookup(type, &map_entry)) {
- slapi_rwlock_rdlock(the_map->lock);
+ vattr_rdlock();
obj = map_entry->objectclasses;
@@ -2148,7 +2204,7 @@ slapi_vattr_schema_check_type(Slapi_Entry *e, char *type)
obj = obj->pNext;
}
- slapi_rwlock_unlock(the_map->lock);
+ vattr_unlock();
}
slapi_valueset_free(vs);
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.