From 03fdcbc4ecc902decc647487ce84141ca561c49e Mon Sep 17 00:00:00 2001 From: William Brown Date: Tue, 14 Nov 2017 16:40:32 +1000 Subject: [PATCH 2/9] Ticket 49218 - Certmap - sds changes for set manipulation Bug Description: This adds support for pluggable certificate mapping libraries. To achieve this, this replaces the existing baked in certificate mapping code. Fix Description: Improve the performance of sds, as well as improving some behaviours of the set functions for the b+tree related to iteration. This is required for the plugin interface for efficent calls of plugins. https://pagure.io/389-ds-base/issue/49218 https://pagure.io/lib389/issue/95 https://pagure.io/lib389/issue/84 Author: wibrown Review by: ??? --- src/libsds/include/sds.h | 24 ++++++++++++++++++--- src/libsds/sds/bpt/bpt.c | 3 +++ src/libsds/sds/bpt/common.c | 2 +- src/libsds/sds/bpt/set.c | 4 ++-- src/libsds/sds/bpt_cow/bpt_cow.c | 45 ++++++++++++++++++++++++++++++++++++++++ src/libsds/sds/bpt_cow/txn.c | 27 +++++++++++++++++++----- src/libsds/test/test_sds_cow.c | 37 +++++++++++++++++++++++++++++++++ src/libsds/test/test_sds_set.c | 4 ++-- 8 files changed, 133 insertions(+), 13 deletions(-) diff --git a/src/libsds/include/sds.h b/src/libsds/include/sds.h index c649c03..c92b3b8 100644 --- a/src/libsds/include/sds.h +++ b/src/libsds/include/sds.h @@ -1079,7 +1079,7 @@ sds_result sds_bptree_verify(sds_bptree_instance *binst); * \param fn The function to be applied to each key-value pair. * \retval Result of the operation as sds_result. */ -sds_result sds_bptree_map(sds_bptree_instance *binst, void (*fn)(void *k, void *v)); +sds_result sds_bptree_map(sds_bptree_instance *binst, void *arg, void (*fn)(void *k, void *v, void *arg)); /** * From instance a, and instance b, create a new insance that contains the * keys and values where keys exist in a or b but not both. @@ -1227,7 +1227,16 @@ sds_result sds_bptree_cow_wrtxn_abort(sds_bptree_transaction **btxn); * \retval Result of the operation as sds_result. */ sds_result sds_bptree_cow_wrtxn_commit(sds_bptree_transaction **btxn); - +/** + * Retrieve the current transaction's id. There is *no guarantee* that this + * id increments sequentially, or avoids any kind of int rollover. It's only + * purpose is to allow generational timestamping of external data to assert + * if a newer transaction has passed since. + * + * \param btxn The transaction from which you wish to retrieve the ID + * \retval The transaction ID. + */ +uint64_t sds_bptree_txn_get_id(sds_bptree_transaction *btxn); /** * Search a tree with a valid transaction reference. This returns KEY_PRESENT * or KEY_NOT_PRESENT if the search suceeds or not. Search may operation on a valid @@ -1288,10 +1297,19 @@ sds_result sds_bptree_cow_insert(sds_bptree_transaction *btxn, void *key, void * * \param value The value to update. May be NULL. * \retval Result of the operation as sds_result. */ - sds_result sds_bptree_cow_update(sds_bptree_transaction *btxn, void *key, void *value); /** + * Map over all key/values in the tree within this txn. If the transaction is readonly + * you should only read these values. If the txn is write, you may modify value. + * + * \param btxn The transaction to map over. + * \param arg A generic argument that will be passed to each invocation of fn. + * \param fn the function to apply to each key/value. + */ +sds_result sds_bptree_cow_map(sds_bptree_transaction *btxn, void *arg, void (*fn)(void *k, void *v, void *arg)); + +/** * Search atomic functions as search, but implies a single short lived read transaction. * * If you have multiple searches to make, it is better to use a read transaction due to diff --git a/src/libsds/sds/bpt/bpt.c b/src/libsds/sds/bpt/bpt.c index 8703e2d..79951e0 100644 --- a/src/libsds/sds/bpt/bpt.c +++ b/src/libsds/sds/bpt/bpt.c @@ -384,6 +384,9 @@ sds_bptree_destroy(sds_bptree_instance *binst) { // Remove all the other elements sds_result result = SDS_SUCCESS; + if (binst == NULL) { + return SDS_NULL_POINTER; + } result = sds_bptree_map_nodes(binst, binst->root, sds_bptree_node_destroy); // Finally remove the binst sds_free(binst); diff --git a/src/libsds/sds/bpt/common.c b/src/libsds/sds/bpt/common.c index edae1e3..275ee10 100644 --- a/src/libsds/sds/bpt/common.c +++ b/src/libsds/sds/bpt/common.c @@ -522,7 +522,7 @@ sds_bptree_leaf_delete(sds_bptree_instance *binst, sds_bptree_node *node, void * /* extract the contents (if any) */ void *value = node->values[index]; - if (value != NULL) { + if (value != NULL && binst->value_free_fn != NULL) { binst->value_free_fn(value); } /* Delete the key + value */ diff --git a/src/libsds/sds/bpt/set.c b/src/libsds/sds/bpt/set.c index 9f00d35..523c56c 100644 --- a/src/libsds/sds/bpt/set.c +++ b/src/libsds/sds/bpt/set.c @@ -66,7 +66,7 @@ sds_bptree_list_advance(sds_bptree_node **item, size_t *index) /* Tree mapping functions */ /* Shouldn't this make a set of results? */ sds_result -sds_bptree_map(sds_bptree_instance *binst, void (*fn)(void *k, void *v)) +sds_bptree_map(sds_bptree_instance *binst, void *arg, void (*fn)(void *k, void *v, void *arg)) { /* If this is the non-cow tree, this is easy. */ /* Find the bottom left node, then iterate to the right! */ @@ -74,7 +74,7 @@ sds_bptree_map(sds_bptree_instance *binst, void (*fn)(void *k, void *v)) while (work_node != NULL) { for (size_t index = 0; index < work_node->item_count; index++) { - fn(work_node->keys[index], work_node->values[index]); + fn(work_node->keys[index], work_node->values[index], arg); } work_node = (sds_bptree_node *)work_node->values[SDS_BPTREE_DEFAULT_CAPACITY]; } diff --git a/src/libsds/sds/bpt_cow/bpt_cow.c b/src/libsds/sds/bpt_cow/bpt_cow.c index 0b6f229..81d03a1 100644 --- a/src/libsds/sds/bpt_cow/bpt_cow.c +++ b/src/libsds/sds/bpt_cow/bpt_cow.c @@ -469,6 +469,51 @@ sds_bptree_cow_update(sds_bptree_transaction *btxn, void *key, void *value) return SDS_SUCCESS; } +sds_result +sds_bptree_cow_map(sds_bptree_transaction *btxn, void *arg, void (*fn)(void *k, void *v, void *arg)) { + /* + * For each leaf node, map fn to each value. + * + * This is really similar to map_nodes, but only applies fn at level == 0 + */ + + if (btxn == NULL) { + return SDS_INVALID_TXN; + } + + sds_bptree_node_list *cur = sds_malloc(sizeof(sds_bptree_node_list)); + sds_bptree_node_list *prev = cur; + sds_bptree_node_list *tail = cur; + + cur->node = btxn->root; + cur->next = NULL; + sds_result final_result = SDS_SUCCESS; + + while (cur != NULL) { + if (cur->node->level > 0) { + /* Has to be <= here as this is access values, not keys! */ + for (size_t i = 0; i <= cur->node->item_count; i++) { + /* Alloc a new element, and shuffle along .... */ + if (cur->node->values[i] != NULL) { + tail->next = sds_malloc(sizeof(sds_bptree_node_list)); + tail = tail->next; + tail->node = (sds_bptree_node *)cur->node->values[i]; + tail->next = NULL; + } + } + } else { + /* We have a leaf! map the fn over valid values. */ + for (size_t index = 0; index < cur->node->item_count; index++) { + fn(cur->node->keys[index], cur->node->values[index], arg); + } + } + prev = cur; + cur = cur->next; + free(prev); + } + return final_result; +} + // Does this need to work on a transaction perhaps to verify the tree is "sane"? sds_result sds_bptree_cow_verify(sds_bptree_cow_instance *binst) diff --git a/src/libsds/sds/bpt_cow/txn.c b/src/libsds/sds/bpt_cow/txn.c index 1da0ae4..c6be44e 100644 --- a/src/libsds/sds/bpt_cow/txn.c +++ b/src/libsds/sds/bpt_cow/txn.c @@ -54,7 +54,7 @@ sds_bptree_txn_create(sds_bptree_cow_instance *binst) // The initial ref count is 0, and we only up to 1 when we commit. // Atomically set this to 0. - __atomic_and_fetch(&(btxn->reference_count), 0, __ATOMIC_SEQ_CST); + __atomic_and_fetch(&(btxn->reference_count), 0, __ATOMIC_RELAXED); #ifdef SDS_DEBUG // Update our needed checksums @@ -76,6 +76,23 @@ sds_bptree_txn_create(sds_bptree_cow_instance *binst) * ============================================================== */ +uint64_t +sds_bptree_txn_get_id(sds_bptree_transaction *btxn) { + if (btxn != NULL) { + return btxn->txn_id; + } + return 0; +} + +/* ========================= WARNING ============================ + * UNLESS YOU HAVE READ: + * https://www.kernel.org/doc/Documentation/memory-barriers.txt + * and SERIOUSLY understand it, and how it works you *MUST* not + * edit this file. This section of the code relies on a deep + * understanding of locking and memory barriers. + * ============================================================== + */ + // Should be caled by txn decrement. static void sds_bptree_txn_free(sds_bptree_transaction *btxn) @@ -111,7 +128,7 @@ sds_bptree_txn_free(sds_bptree_transaction *btxn) static void sds_bptree_txn_increment(sds_bptree_transaction *btxn) { - __atomic_add_fetch(&(btxn->reference_count), 1, __ATOMIC_SEQ_CST); + __atomic_add_fetch(&(btxn->reference_count), 1, __ATOMIC_RELAXED); // PR_AtomicIncrement(&(btxn->reference_count)); #ifdef SDS_DEBUG @@ -140,7 +157,7 @@ sds_bptree_txn_decrement(sds_bptree_transaction *btxn) // Atomic dec the counter. // PR_AtomicDecrement returns the set value. - uint32_t result = __atomic_sub_fetch(&(btxn->reference_count), 1, __ATOMIC_SEQ_CST); + uint32_t result = __atomic_sub_fetch(&(btxn->reference_count), 1, __ATOMIC_RELAXED); /* WARNING: After this point, another thread MAY free btxn under us. * You MUST *not* deref btxn after this point. */ @@ -169,7 +186,7 @@ sds_bptree_txn_decrement(sds_bptree_transaction *btxn) // * there are more parents left, so we are > 0 // * there are still active holders left, so we are > 0 if (btxn != NULL) { - result = __atomic_sub_fetch(&(btxn->reference_count), 1, __ATOMIC_SEQ_CST); + result = __atomic_sub_fetch(&(btxn->reference_count), 1, __ATOMIC_RELAXED); } } } @@ -400,7 +417,7 @@ sds_bptree_cow_wrtxn_commit(sds_bptree_transaction **btxn) // Say we are alive and commited - 2 means "our former transaction owns us" // and "we are the active root". uint32_t default_ref_count = 2; - __atomic_store(&((*btxn)->reference_count), &default_ref_count, __ATOMIC_SEQ_CST); + __atomic_store(&((*btxn)->reference_count), &default_ref_count, __ATOMIC_RELAXED); // Set it. (*btxn)->binst->txn = *btxn; // Update our parent to reference us. diff --git a/src/libsds/test/test_sds_cow.c b/src/libsds/test/test_sds_cow.c index 9fd5ab0..0ece309 100644 --- a/src/libsds/test/test_sds_cow.c +++ b/src/libsds/test/test_sds_cow.c @@ -562,6 +562,40 @@ test_cow_update(void **state) assert_int_equal(sds_bptree_cow_rotxn_close(&ro_btxn_b), SDS_SUCCESS); } +static void +test_cow_map_cb(void *key __attribute((unused)), void *value, void *arg __attribute((unused))) { + *(uint64_t *)value = 12345; +} + +static void +test_cow_map(void **state) +{ + sds_bptree_cow_instance *binst = *state; + sds_bptree_transaction *wr_btxn = NULL; + sds_result result = SDS_SUCCESS; + + assert_int_equal(sds_bptree_cow_wrtxn_begin(binst, &wr_btxn), SDS_SUCCESS); + + /* Insert a set of keys with values */ + for (uint64_t i = 10; i < (10 + SDS_BPTREE_DEFAULT_CAPACITY); i++) { + result = sds_bptree_cow_insert(wr_btxn, (void *)&i, sds_uint64_t_dup((void *)&i)); + assert_int_equal(result, SDS_SUCCESS); + } + /* Map and change the values */ + + assert_int_equal(sds_bptree_cow_map(wr_btxn, NULL, test_cow_map_cb), SDS_SUCCESS); + + for (uint64_t i = 10; i < (10 + SDS_BPTREE_DEFAULT_CAPACITY); i++) { + uint64_t *output = NULL; + assert_int_equal(sds_bptree_cow_retrieve(wr_btxn, (void *)&i, (void **)&output), SDS_KEY_PRESENT); + assert_int_equal(result, SDS_SUCCESS); + assert_int_equal(*output, 12345); + } + /* Assert the values have changed. */ + + assert_int_equal(sds_bptree_cow_wrtxn_commit(&wr_btxn), SDS_SUCCESS); +} + int run_cow_tests(void) { @@ -627,6 +661,9 @@ run_cow_tests(void) cmocka_unit_test_setup_teardown(test_cow_update, bptree_test_cow_setup, bptree_test_cow_teardown), + cmocka_unit_test_setup_teardown(test_cow_map, + bptree_test_cow_setup, + bptree_test_cow_teardown), }; return cmocka_run_group_tests_name("bpt_cow", tests, NULL, NULL); } diff --git a/src/libsds/test/test_sds_set.c b/src/libsds/test/test_sds_set.c index e1080dc..3831bba 100644 --- a/src/libsds/test/test_sds_set.c +++ b/src/libsds/test/test_sds_set.c @@ -16,7 +16,7 @@ static int32_t cb_count = 0; static void -test_31_map_cb(void *k __attribute__((unused)), void *v __attribute__((unused))) +test_31_map_cb(void *k __attribute__((unused)), void *v __attribute__((unused)), void *arg __attribute__((unused))) { cb_count++; } @@ -39,7 +39,7 @@ test_31_map(void **state) result = sds_bptree_verify(binst); assert_int_equal(result, SDS_SUCCESS); } - sds_bptree_map(binst, test_31_map_cb); + sds_bptree_map(binst, NULL, test_31_map_cb); assert_int_equal(cb_count, 199); } -- 1.8.3.1