* Lai Jiangshan ([email protected]) wrote: > Make a function only do one thing. > > Signed-off-by: Lai Jiangshan <[email protected]> > --- > rculfhash.c | 38 ++++++++++++++------------------------ > 1 files changed, 14 insertions(+), 24 deletions(-) > > diff --git a/rculfhash.c b/rculfhash.c > index 8433ec4..f412c6f 100644 > --- a/rculfhash.c > +++ b/rculfhash.c > @@ -264,7 +264,6 @@ struct partition_resize_work { > enum add_mode { > ADD_DEFAULT = 0, > ADD_UNIQUE = 1, > - ADD_REPLACE = 2, > }; > > static > @@ -883,16 +882,13 @@ struct cds_lfht_node *_cds_lfht_add(struct cds_lfht *ht, > next = rcu_dereference(clear_flag(iter)->p.next); > if (unlikely(is_removed(next))) > goto gc_node; > - if ((mode == ADD_UNIQUE || mode == ADD_REPLACE) > + if ((mode == ADD_UNIQUE) > && !is_dummy(next) > && clear_flag(iter)->p.reverse_hash == > node->p.reverse_hash > && !ht->compare_fct(node->key, node->key_len, > clear_flag(iter)->key, > clear_flag(iter)->key_len)) { > - if (mode == ADD_UNIQUE) > - return clear_flag(iter); > - else /* mode == ADD_REPLACE */ > - goto replace; > + return clear_flag(iter); > } > /* Only account for identical reverse hash once */ > if (iter_prev->p.reverse_hash != > clear_flag(iter)->p.reverse_hash > @@ -919,23 +915,10 @@ struct cds_lfht_node *_cds_lfht_add(struct cds_lfht *ht, > new_node) != iter) { > continue; /* retry */ > } else { > - if (mode == ADD_REPLACE) > - return_node = NULL; > - else /* ADD_DEFAULT and ADD_UNIQUE */ > - return_node = node; > + return_node = node; > goto end; > } > > - replace: > - > - if (!_cds_lfht_replace(ht, size, clear_flag(iter), next, > - node)) { > - return_node = clear_flag(iter); > - goto end; /* gc already done */ > - } else { > - continue; /* retry */ > - } > - > gc_node: > assert(!is_removed(iter)); > if (is_dummy(iter)) > @@ -1455,10 +1438,17 @@ struct cds_lfht_node *cds_lfht_add_replace(struct > cds_lfht *ht, > node->p.reverse_hash = bit_reverse_ulong((unsigned long) hash); > > size = rcu_dereference(ht->t.size); > - ret = _cds_lfht_add(ht, size, node, ADD_REPLACE, 0); > - if (ret == NULL) > - ht_count_add(ht, size); > - return ret; > + for (;;) { > + ret = _cds_lfht_add(ht, size, node, ADD_UNIQUE, 0); > + if (ret == node) { > + ht_count_add(ht, size); > + return NULL; > + } > + > + if (!_cds_lfht_replace(ht, size, ret, > + rcu_dereference(ret->p.next), node))
Hrm, if ret->p.next changes between the iteration done in _cds_lfht_add and this rcu_dereference, I think we may have an inconsistency. We should return the ret next pointer read from _cds_lfht_add (maybe by adding a parameter to _cds_lfht_add), and use the returned pointer here instead. This will ensure that the checks done within the _cds_lfht_add iteration (is the pointer null, or is it logically removed) are still valid. Re-fetching the next value here skips these checks. Thanks, Mathieu > + return ret; > + } > } > > int cds_lfht_replace(struct cds_lfht *ht, struct cds_lfht_iter *old_iter, > -- > 1.7.4.4 > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com _______________________________________________ ltt-dev mailing list [email protected] http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
