Re: [PATCH net-next v2 2/5] rhashtable: add function to replace an element

On Tue, Dec 8, 2015 at 1:39 AM, Herbert Xu [off-list ref] wrote:

David Miller [off-list ref] wrote:

quoted

From: Tom Herbert <redacted>
Date: Tue, 1 Dec 2015 15:11:09 -0800

quoted

+     lock = rht_bucket_lock(tbl, hash);
+
+     spin_lock_bh(lock);
+
+     pprev = &tbl->buckets[hash];
+     rht_for_each(he, tbl, hash) {
+             if (he != obj_old) {
+                     pprev = &he->next;
+                     continue;
+             }
+
+             rcu_assign_pointer(obj_new->next, obj_old->next);
+             rcu_assign_pointer(*pprev, obj_new);
+             err = 0;
+             break;

Are you sure this works fine in the presence of both parallel readers and
table expansion passes?

Good question.

What's more this is something that can be easily implemented
outside of rhashtable, i.e., by hashing a pointer to the actual
object rather than the object itself.  So I'd like to see some
pretty good reasons for penny-pinching on memory and adding more
complexity to rhashtable.

That creates one more level of indirection. I don't see how add an
atomic replace operation adds any complexity to the rhashtable, none
of the semantics for rhashtable need to be changed.

Cheers,
--
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt