From a2a7ff599330d13b146409b95cf054a71114c338 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Mon, 20 Aug 2012 20:25:37 -0400 Subject: [PATCH] rcuja: various fixes Signed-off-by: Mathieu Desnoyers --- rcuja/rcuja-internal.h | 16 ++++++ rcuja/rcuja-shadow-nodes.c | 62 +++++++++++++++++++----- rcuja/rcuja.c | 99 ++++++++++++++++++++++++++------------ tests/test_urcu_ja.c | 48 +++++++++++++++--- 4 files changed, 176 insertions(+), 49 deletions(-) diff --git a/rcuja/rcuja-internal.h b/rcuja/rcuja-internal.h index 0277808..67d121b 100644 --- a/rcuja/rcuja-internal.h +++ b/rcuja/rcuja-internal.h @@ -24,6 +24,8 @@ */ #include +#include +#include #include /* Never declared. Opaque type used to store flagged node pointers. */ @@ -79,6 +81,7 @@ enum { __attribute__((visibility("protected"))) int rcuja_shadow_clear(struct cds_lfht *ht, struct cds_ja_inode *node, + struct cds_ja_shadow_node *shadow_node, unsigned int flags); __attribute__((visibility("protected"))) @@ -91,4 +94,17 @@ struct cds_lfht *rcuja_create_ht(const struct rcu_flavor_struct *flavor); __attribute__((visibility("protected"))) int rcuja_delete_ht(struct cds_lfht *ht); +#define DEBUG + +#ifdef DEBUG +#define dbg_printf(fmt, args...) printf("[debug rcuja] " fmt, ## args) +#else +#define dbg_printf(fmt, args...) \ +do { \ + /* do nothing but check printf format */ \ + if (0) \ + printf("[debug rcuja] " fmt, ## args); \ +} while (0) +#endif + #endif /* _URCU_RCUJA_INTERNAL_H */ diff --git a/rcuja/rcuja-shadow-nodes.c b/rcuja/rcuja-shadow-nodes.c index 2b369ad..3a5de33 100644 --- a/rcuja/rcuja-shadow-nodes.c +++ b/rcuja/rcuja-shadow-nodes.c @@ -196,6 +196,7 @@ struct cds_ja_shadow_node *rcuja_shadow_lookup_lock(struct cds_lfht *ht, } shadow_node = caa_container_of(lookup_node, struct cds_ja_shadow_node, ht_node); + dbg_printf("Lock %p\n", shadow_node->lock); ret = pthread_mutex_lock(shadow_node->lock); assert(!ret); if (cds_lfht_is_node_deleted(lookup_node)) { @@ -213,6 +214,7 @@ void rcuja_shadow_unlock(struct cds_ja_shadow_node *shadow_node) { int ret; + dbg_printf("Unlock %p\n", shadow_node->lock); ret = pthread_mutex_unlock(shadow_node->lock); assert(!ret); } @@ -261,6 +263,14 @@ int rcuja_shadow_set(struct cds_lfht *ht, return 0; } +static +void free_shadow_node(struct rcu_head *head) +{ + struct cds_ja_shadow_node *shadow_node = + caa_container_of(head, struct cds_ja_shadow_node, head); + free(shadow_node); +} + static void free_shadow_node_and_node(struct rcu_head *head) { @@ -270,6 +280,15 @@ void free_shadow_node_and_node(struct rcu_head *head) free(shadow_node); } +static +void free_shadow_node_and_lock(struct rcu_head *head) +{ + struct cds_ja_shadow_node *shadow_node = + caa_container_of(head, struct cds_ja_shadow_node, head); + free(shadow_node->lock); + free(shadow_node); +} + static void free_shadow_node_and_node_and_lock(struct rcu_head *head) { @@ -283,16 +302,18 @@ void free_shadow_node_and_node_and_lock(struct rcu_head *head) __attribute__((visibility("protected"))) int rcuja_shadow_clear(struct cds_lfht *ht, struct cds_ja_inode *node, + struct cds_ja_shadow_node *shadow_node, unsigned int flags) { struct cds_lfht_iter iter; struct cds_lfht_node *lookup_node; - struct cds_ja_shadow_node *shadow_node; const struct rcu_flavor_struct *flavor; int ret, lockret; + int lookup_shadow = 0; flavor = cds_lfht_rcu_flavor(ht); flavor->read_lock(); + cds_lfht_lookup(ht, hash_pointer(node, hash_seed), match_pointer, node, &iter); lookup_node = cds_lfht_iter_get_node(&iter); @@ -300,10 +321,14 @@ int rcuja_shadow_clear(struct cds_lfht *ht, ret = -ENOENT; goto rcu_unlock; } - shadow_node = caa_container_of(lookup_node, - struct cds_ja_shadow_node, ht_node); - lockret = pthread_mutex_lock(shadow_node->lock); - assert(!lockret); + + if (!shadow_node) { + shadow_node = caa_container_of(lookup_node, + struct cds_ja_shadow_node, ht_node); + lockret = pthread_mutex_lock(shadow_node->lock); + assert(!lockret); + lookup_shadow = 1; + } /* * Holding the mutex across deletion, and by also re-checking if @@ -312,17 +337,28 @@ int rcuja_shadow_clear(struct cds_lfht *ht, */ ret = cds_lfht_del(ht, lookup_node); if (!ret) { - assert(flags & RCUJA_SHADOW_CLEAR_FREE_NODE); - if (flags & RCUJA_SHADOW_CLEAR_FREE_LOCK) { - flavor->update_call_rcu(&shadow_node->head, - free_shadow_node_and_node_and_lock); + if (flags & RCUJA_SHADOW_CLEAR_FREE_NODE) { + if (flags & RCUJA_SHADOW_CLEAR_FREE_LOCK) { + flavor->update_call_rcu(&shadow_node->head, + free_shadow_node_and_node_and_lock); + } else { + flavor->update_call_rcu(&shadow_node->head, + free_shadow_node_and_node); + } } else { - flavor->update_call_rcu(&shadow_node->head, - free_shadow_node_and_node); + if (flags & RCUJA_SHADOW_CLEAR_FREE_LOCK) { + flavor->update_call_rcu(&shadow_node->head, + free_shadow_node_and_lock); + } else { + flavor->update_call_rcu(&shadow_node->head, + free_shadow_node); + } } } - lockret = pthread_mutex_unlock(shadow_node->lock); - assert(!lockret); + if (lookup_shadow) { + lockret = pthread_mutex_unlock(shadow_node->lock); + assert(!lockret); + } rcu_unlock: flavor->read_unlock(); diff --git a/rcuja/rcuja.c b/rcuja/rcuja.c index 945b299..64c6c42 100644 --- a/rcuja/rcuja.c +++ b/rcuja/rcuja.c @@ -65,14 +65,14 @@ struct cds_ja_type { * child type. */ #define JA_TYPE_BITS 3 -#define JA_TYPE_MAX_NR (1U << JA_TYPE_BITS) +#define JA_TYPE_MAX_NR (1UL << JA_TYPE_BITS) #define JA_TYPE_MASK (JA_TYPE_MAX_NR - 1) #define JA_PTR_MASK (~JA_TYPE_MASK) #define JA_ENTRY_PER_NODE 256UL #define JA_BITS_PER_BYTE 3 -#define JA_MAX_INTERNAL_DEPTH 5 /* Maximum depth, excluding leafs */ +#define JA_MAX_DEPTH 5 /* Maximum depth, including leafs */ /* * Entry for NULL node is at index 8 of the table. It is never encoded @@ -254,28 +254,28 @@ struct cds_ja_inode { static struct cds_ja_inode_flag *ja_node_flag(struct cds_ja_inode *node, - unsigned int type) + unsigned long type) { - assert(type < RCU_JA_NR_TYPES); + assert(type < (1UL << JA_TYPE_BITS)); return (struct cds_ja_inode_flag *) (((unsigned long) node) | type); } static struct cds_ja_inode *ja_node_ptr(struct cds_ja_inode_flag *node) { - return (struct cds_ja_inode *) (((unsigned long) node) | JA_PTR_MASK); + return (struct cds_ja_inode *) (((unsigned long) node) & JA_PTR_MASK); } static -unsigned int ja_node_type(struct cds_ja_inode_flag *node) +unsigned long ja_node_type(struct cds_ja_inode_flag *node) { - unsigned int type; + unsigned long type; if (ja_node_ptr(node) == NULL) { return NODE_INDEX_NULL; } type = (unsigned int) ((unsigned long) node & JA_TYPE_MASK); - assert(type < RCU_JA_NR_TYPES); + assert(type < (1UL << JA_TYPE_BITS)); return type; } @@ -470,9 +470,9 @@ int ja_linear_node_set_nth(const struct cds_ja_type *type, assert(type->type_class == RCU_JA_LINEAR || type->type_class == RCU_JA_POOL); nr_child_ptr = &node->u.data[0]; + dbg_printf("linear set nth: nr_child_ptr %p\n", nr_child_ptr); nr_child = *nr_child_ptr; assert(nr_child <= type->max_linear_child); - assert(type->type_class != RCU_JA_LINEAR || nr_child >= type->min_child); values = &node->u.data[1]; for (i = 0; i < nr_child; i++) { @@ -490,6 +490,11 @@ int ja_linear_node_set_nth(const struct cds_ja_type *type, cmm_smp_wmb(); /* write value and pointer before nr_child */ CMM_STORE_SHARED(*nr_child_ptr, nr_child + 1); shadow_node->nr_child++; + dbg_printf("linear set nth: %u child, shadow: %u child, for node %p shadow %p\n", + (unsigned int) CMM_LOAD_SHARED(*nr_child_ptr), + (unsigned int) shadow_node->nr_child, + node, shadow_node); + return 0; } @@ -581,17 +586,20 @@ int ja_node_recompact_add(struct cds_ja *ja, int new_shadow = 0; int ret; - if (!shadow_node) { + if (!shadow_node || old_type_index == NODE_INDEX_NULL) { new_type_index = 0; } else { new_type_index = old_type_index + 1; } + dbg_printf("Recompact to type %d\n", new_type_index); + new_type = &ja_types[new_type_index]; new_node = alloc_cds_ja_node(new_type); if (!new_node) return -ENOMEM; new_node_flag = ja_node_flag(new_node, new_type_index); + dbg_printf("Recompact inherit lock from %p\n", shadow_node); ret = rcuja_shadow_set(ja->ht, new_node, shadow_node); if (ret) { free(new_node); @@ -658,6 +666,9 @@ int ja_node_recompact_add(struct cds_ja *ja, } break; } + case RCU_JA_NULL: + /* Nothing to copy */ + break; case RCU_JA_PIGEON: default: assert(0); @@ -671,9 +682,11 @@ int ja_node_recompact_add(struct cds_ja *ja, assert(!ret); /* Return pointer to new recompacted new through old_node_flag */ *old_node_flag = new_node_flag; - ret = rcuja_shadow_clear(ja->ht, old_node, - RCUJA_SHADOW_CLEAR_FREE_NODE); - assert(!ret); + if (old_node) { + ret = rcuja_shadow_clear(ja->ht, old_node, shadow_node, + RCUJA_SHADOW_CLEAR_FREE_NODE); + assert(!ret); + } ret = 0; @@ -698,6 +711,9 @@ int ja_node_set_nth(struct cds_ja *ja, const struct cds_ja_type *type; struct cds_ja_inode *node; + dbg_printf("ja_node_set_nth for n=%u, node %p, shadow %p\n", + (unsigned int) n, ja_node_ptr(*node_flag), shadow_node); + node = ja_node_ptr(*node_flag); type_index = ja_node_type(*node_flag); type = &ja_types[type_index]; @@ -765,9 +781,11 @@ int ja_attach_node(struct cds_ja *ja, struct cds_hlist_head head; struct cds_ja_inode_flag *iter_node_flag, *iter_dest_node_flag; int ret, i; - struct cds_ja_inode_flag *created_nodes[JA_MAX_INTERNAL_DEPTH]; + struct cds_ja_inode_flag *created_nodes[JA_MAX_DEPTH]; int nr_created_nodes = 0; + dbg_printf("Attach node at depth %u\n", depth); + assert(node); shadow_node = rcuja_shadow_lookup_lock(ja->ht, node); if (!shadow_node) { @@ -783,24 +801,24 @@ int ja_attach_node(struct cds_ja *ja, } } + /* Create new branch, starting from bottom */ CDS_INIT_HLIST_HEAD(&head); cds_hlist_add_head_rcu(&child_node->list, &head); + iter_node_flag = (struct cds_ja_inode_flag *) head.next; - iter_dest_node_flag = NULL; - ret = ja_node_set_nth(ja, &iter_dest_node_flag, - key >> (JA_BITS_PER_BYTE * (ja->tree_depth - 2)), - (struct cds_ja_inode_flag *) head.next, - NULL); + /* Create shadow node for the leaf node */ + dbg_printf("leaf shadow node creation\n"); + ret = rcuja_shadow_set(ja->ht, ja_node_ptr(iter_node_flag), NULL); if (ret) - goto unlock_parent; - created_nodes[nr_created_nodes++] = iter_dest_node_flag; - iter_node_flag = iter_dest_node_flag; + goto check_error; + created_nodes[nr_created_nodes++] = iter_node_flag; - /* Create new branch, starting from bottom */ - for (i = ja->tree_depth - 2; i >= (int) depth; i--) { + for (i = ja->tree_depth - 1; i >= (int) depth; i--) { + dbg_printf("branch creation level %d, key %" PRIu64 "\n", + i, key >> (JA_BITS_PER_BYTE * (i - 2))); iter_dest_node_flag = NULL; ret = ja_node_set_nth(ja, &iter_dest_node_flag, - key >> (JA_BITS_PER_BYTE * (i - 1)), + key >> (JA_BITS_PER_BYTE * (i - 2)), iter_node_flag, NULL); if (ret) @@ -809,7 +827,22 @@ int ja_attach_node(struct cds_ja *ja, iter_node_flag = iter_dest_node_flag; } + if (depth > 1) { + /* We need to use set_nth on the previous level. */ + iter_dest_node_flag = node_flag; + ret = ja_node_set_nth(ja, &iter_dest_node_flag, + key >> (JA_BITS_PER_BYTE * (depth - 2)), + iter_node_flag, + shadow_node); + if (ret) + goto check_error; + created_nodes[nr_created_nodes++] = iter_dest_node_flag; + iter_node_flag = iter_dest_node_flag; + } + /* Publish new branch */ + dbg_printf("Publish branch %p, replacing %p\n", + iter_node_flag, *node_flag_ptr); rcu_assign_pointer(*node_flag_ptr, iter_node_flag); /* Success */ @@ -819,14 +852,18 @@ check_error: if (ret) { for (i = 0; i < nr_created_nodes; i++) { int tmpret; + int flags; + + flags = RCUJA_SHADOW_CLEAR_FREE_LOCK; + if (i) + flags |= RCUJA_SHADOW_CLEAR_FREE_NODE; tmpret = rcuja_shadow_clear(ja->ht, ja_node_ptr(created_nodes[i]), - RCUJA_SHADOW_CLEAR_FREE_NODE - | RCUJA_SHADOW_CLEAR_FREE_LOCK); + NULL, + flags); assert(!tmpret); } } -unlock_parent: if (parent_shadow_node) rcuja_shadow_unlock(parent_shadow_node); unlock_shadow: @@ -874,6 +911,8 @@ int cds_ja_add(struct cds_ja *ja, uint64_t key, tree_depth = ja->tree_depth; retry: + dbg_printf("cds_ja_add attempt: key %" PRIu64 ", node %p\n", + key, new_node); iter_key = key; parent2_node_flag = NULL; parent_node_flag = @@ -882,7 +921,7 @@ retry: node_flag = rcu_dereference(*node_flag_ptr); /* Iterate on all internal levels */ - for (i = 0; i < tree_depth - 1; i++) { + for (i = 1; i < tree_depth; i++) { if (!ja_node_ptr(node_flag)) { ret = ja_attach_node(ja, node_flag_ptr, parent_node_flag, parent2_node_flag, @@ -948,7 +987,7 @@ struct cds_ja *_cds_ja_new(unsigned int key_bits, /* ja->root is NULL */ /* tree_depth 0 is for pointer to root node */ ja->tree_depth = (key_bits >> JA_BITS_PER_BYTE) + 1; - assert(ja->tree_depth <= JA_MAX_INTERNAL_DEPTH + 1); + assert(ja->tree_depth <= JA_MAX_DEPTH); ja->ht = rcuja_create_ht(flavor); if (!ja->ht) goto ht_error; diff --git a/tests/test_urcu_ja.c b/tests/test_urcu_ja.c index 2130670..1f67b01 100644 --- a/tests/test_urcu_ja.c +++ b/tests/test_urcu_ja.c @@ -22,6 +22,7 @@ #define _GNU_SOURCE #include "test_urcu_ja.h" +#include DEFINE_URCU_TLS(unsigned int, rand_lookup); DEFINE_URCU_TLS(unsigned long, nr_add); @@ -182,6 +183,7 @@ int main(int argc, char **argv) tot_add = 0, tot_add_exist = 0, tot_remove = 0; int i, a, ret; unsigned int remain; + uint64_t key; if (argc < 4) { show_usage(argc, argv); @@ -308,21 +310,55 @@ int main(int argc, char **argv) } /* Add keys */ - for (i = 0; i < 200; i++) { + printf("Test #1: add keys (8-bit).\n"); + for (key = 0; key < 200; key++) { struct ja_test_node *node = calloc(sizeof(*node), 1); - ja_test_node_init(node, i); - ret = cds_ja_add(test_ja, i, &node->node); + ja_test_node_init(node, key); + rcu_read_lock(); + ret = cds_ja_add(test_ja, key, &node->node); + rcu_read_unlock(); if (ret) { - printf("Error adding node (%d)\n", ret); - return -1; + fprintf(stderr, "Error (%d) adding node %" PRIu64 "\n", + ret, key); + assert(0); } } + printf("OK\n"); + + printf("Test #2: successful key lookup (8-bit).\n"); + for (key = 0; key < 200; key++) { + struct cds_hlist_head *head; + + rcu_read_lock(); + head = cds_ja_lookup(test_ja, key); + if (!head) { + fprintf(stderr, "Error lookup node %" PRIu64 "\n", key); + assert(0); + } + rcu_read_unlock(); + } + printf("OK\n"); + printf("Test #3: unsuccessful key lookup (8-bit).\n"); + for (key = 200; key < 240; key++) { + struct cds_hlist_head *head; + + rcu_read_lock(); + head = cds_ja_lookup(test_ja, key); + if (head) { + fprintf(stderr, + "Error unexpected lookup node %" PRIu64 "\n", + key); + assert(0); + } + rcu_read_unlock(); + } + printf("OK\n"); ret = cds_ja_destroy(test_ja); if (ret) { - printf("Error destroying judy array\n"); + fprintf(stderr, "Error destroying judy array\n"); return -1; } printf("Test end.\n"); -- 2.34.1