From 9fb9eafde99f16ce424af6d53100412f99254f98 Mon Sep 17 00:00:00 2001 From: "Thierry bordaz (tbordaz)" Date: Thu, 14 Mar 2013 18:35:21 +0100 Subject: [PATCH] Ticket 616 - High contention on computed attribute lock Bug Description: On search request, some requested attributes are computed: - subschemasubentry - nscpentrywsi - numsubordinates - hassubordinates This is done with a linked list of registered callback. When sending the results, each requested attribute is evaluated against all the callbacks. When going through the callbacks list the thread hold the lock on the list. Currently all the server callbacks are registered at startup time with only one thread registering them (main). A custom plugin may register custom callback using slapi_compute_add_evaluator The important point is that callback are only added to the list. Also on standard deployment without custom plugin adding new computed attribute, this list is built at startup. The problem on computed attribute is that it can create contention between search threads trying to evaluate/compute the requested attributes The same description applies to computed filters. For example with 'views' or filters containing "subordinates", the filter is recomputed. This using a linked list of callbacks registered during startup. A custom plugin may register custom callback using slapi_compute_add_search_rewriter. In short: On standard deployment computed filters and computed attributes lists are unchanged after startup. Computed filters/attribute are only added to the lists. Fix Description: The fix consist to not protect the access to computed filter/attribute unless a computed filter/attribute is added after startup. The fix also contains a improvement of rsearch to support filters larger than 256 char The benefit is limited to 2% in terms of search throughput (8*4core machine) https://fedorahosted.org/389/ticket/616 Reviewed by: ? Platforms tested: 1*4cores fedora and 8*4cores RHEL6.4 Flag Day: no Doc impact: no --- ldap/servers/slapd/computed.c | 134 +++++++++++++++++++----- ldap/servers/slapd/main.c | 3 +- ldap/servers/slapd/proto-slap.h | 1 + ldap/servers/slapd/tools/rsearch/nametable.c | 4 +- ldap/servers/slapd/tools/rsearch/searchthread.c | 4 +- 5 files changed, 112 insertions(+), 34 deletions(-) diff --git a/ldap/servers/slapd/computed.c b/ldap/servers/slapd/computed.c index 353d375..8628033 100644 --- a/ldap/servers/slapd/computed.c +++ b/ldap/servers/slapd/computed.c @@ -44,6 +44,7 @@ /* Handles computed attributes for entries as they're returned to the client */ #include "slap.h" +#include "proto-slap.h" /* Structure used to pass the context needed for completing a computed attribute operation */ @@ -61,8 +62,11 @@ struct _compute_evaluator { }; typedef struct _compute_evaluator compute_evaluator; +static PRBool startup_completed = PR_FALSE; + static compute_evaluator *compute_evaluators = NULL; static Slapi_RWLock *compute_evaluators_lock = NULL; +static PRBool require_compute_evaluator_lock = PR_FALSE; static int compute_stock_evaluator(computed_attr_context *c,char* type,Slapi_Entry *e,slapi_compute_output_t outputfn); @@ -75,6 +79,7 @@ typedef struct _compute_rewriter compute_rewriter; static compute_rewriter *compute_rewriters = NULL; static Slapi_RWLock *compute_rewriters_lock = NULL; +static PRBool require_compute_rewriters_lock = PR_FALSE; /* Function called by evaluators to have the value output */ static int @@ -83,17 +88,32 @@ compute_output_callback(computed_attr_context *c,Slapi_Attr *a , Slapi_Entry *e) return encode_attr (c->pb, c->ber, e, a, c->attrsonly, c->requested_type); } +static +int compute_call_evaluators_nolock(computed_attr_context *c,slapi_compute_output_t outfn,char *type,Slapi_Entry *e) +{ + int rc = -1; + compute_evaluator *current = NULL; + + for (current = compute_evaluators; (current != NULL) && (-1 == rc); current = current->next) { + rc = (*(current->function))(c,type,e,outfn); + } + return rc; +} + static int compute_call_evaluators(computed_attr_context *c,slapi_compute_output_t outfn,char *type,Slapi_Entry *e) { int rc = -1; - compute_evaluator *current = NULL; /* Walk along the list (locked) calling the evaluator functions util one says yes, an error happens, or we finish */ - slapi_rwlock_rdlock(compute_evaluators_lock); - for (current = compute_evaluators; (current != NULL) && (-1 == rc); current = current->next) { - rc = (*(current->function))(c,type,e,outfn); - } - slapi_rwlock_unlock(compute_evaluators_lock); + + if (require_compute_evaluator_lock) { + slapi_rwlock_rdlock(compute_evaluators_lock); + rc = compute_call_evaluators_nolock(c, outfn, type, e); + slapi_rwlock_unlock(compute_evaluators_lock); + } else { + rc = compute_call_evaluators_nolock(c, outfn, type, e); + } + return rc; } @@ -132,25 +152,50 @@ compute_stock_evaluator(computed_attr_context *c,char* type,Slapi_Entry *e,slapi return rc; /* I see no ships */ } +static void +compute_add_elevator_nolock(slapi_compute_callback_t function, compute_evaluator *new_eval) +{ + new_eval->next = compute_evaluators; + new_eval->function = function; + compute_evaluators = new_eval; +} int slapi_compute_add_evaluator(slapi_compute_callback_t function) { int rc = 0; compute_evaluator *new_eval = NULL; PR_ASSERT(NULL != function); PR_ASSERT(NULL != compute_evaluators_lock); - slapi_rwlock_wrlock(compute_evaluators_lock); + if (startup_completed) { + /* We are now in multi-threaded and we still add + * a attribute elevator. + * switch to use locking mechanimsm + */ + require_compute_evaluator_lock = PR_TRUE; + } + new_eval = (compute_evaluator *)slapi_ch_calloc(1,sizeof (compute_evaluator)); if (NULL == new_eval) { rc = ENOMEM; } else { - new_eval->next = compute_evaluators; - new_eval->function = function; - compute_evaluators = new_eval; + if (require_compute_evaluator_lock) { + slapi_rwlock_wrlock(compute_evaluators_lock); + compute_add_elevator_nolock(function, new_eval); + slapi_rwlock_unlock(compute_evaluators_lock); + } else { + compute_add_elevator_nolock(function, new_eval); + } } - slapi_rwlock_unlock(compute_evaluators_lock); + return rc; } +/* Called when */ +void +compute_plugins_started() +{ + startup_completed = PR_TRUE; +} + /* Call this on server startup, before the first LDAP operation is serviced */ int compute_init() { @@ -200,6 +245,14 @@ int compute_terminate() return 0; } +static void +compute_add_search_rewrite_nolock(slapi_search_rewrite_callback_t function, compute_rewriter *new_rewriter) +{ + new_rewriter->next = compute_rewriters; + new_rewriter->function = function; + compute_rewriters = new_rewriter; +} + /* Functions dealing with re-writing of search filters */ int slapi_compute_add_search_rewriter(slapi_search_rewrite_callback_t function) @@ -208,36 +261,59 @@ int slapi_compute_add_search_rewriter(slapi_search_rewrite_callback_t function) compute_rewriter *new_rewriter = NULL; PR_ASSERT(NULL != function); PR_ASSERT(NULL != compute_rewriters_lock); + if (startup_completed) { + /* We are now in multi-threaded and we still add + * a filter rewriter. + * switch to use locking mechanimsm + */ + require_compute_rewriters_lock = PR_TRUE; + } new_rewriter = (compute_rewriter *)slapi_ch_calloc(1,sizeof (compute_rewriter)); if (NULL == new_rewriter) { rc = ENOMEM; } else { - slapi_rwlock_wrlock(compute_rewriters_lock); - new_rewriter->next = compute_rewriters; - new_rewriter->function = function; - compute_rewriters = new_rewriter; - slapi_rwlock_unlock(compute_rewriters_lock); + if (require_compute_rewriters_lock) { + slapi_rwlock_wrlock(compute_rewriters_lock); + compute_add_search_rewrite_nolock(function, new_rewriter); + slapi_rwlock_unlock(compute_rewriters_lock); + } else { + compute_add_search_rewrite_nolock(function, new_rewriter); + } } return rc; } -int compute_rewrite_search_filter(Slapi_PBlock *pb) +static +int compute_rewrite_search_filter_nolock(Slapi_PBlock *pb) +{ + int rc = -1; + compute_rewriter *current = NULL; + + for (current = compute_rewriters; (current != NULL) && (-1 == rc); current = current->next) { + rc = (*(current->function))(pb); + /* Meaning of the return code : + -1 : keep looking + 0 : rewrote OK + 1 : refuse to do this search + 2 : operations error + */ + } + return(rc); +} + +int compute_rewrite_search_filter(Slapi_PBlock *pb) { /* Iterate through the listed rewriters until one says it matched */ int rc = -1; - compute_rewriter *current = NULL; + /* Walk along the list (locked) calling the evaluator functions util one says yes, an error happens, or we finish */ - slapi_rwlock_rdlock(compute_rewriters_lock); - for (current = compute_rewriters; (current != NULL) && (-1 == rc); current = current->next) { - rc = (*(current->function))(pb); - /* Meaning of the return code : - -1 : keep looking - 0 : rewrote OK - 1 : refuse to do this search - 2 : operations error - */ - } - slapi_rwlock_unlock(compute_rewriters_lock); + if (require_compute_rewriters_lock) { + slapi_rwlock_rdlock(compute_rewriters_lock); + rc = compute_rewrite_search_filter_nolock(pb); + slapi_rwlock_unlock(compute_rewriters_lock); + } else { + rc = compute_rewrite_search_filter_nolock(pb); + } return rc; } diff --git a/ldap/servers/slapd/main.c b/ldap/servers/slapd/main.c index f438fce..e97e5ef 100644 --- a/ldap/servers/slapd/main.c +++ b/ldap/servers/slapd/main.c @@ -1178,7 +1178,8 @@ main( int argc, char **argv) pw_exp_init (); plugin_print_lists(); - plugin_startall(argc, argv, 1 /* Start Backends */, 1 /* Start Globals */); + plugin_startall(argc, argv, 1 /* Start Backends */, 1 /* Start Globals */); + compute_plugins_started(); if (housekeeping_start((time_t)0, NULL) == NULL) { return_value = 1; goto cleanup; diff --git a/ldap/servers/slapd/proto-slap.h b/ldap/servers/slapd/proto-slap.h index 0a07a29..b13c494 100644 --- a/ldap/servers/slapd/proto-slap.h +++ b/ldap/servers/slapd/proto-slap.h @@ -250,6 +250,7 @@ void do_compare( Slapi_PBlock *pb ); int compute_attribute(char *type, Slapi_PBlock *pb,BerElement *ber,Slapi_Entry *e,int attrsonly,char *requested_type); int compute_init(); int compute_terminate(); +void compute_plugins_started(); /* diff --git a/ldap/servers/slapd/tools/rsearch/nametable.c b/ldap/servers/slapd/tools/rsearch/nametable.c index 0a4b6fc..9215fd5 100644 --- a/ldap/servers/slapd/tools/rsearch/nametable.c +++ b/ldap/servers/slapd/tools/rsearch/nametable.c @@ -152,8 +152,8 @@ int nt_load(NameTable *nt, const char *filename) if (!fd) return 0; while (PR_Available(fd) > 0) { - char temp[256], *s; - if (PR_GetLine(fd, temp, 256)) break; + char temp[10000], *s; + if (PR_GetLine(fd, temp, 10000)) break; s = strdup(temp); if (!s) break; if (!nt_push(nt, s)) break; diff --git a/ldap/servers/slapd/tools/rsearch/searchthread.c b/ldap/servers/slapd/tools/rsearch/searchthread.c index 8a74d58..e44ece6 100644 --- a/ldap/servers/slapd/tools/rsearch/searchthread.c +++ b/ldap/servers/slapd/tools/rsearch/searchthread.c @@ -285,7 +285,7 @@ static int st_bind(SearchThread *st) return 0; } } else if (uid) { - char filterBuffer[100]; + char filterBuffer[10000]; char *pFilter; char *filterTemplate = (uidFilter) ? uidFilter : "(uid=%s)"; struct timeval timeout; @@ -366,7 +366,7 @@ static void st_unbind(SearchThread *st) static int st_search(SearchThread *st) { - char filterBuffer[100]; + char filterBuffer[10000]; char *pFilter; struct timeval timeout; struct timeval *timeoutp; -- 1.7.11.7