From d1236b0d3917b024a252b4d6acf823a975182d3b Mon Sep 17 00:00:00 2001 From: William Brown Date: Mon, 11 Dec 2017 15:48:24 +0100 Subject: [PATCH] Ticket 49495 - Fix memory management is vattr. Bug Description: During the fix for https://pagure.io/389-ds-base/issue/49436 a issue was exposed in how registration of attributes to cos work. With the change to handle -> attr link, this exposed that cos treats each attribute and template pair as a new type for the handle. As aresult, this caused the sp_list to create a long linked list of M*N entries for each attr - template value. Obviously, this is extremely slow to traverse during a search! Fix Description: Undo part of the SLL next change and convert to reference counting. The issue remains that there is a defect in how cos handles attribute registration, but this can not be resolved without a significant rearchitecture of the code related to virtual attributes. https://pagure.io/389-ds-base/issue/49495 Author: wibrown Review by: tbordaz, lkrispen (Thanks!) Note: includes backport of incr and decr for rc --- ldap/servers/plugins/cos/cos_cache.c | 28 ++++------ ldap/servers/slapd/slapi-plugin.h | 21 ++++++++ ldap/servers/slapd/slapi_counter.c | 30 +++++++++++ ldap/servers/slapd/vattr.c | 100 +++++++++++++++++++++-------------- 4 files changed, 122 insertions(+), 57 deletions(-) diff --git a/ldap/servers/plugins/cos/cos_cache.c b/ldap/servers/plugins/cos/cos_cache.c index 0e93183d2..74261af87 100644 --- a/ldap/servers/plugins/cos/cos_cache.c +++ b/ldap/servers/plugins/cos/cos_cache.c @@ -275,7 +275,7 @@ static Slapi_Mutex *start_lock; static Slapi_Mutex *stop_lock; static Slapi_CondVar *something_changed = NULL; static Slapi_CondVar *start_cond = NULL; - +static vattr_sp_handle *vattr_handle = NULL; /* cos_cache_init @@ -313,6 +313,15 @@ int cos_cache_init(void) goto out; } + if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle, + cos_cache_vattr_get, + cos_cache_vattr_compare, + cos_cache_vattr_types) != 0) { + slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider\n"); + ret = -1; + goto out; + } + /* grab the views interface */ if (slapi_apib_get_interface(Views_v1_0_GUID, &views_api)) { /* lets be tolerant if views is disabled */ @@ -872,22 +881,7 @@ cos_dn_defs_cb (Slapi_Entry* e, void *callback_data) dnVals[valIndex]->bv_val); } - /* - * Each SP_handle is associated to one and only one vattr. - * We could consider making this a single function rather - * than the double-call. - */ - - vattr_sp_handle *vattr_handle = NULL; - - if (slapi_vattrspi_register((vattr_sp_handle **)&vattr_handle, - cos_cache_vattr_get, - cos_cache_vattr_compare, - cos_cache_vattr_types) != 0) { - slapi_log_err(SLAPI_LOG_ERR, COS_PLUGIN_SUBSYSTEM, "cos_cache_init - Cannot register as service provider for %s\n", dnVals[valIndex]->bv_val); - } else { - slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL); - } + slapi_vattrspi_regattr((vattr_sp_handle *)vattr_handle, dnVals[valIndex]->bv_val, NULL, NULL); } /* if(attrType is cosAttribute) */ diff --git a/ldap/servers/slapd/slapi-plugin.h b/ldap/servers/slapd/slapi-plugin.h index 4084945f4..16aa1b711 100644 --- a/ldap/servers/slapd/slapi-plugin.h +++ b/ldap/servers/slapd/slapi-plugin.h @@ -8063,6 +8063,27 @@ int slapi_is_special_rdn(const char *rdn, int flag); */ void DS_Sleep(PRIntervalTime ticks); +/** + * Increment a 64bitintegral atomicly + * + * \param ptr - pointer to integral to increment + * \param memorder - __ATOMIC_RELAXED, __ATOMIC_CONSUME, __ATOMIC_ACQUIRE, + * __ATOMIC_RELEASE, __ATOMIC_ACQ_REL, __ATOMIC_SEQ_CST + * \return - new value of ptr + */ +uint64_t slapi_atomic_incr_64(uint64_t *ptr, int memorder); + +/** + * Decrement a 64bitintegral atomicly + * + * \param ptr - pointer to integral to decrement + * \param memorder - __ATOMIC_RELAXED, __ATOMIC_CONSUME, __ATOMIC_ACQUIRE, + * __ATOMIC_RELEASE, __ATOMIC_ACQ_REL, __ATOMIC_SEQ_CST + * \return - new value of ptr + */ +uint64_t slapi_atomic_decr_64(uint64_t *ptr, int memorder); + + #ifdef __cplusplus } #endif diff --git a/ldap/servers/slapd/slapi_counter.c b/ldap/servers/slapd/slapi_counter.c index 9904fe964..59e5223ad 100644 --- a/ldap/servers/slapd/slapi_counter.c +++ b/ldap/servers/slapd/slapi_counter.c @@ -269,3 +269,33 @@ uint64_t slapi_counter_get_value(Slapi_Counter *counter) return value; } +/* + * atomic increment functions (64bit) + */ +uint64_t +slapi_atomic_incr_64(uint64_t *ptr, int memorder) +{ +#ifdef ATOMIC_64BIT_OPERATIONS + return __atomic_add_fetch_8(ptr, 1, memorder); +#else + PRInt32 *pr_ptr = (PRInt32 *)ptr; + return PR_AtomicIncrement(pr_ptr); +#endif +} + +/* + * atomic decrement functions (64bit) + */ + +uint64_t +slapi_atomic_decr_64(uint64_t *ptr, int memorder) +{ +#ifdef ATOMIC_64BIT_OPERATIONS + return __atomic_sub_fetch_8(ptr, 1, memorder); +#else + PRInt32 *pr_ptr = (PRInt32 *)ptr; + return PR_AtomicDecrement(pr_ptr); +#endif +} + + diff --git a/ldap/servers/slapd/vattr.c b/ldap/servers/slapd/vattr.c index 84e01cd62..adf44b0b6 100644 --- a/ldap/servers/slapd/vattr.c +++ b/ldap/servers/slapd/vattr.c @@ -1529,10 +1529,12 @@ struct _vattr_sp { typedef struct _vattr_sp vattr_sp; /* Service provider handle */ -struct _vattr_sp_handle { - vattr_sp *sp; - struct _vattr_sp_handle *next; /* So we can link them together in the map */ - void *hint; /* Hint to the SP */ +struct _vattr_sp_handle +{ + vattr_sp *sp; + struct _vattr_sp_handle *next; /* So we can link them together in the map */ + void *hint; /* Hint to the SP */ + uint64_t rc; }; /* Calls made by Service Providers */ @@ -1758,7 +1760,7 @@ is a separate thing in the insterests of stability. */ -#define VARRT_MAP_HASHTABLE_SIZE 10 +#define VARRT_MAP_HASHTABLE_SIZE 32 /* Attribute map oject */ /* Needs to contain: a linked list of pointers to provider handles handles, @@ -1849,7 +1851,10 @@ vattr_map_entry_free(vattr_map_entry *vae) vattr_sp_handle *list_entry = vae->sp_list; while (list_entry != NULL) { vattr_sp_handle *next_entry = list_entry->next; - slapi_ch_free((void **)&list_entry); + if (slapi_atomic_decr_64(&(list_entry->rc), __ATOMIC_RELAXED) == 0) { + /* Only free on RC 0 */ + slapi_ch_free((void **)&list_entry); + } list_entry = next_entry; } slapi_ch_free_string(&(vae->type_name)); @@ -2268,41 +2273,56 @@ to handle the calls on it, but return nothing */ * * Better idea, is that regattr should just take the fn pointers * and callers never *see* the sp_handle structure at all. + * + * This leaves us with some quirks today. First: if you have plugin A + * and B, A registers attr 1 and B 1 and 2, it's possible that if you + * register A1 first, then B1, you have B->A in next. Then when you + * register B2, because we take 0==result from map_lookup, we add sp + * "as is" to the map. This means that B2 now has the same next to A1 + * handle. This won't add a bug, because A1 won't be able to service the + * attr, but it could cause some head scratching ... + * + * Again, to fix this, the whole vattr external interface needs a + * redesign ... :( */ - -int vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint) -{ - int result = 0; - vattr_map_entry *map_entry = NULL; - /* Is this type already there ? */ - result = vattr_map_lookup(type_to_add,&map_entry); - /* If it is, add this SP to the list, safely even if readers are traversing the list at the same time */ - if (0 == result) { - int found = 0; - vattr_sp_handle *list_entry = NULL; - /* Walk the list checking that the daft SP isn't already here */ - for (list_entry = map_entry->sp_list ; list_entry; list_entry = list_entry->next) { - if (list_entry == sp) { - found = 1; - break; - } - } - /* If it is, we do nothing */ - if(found) { - return 0; - } - /* We insert the SP handle into the linked list at the head */ - sp->next = map_entry->sp_list; - map_entry->sp_list = sp; - } else { - /* If not, add it */ - map_entry = vattr_map_entry_new(type_to_add,sp,hint); - if (NULL == map_entry) { - return ENOMEM; - } - return vattr_map_insert(map_entry); - } - return 0; +int +vattr_map_sp_insert(char *type_to_add, vattr_sp_handle *sp, void *hint) +{ + int result = 0; + vattr_map_entry *map_entry = NULL; + /* Is this type already there ? */ + result = vattr_map_lookup(type_to_add, &map_entry); + /* If it is, add this SP to the list, safely even if readers are traversing the list at the same time */ + if (0 == result) { + int found = 0; + vattr_sp_handle *list_entry = NULL; + /* Walk the list checking that the daft SP isn't already here */ + for (list_entry = map_entry->sp_list; list_entry; list_entry = list_entry->next) { + if (list_entry == sp) { + found = 1; + break; + } + } + /* If it is, we do nothing */ + if (found) { + return 0; + } + /* Increase the ref count of the sphandle */ + slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED); + /* We insert the SP handle into the linked list at the head */ + sp->next = map_entry->sp_list; + map_entry->sp_list = sp; + } else { + /* If not, add it */ + /* Claim a reference on the sp ... */ + slapi_atomic_incr_64(&(sp->rc), __ATOMIC_RELAXED); + map_entry = vattr_map_entry_new(type_to_add, sp, hint); + if (NULL == map_entry) { + return ENOMEM; + } + return vattr_map_insert(map_entry); + } + return 0; } /* -- 2.13.6