Re: [PATCH 3/8] rhashtable: use cmpxchg() to protect ->future_tbl.
On Sat, May 05 2018, Herbert Xu wrote: > On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote: >> Rather than borrowing one of the bucket locks to >> protect ->future_tbl updates, use cmpxchg(). >> This gives more freedom to change how bucket locking >> is implemented. >> >> Signed-off-by: NeilBrown > > This looks nice. > >> -spin_unlock_bh(old_tbl->locks); >> +rcu_assign_pointer(tmp, new_tbl); > > Do we need this barrier since cmpxchg is supposed to provide memory > barrier semantics? It's hard to find documentation even for what cmpxchg() is meant do, let alone what barriers is provides, but there does seem to be something hidden in Documentation/core-api/atomic_ops.rst which suggests full barrier semantics if the comparison succeeds. I'll replace the rcu_assign_pointer with a comment saying why it isn't needed. Thanks, NeilBrown > >> +if (cmpxchg(&old_tbl->future_tbl, NULL, tmp) != NULL) >> +return -EEXIST; > > Thanks, > -- > Email: Herbert Xu > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt signature.asc Description: PGP signature
Re: [PATCH 3/8] rhashtable: use cmpxchg() to protect ->future_tbl.
On Fri, May 04, 2018 at 01:54:14PM +1000, NeilBrown wrote: > Rather than borrowing one of the bucket locks to > protect ->future_tbl updates, use cmpxchg(). > This gives more freedom to change how bucket locking > is implemented. > > Signed-off-by: NeilBrown This looks nice. > - spin_unlock_bh(old_tbl->locks); > + rcu_assign_pointer(tmp, new_tbl); Do we need this barrier since cmpxchg is supposed to provide memory barrier semantics? > + if (cmpxchg(&old_tbl->future_tbl, NULL, tmp) != NULL) > + return -EEXIST; Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[PATCH 3/8] rhashtable: use cmpxchg() to protect ->future_tbl.
Rather than borrowing one of the bucket locks to protect ->future_tbl updates, use cmpxchg(). This gives more freedom to change how bucket locking is implemented. Signed-off-by: NeilBrown --- lib/rhashtable.c | 17 ++--- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/lib/rhashtable.c b/lib/rhashtable.c index 4a3f94e8e8a6..b73afe1dec7e 100644 --- a/lib/rhashtable.c +++ b/lib/rhashtable.c @@ -298,21 +298,16 @@ static int rhashtable_rehash_attach(struct rhashtable *ht, struct bucket_table *old_tbl, struct bucket_table *new_tbl) { - /* Protect future_tbl using the first bucket lock. */ - spin_lock_bh(old_tbl->locks); - - /* Did somebody beat us to it? */ - if (rcu_access_pointer(old_tbl->future_tbl)) { - spin_unlock_bh(old_tbl->locks); - return -EEXIST; - } - /* Make insertions go into the new, empty table right away. Deletions * and lookups will be attempted in both tables until we synchronize. +* The use of 'tmp' is simply to ensure we get the required memory +* barriers before the cmpxchg(). */ - rcu_assign_pointer(old_tbl->future_tbl, new_tbl); + struct bucket_table *tmp; - spin_unlock_bh(old_tbl->locks); + rcu_assign_pointer(tmp, new_tbl); + if (cmpxchg(&old_tbl->future_tbl, NULL, tmp) != NULL) + return -EEXIST; return 0; }