Skip to contentSkip to conversation

rhashtable updates on top of Herbert's work

PATCH In review Thomas Graf 3 participants 12 messages Mar 24, 2015

Subsystems (6)
lore
Share & tools
lore

Thomas Graf

author maintainer
#0 lore raw

cover rhashtable updates on top of Herbert's work

Patch 1 is a bugfix for an RCU splash I encountered while testing. Patch 2 & 3 are pure cleanups. Patch 4 disables automatic shrinking by default as discussed in previous thread. Patch 5 removes some rhashtable internal knowledge from nft_hash and fixes another RCU splash.

I've pushed various rhashtable tests (Netlink, nft) together with a Makefile to a git tree [0] for easier stress testing.

[0] https://github.com/tgraf/rhashtable

Thomas Graf (5):
rhashtable: Extend RCU read lock into rhashtable_insert_rehash()
rhashtable: Use 'unsigned int' consistently
rhashtable: Mark internal/private inline functions as such
rhashtable: Disable automatic shrinking by default
rhashtable: Add rhashtable_free_and_destroy()

 include/linux/rhashtable.h | 50 +++++++++++++++++++++++-----------
 lib/rhashtable.c           | 67 +++++++++++++++++++++++++++++++++-------------
 lib/test_rhashtable.c      |  2 +-
 net/netfilter/nft_hash.c   | 27 ++++++-------------
 net/netlink/af_netlink.c   |  4 +--
 net/tipc/socket.c          |  3 ++-
 6 files changed, 96 insertions(+), 57 deletions(-)
Signature · 1 line
1.9.3
author maintainer
#1 lore raw

PATCH 1/5 rhashtable: Extend RCU read lock into rhashtable_insert_rehash()

rhashtable_insert_rehash() requires RCU locks to be held in order to access ht->tbl and traverse to the last table.

Fixes ccd57b1bd324 ("rhashtable: Add immediate rehash during insertion")
---
 include/linux/rhashtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/rhashtable.h +1 -1
index d7be9cb..5976ab5 100644
@@ -585,8 +585,8 @@ restart:
 	if (unlikely(rht_grow_above_100(ht, tbl))) {
 slow_path:
 		spin_unlock_bh(lock);
-		rcu_read_unlock();
 		err = rhashtable_insert_rehash(ht);
+		rcu_read_unlock();
 		if (err)
 			return err;
 
Signature · 1 line
1.9.3
author maintainer
#2 lore raw

Re: [PATCH 1/5 net-next] rhashtable: Extend RCU read lock into rhashtable_insert_rehash()

include/linux/rhashtable.h restart: · line 591

Quoted 9 lines · Thomas Graf
include/linux/rhashtable.h +1 -1
@@ -585,8 +585,8 @@ restart:
 	if (unlikely(rht_grow_above_100(ht, tbl))) {
 slow_path:
 		spin_unlock_bh(lock);
-		rcu_read_unlock();
 		err = rhashtable_insert_rehash(ht);
+		rcu_read_unlock();
 		if (err)
 			return err;

An alternative would be to start a new RCU section inside rhashtable_insert_rehash(). This was the easier fix but I'm fine either way.

author maintainer
#3 lore raw

PATCH 2/5 rhashtable: Use 'unsigned int' consistently

In reply to Thomas Graf
---
 include/linux/rhashtable.h | 14 +++++++-------
 lib/rhashtable.c           | 18 ++++++++++--------
 2 files changed, 17 insertions(+), 15 deletions(-)
include/linux/rhashtable.h +7 -7
index 5976ab5..f89cda0 100644
@@ -208,13 +208,13 @@ static inline unsigned int rht_key_hashfn(
 	struct rhashtable *ht, const struct bucket_table *tbl,
 	const void *key, const struct rhashtable_params params)
 {
-	unsigned hash;
+	unsigned int hash;
 
 	/* params must be equal to ht->p if it isn't constant. */
 	if (!__builtin_constant_p(params.key_len))
 		hash = ht->p.hashfn(key, ht->key_len, tbl->hash_rnd);
 	else if (params.key_len) {
-		unsigned key_len = params.key_len;
+		unsigned int key_len = params.key_len;
 
 		if (params.hashfn)
 			hash = params.hashfn(key, key_len, tbl->hash_rnd);
@@ -224,7 +224,7 @@ static inline unsigned int rht_key_hashfn(
 			hash = jhash2(key, key_len / sizeof(u32),
 				      tbl->hash_rnd);
 	} else {
-		unsigned key_len = ht->p.key_len;
+		unsigned int key_len = ht->p.key_len;
 
 		if (params.hashfn)
 			hash = params.hashfn(key, key_len, tbl->hash_rnd);
@@ -512,7 +512,7 @@ static inline void *rhashtable_lookup_fast(
 	};
 	const struct bucket_table *tbl;
 	struct rhash_head *he;
-	unsigned hash;
+	unsigned int hash;
 
 	rcu_read_lock();
 
@@ -550,8 +550,8 @@ static inline int __rhashtable_insert_fast(
 	struct bucket_table *tbl, *new_tbl;
 	struct rhash_head *head;
 	spinlock_t *lock;
-	unsigned elasticity;
-	unsigned hash;
+	unsigned int elasticity;
+	unsigned int hash;
 	int err;
 
 restart:
@@ -718,7 +718,7 @@ static inline int __rhashtable_remove_fast(
 	struct rhash_head __rcu **pprev;
 	struct rhash_head *he;
 	spinlock_t * lock;
-	unsigned hash;
+	unsigned int hash;
 	int err = -ENOENT;
 
 	hash = rht_head_hashfn(ht, tbl, obj, params);
lib/rhashtable.c +10 -8
index e96ad1a..93374d7 100644
@@ -153,7 +153,7 @@ static struct bucket_table *rhashtable_last_table(struct rhashtable *ht,
 	return new_tbl;
 }
 
-static int rhashtable_rehash_one(struct rhashtable *ht, unsigned old_hash)
+static int rhashtable_rehash_one(struct rhashtable *ht, unsigned int old_hash)
 {
 	struct bucket_table *old_tbl = rht_dereference(ht->tbl, ht);
 	struct bucket_table *new_tbl = rhashtable_last_table(ht,
@@ -162,7 +162,7 @@ static int rhashtable_rehash_one(struct rhashtable *ht, unsigned old_hash)
 	int err = -ENOENT;
 	struct rhash_head *head, *next, *entry;
 	spinlock_t *new_bucket_lock;
-	unsigned new_hash;
+	unsigned int new_hash;
 
 	rht_for_each(entry, old_tbl, old_hash) {
 		err = 0;
@@ -199,7 +199,8 @@ out:
 	return err;
 }
 
-static void rhashtable_rehash_chain(struct rhashtable *ht, unsigned old_hash)
+static void rhashtable_rehash_chain(struct rhashtable *ht,
+				    unsigned int old_hash)
 {
 	struct bucket_table *old_tbl = rht_dereference(ht->tbl, ht);
 	spinlock_t *old_bucket_lock;
@@ -244,7 +245,7 @@ static int rhashtable_rehash_table(struct rhashtable *ht)
 	struct bucket_table *old_tbl = rht_dereference(ht->tbl, ht);
 	struct bucket_table *new_tbl;
 	struct rhashtable_walker *walker;
-	unsigned old_hash;
+	unsigned int old_hash;
 
 	new_tbl = rht_dereference(old_tbl->future_tbl, ht);
 	if (!new_tbl)
@@ -324,11 +325,12 @@ static int rhashtable_expand(struct rhashtable *ht)
 static int rhashtable_shrink(struct rhashtable *ht)
 {
 	struct bucket_table *new_tbl, *old_tbl = rht_dereference(ht->tbl, ht);
-	unsigned size = roundup_pow_of_two(atomic_read(&ht->nelems) * 3 / 2);
+	unsigned int size;
 	int err;
 
 	ASSERT_RHT_MUTEX(ht);
 
+	size = roundup_pow_of_two(atomic_read(&ht->nelems) * 3 / 2);
 	if (size < ht->p.min_size)
 		size = ht->p.min_size;
 
@@ -379,9 +381,9 @@ unlock:
 
 static bool rhashtable_check_elasticity(struct rhashtable *ht,
 					struct bucket_table *tbl,
-					unsigned hash)
+					unsigned int hash)
 {
-	unsigned elasticity = ht->elasticity;
+	unsigned int elasticity = ht->elasticity;
 	struct rhash_head *head;
 
 	rht_for_each(head, tbl, hash)
@@ -431,7 +433,7 @@ int rhashtable_insert_slow(struct rhashtable *ht, const void *key,
 			   struct bucket_table *tbl)
 {
 	struct rhash_head *head;
-	unsigned hash;
+	unsigned int hash;
 	int err;
 
 	tbl = rhashtable_last_table(ht, tbl);
Signature · 1 line
1.9.3
author maintainer
#4 lore raw

PATCH 3/5 rhashtable: Mark internal/private inline functions as such

In reply to Thomas Graf
---
 include/linux/rhashtable.h | 2 ++
 1 file changed, 2 insertions(+)
include/linux/rhashtable.h +2 -0
index f89cda0..0e1f975 100644
@@ -539,6 +539,7 @@ restart:
 	return NULL;
 }
 
+/* Internal function, please use rhashtable_insert_fast() instead */
 static inline int __rhashtable_insert_fast(
 	struct rhashtable *ht, const void *key, struct rhash_head *obj,
 	const struct rhashtable_params params)
@@ -711,6 +712,7 @@ static inline int rhashtable_lookup_insert_key(
 	return __rhashtable_insert_fast(ht, key, obj, params);
 }
 
+/* Internal function, please use rhashtable_remove_fast() instead */
 static inline int __rhashtable_remove_fast(
 	struct rhashtable *ht, struct bucket_table *tbl,
 	struct rhash_head *obj, const struct rhashtable_params params)
Signature · 1 line
1.9.3
author maintainer
#5 lore raw

PATCH 4/5 rhashtable: Disable automatic shrinking by default

In reply to Thomas Graf

Using rhashtable_remove_fast() will no longer automatically shrink the table. Users need to opt-in to shrinking by explicitly using rhashtable_remove_and_shrink()

---
 include/linux/rhashtable.h | 27 +++++++++++++++++++++------
 lib/test_rhashtable.c      |  2 +-
 net/netfilter/nft_hash.c   |  2 +-
 net/netlink/af_netlink.c   |  4 ++--
 net/tipc/socket.c          |  3 ++-
 5 files changed, 27 insertions(+), 11 deletions(-)
include/linux/rhashtable.h +21 -6
index 0e1f975..d0ca452 100644
@@ -2,7 +2,7 @@
  * Resizable, Scalable, Concurrent Hash Table
  *
  * Copyright (c) 2015 Herbert Xu <[email protected]>
- * Copyright (c) 2014 Thomas Graf <[email protected]>
+ * Copyright (c) 2014-2015 Thomas Graf <[email protected]>
  * Copyright (c) 2008-2014 Patrick McHardy <[email protected]>
  *
  * Code partially derived from nft_hash
@@ -780,14 +780,29 @@ static inline int rhashtable_remove_fast(
 	       (tbl = rht_dereference_rcu(tbl->future_tbl, ht)))
 		;
 
-	if (err)
-		goto out;
+	if (likely(!err))
+		atomic_dec(&ht->nelems);
+
+	rcu_read_unlock();
 
-	atomic_dec(&ht->nelems);
-	if (rht_shrink_below_30(ht, tbl))
+	return err;
+}
+
+static inline int rhashtable_remove_and_shrink(
+	struct rhashtable *ht, struct rhash_head *obj,
+	const struct rhashtable_params params)
+{
+	struct bucket_table *tbl;
+	int err;
+
+	rcu_read_lock();
+
+	tbl = rht_dereference_rcu(ht->tbl, ht);
+
+	err = rhashtable_remove_fast(ht, obj, params);
+	if (!err && rht_shrink_below_30(ht, tbl))
 		schedule_work(&ht->run_work);
 
-out:
 	rcu_read_unlock();
 
 	return err;
lib/test_rhashtable.c +1 -1
index a42a0d4..fe6d4a9 100644
@@ -166,7 +166,7 @@ static int __init test_rhashtable(struct rhashtable *ht)
 		obj = rhashtable_lookup_fast(ht, &key, test_rht_params);
 		BUG_ON(!obj);
 
-		rhashtable_remove_fast(ht, &obj->node, test_rht_params);
+		rhashtable_remove_and_shrink(ht, &obj->node, test_rht_params);
 		kfree(obj);
 	}
 
net/netfilter/nft_hash.c +1 -1
index ad39669..2986f1d 100644
@@ -89,7 +89,7 @@ static void nft_hash_remove(const struct nft_set *set,
 {
 	struct rhashtable *priv = nft_set_priv(set);
 
-	rhashtable_remove_fast(priv, elem->cookie, nft_hash_params);
+	rhashtable_remove_and_shrink(priv, elem->cookie, nft_hash_params);
 	synchronize_rcu();
 	kfree(elem->cookie);
 }
net/netlink/af_netlink.c +2 -2
index e2f7f28..b9d3215 100644
@@ -1094,8 +1094,8 @@ static void netlink_remove(struct sock *sk)
 	struct netlink_table *table;
 
 	table = &nl_table[sk->sk_protocol];
-	if (!rhashtable_remove_fast(&table->hash, &nlk_sk(sk)->node,
-				    netlink_rhashtable_params)) {
+	if (!rhashtable_remove_and_shrink(&table->hash, &nlk_sk(sk)->node,
+					  netlink_rhashtable_params)) {
 		WARN_ON(atomic_read(&sk->sk_refcnt) == 1);
 		__sock_put(sk);
 	}
net/tipc/socket.c +2 -1
index 6dd5bd9..33338b4 100644
@@ -2282,7 +2282,8 @@ static void tipc_sk_remove(struct tipc_sock *tsk)
 	struct sock *sk = &tsk->sk;
 	struct tipc_net *tn = net_generic(sock_net(sk), tipc_net_id);
 
-	if (!rhashtable_remove_fast(&tn->sk_rht, &tsk->node, tsk_rht_params)) {
+	if (!rhashtable_remove_and_shrink(&tn->sk_rht, &tsk->node,
+					  tsk_rht_params)) {
 		WARN_ON(atomic_read(&sk->sk_refcnt) == 1);
 		__sock_put(sk);
 	}
Signature · 1 line
1.9.3
maintainer
#6 lore raw

Re: [PATCH 4/5 net-next] rhashtable: Disable automatic shrinking by default

Quoted 5 lines · Thomas Graf

Using rhashtable_remove_fast() will no longer automatically shrink the table. Users need to opt-in to shrinking by explicitly using rhashtable_remove_and_shrink()

Note that this patch doesn't completely disable automatic shrinking since the worker thread can still trigger it.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author maintainer
#7 lore raw

Re: [PATCH 4/5 net-next] rhashtable: Disable automatic shrinking by default

Quoted 9 lines · Herbert Xu
Quoted 5 lines · Thomas Graf

Using rhashtable_remove_fast() will no longer automatically shrink the table. Users need to opt-in to shrinking by explicitly using rhashtable_remove_and_shrink()

Note that this patch doesn't completely disable automatic shrinking since the worker thread can still trigger it.

Right. I'll send a v2.

maintainer
#8 lore raw

Re: [PATCH 4/5 net-next] rhashtable: Disable automatic shrinking by default

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Wed, 25 Mar 2015 05:55:45 +1100

Quoted 9 lines · Thomas Graf
Quoted 5 lines

Using rhashtable_remove_fast() will no longer automatically shrink the table. Users need to opt-in to shrinking by explicitly using rhashtable_remove_and_shrink()

Note that this patch doesn't completely disable automatic shrinking since the worker thread can still trigger it.

Ok, but how do you feel about Thomas's series otherwise?

It looks fine to me.

author maintainer
#9 lore raw

PATCH v2 4/5 rhashtable: Disable automatic shrinking by default

In reply to Herbert Xu

Introduce a new bool automatic_shrinking to require the user to explicitly opt-in to automatic shrinking of tables.

---
v2: Use bool in params to also disable shrinking in worker as
pointed out by Herbert.

@Dave: Patch 5 still applied on top with some fuzz. If not,
let me know and I will respin.

 include/linux/rhashtable.h | 7 +++++--
 lib/rhashtable.c           | 2 +-
 net/netfilter/nft_hash.c   | 1 +
 net/netlink/af_netlink.c   | 1 +
 net/tipc/socket.c          | 1 +
 5 files changed, 9 insertions(+), 3 deletions(-)
include/linux/rhashtable.h +5 -2
index 0e1f975..ae26c49 100644
@@ -2,7 +2,7 @@
  * Resizable, Scalable, Concurrent Hash Table
  *
  * Copyright (c) 2015 Herbert Xu <[email protected]>
- * Copyright (c) 2014 Thomas Graf <[email protected]>
+ * Copyright (c) 2014-2015 Thomas Graf <[email protected]>
  * Copyright (c) 2008-2014 Patrick McHardy <[email protected]>
  *
  * Code partially derived from nft_hash
@@ -104,6 +104,7 @@ struct rhashtable;
  * @min_size: Minimum size while shrinking
  * @nulls_base: Base value to generate nulls marker
  * @insecure_elasticity: Set to true to disable chain length checks
+ * @automatic_shrinking: Enable automatic shrinking of tables
  * @locks_mul: Number of bucket locks to allocate per cpu (default: 128)
  * @hashfn: Hash function (default: jhash2 if !(key_len % 4), or jhash)
  * @obj_hashfn: Function to hash object
@@ -118,6 +119,7 @@ struct rhashtable_params {
 	unsigned int		min_size;
 	u32			nulls_base;
 	bool			insecure_elasticity;
+	bool			automatic_shrinking;
 	size_t			locks_mul;
 	rht_hashfn_t		hashfn;
 	rht_obj_hashfn_t	obj_hashfn;
@@ -784,7 +786,8 @@ static inline int rhashtable_remove_fast(
 		goto out;
 
 	atomic_dec(&ht->nelems);
-	if (rht_shrink_below_30(ht, tbl))
+	if (unlikely(ht->p.automatic_shrinking &&
+		     rht_shrink_below_30(ht, tbl)))
 		schedule_work(&ht->run_work);
 
 out:
lib/rhashtable.c +1 -1
index 93374d7..a0ad777 100644
@@ -367,7 +367,7 @@ static void rht_deferred_worker(struct work_struct *work)
 
 	if (rht_grow_above_75(ht, tbl))
 		rhashtable_expand(ht);
-	else if (rht_shrink_below_30(ht, tbl))
+	else if (ht->p.automatic_shrinking && rht_shrink_below_30(ht, tbl))
 		rhashtable_shrink(ht);
 
 	err = rhashtable_rehash_table(ht);
net/netfilter/nft_hash.c +1 -0
index ad39669..8577a37 100644
@@ -172,6 +172,7 @@ static const struct rhashtable_params nft_hash_params = {
 	.head_offset = offsetof(struct nft_hash_elem, node),
 	.key_offset = offsetof(struct nft_hash_elem, key),
 	.hashfn = jhash,
+	.automatic_shrinking = true,
 };
 
 static int nft_hash_init(const struct nft_set *set,
net/netlink/af_netlink.c +1 -0
index e2f7f28..4caa809 100644
@@ -3142,6 +3142,7 @@ static const struct rhashtable_params netlink_rhashtable_params = {
 	.obj_hashfn = netlink_hash,
 	.obj_cmpfn = netlink_compare,
 	.max_size = 65536,
+	.automatic_shrinking = true,
 };
 
 static int __init netlink_proto_init(void)
net/tipc/socket.c +1 -0
index 6dd5bd9..3cc099d 100644
@@ -2295,6 +2295,7 @@ static const struct rhashtable_params tsk_rht_params = {
 	.key_len = sizeof(u32), /* portid */
 	.max_size = 1048576,
 	.min_size = 256,
+	.automatic_shrinking = true,
 };
 
 int tipc_sk_rht_init(struct net *net)
Signature · 1 line
1.9.3
maintainer
#10 lore raw

Re: [PATCH 4/5 net-next v2] rhashtable: Disable automatic shrinking by default

From: Thomas Graf <[email protected]>
Date: Tue, 24 Mar 2015 20:42:19 +0000

Quoted 10 lines

Introduce a new bool automatic_shrinking to require the user to explicitly opt-in to automatic shrinking of tables.

---
v2: Use bool in params to also disable shrinking in worker as
pointed out by Herbert.

@Dave: Patch 5 still applied on top with some fuzz. If not,
let me know and I will respin.

Ok I applied this series with the updated patch #4, thanks!

author maintainer
#11 lore raw

PATCH 5/5 rhashtable: Add rhashtable_free_and_destroy()

In reply to Thomas Graf

rhashtable_destroy() variant which stops rehashes, iterates over the table and calls a callback to release resources.

Avoids need for nft_hash to embed rhashtable internals and allows to get rid of the being_destroyed flag. It also saves a 2nd mutex lock upon destruction.

Also fixes an RCU lockdep splash on nft set destruction due to calling rht_for_each_entry_safe() without holding bucket locks. Open code this loop as we need know that no mutations may occur in parallel.

---
 include/linux/rhashtable.h |  5 +++--
 lib/rhashtable.c           | 49 ++++++++++++++++++++++++++++++++++++----------
 net/netfilter/nft_hash.c   | 25 +++++++----------------
 3 files changed, 49 insertions(+), 30 deletions(-)
include/linux/rhashtable.h +3 -2
index d0ca452..1995db3 100644
@@ -134,12 +134,10 @@ struct rhashtable_params {
  * @run_work: Deferred worker to expand/shrink asynchronously
  * @mutex: Mutex to protect current/future table swapping
  * @lock: Spin lock to protect walker list
- * @being_destroyed: True if table is set up for destruction
  */
 struct rhashtable {
 	struct bucket_table __rcu	*tbl;
 	atomic_t			nelems;
-	bool                            being_destroyed;
 	unsigned int			key_len;
 	unsigned int			elasticity;
 	struct rhashtable_params	p;
@@ -332,6 +330,9 @@ int rhashtable_walk_start(struct rhashtable_iter *iter) __acquires(RCU);
 void *rhashtable_walk_next(struct rhashtable_iter *iter);
 void rhashtable_walk_stop(struct rhashtable_iter *iter) __releases(RCU);
 
+void rhashtable_free_and_destroy(struct rhashtable *ht,
+				 void (*free_fn)(void *ptr, void *arg),
+				 void *arg);
 void rhashtable_destroy(struct rhashtable *ht);
 
 #define rht_dereference(p, ht) \
lib/rhashtable.c +39 -10

75 lines not on this page · raw message

net/netfilter/nft_hash.c +7 -18
index 2986f1d..ad29965 100644
@@ -187,26 +187,15 @@ static int nft_hash_init(const struct nft_set *set,
 	return rhashtable_init(priv, &params);
 }
 
-static void nft_hash_destroy(const struct nft_set *set)
+static void nft_free_element(void *ptr, void *arg)
 {
-	struct rhashtable *priv = nft_set_priv(set);
-	const struct bucket_table *tbl;
-	struct nft_hash_elem *he;
-	struct rhash_head *pos, *next;
-	unsigned int i;
-
-	/* Stop an eventual async resizing */
-	priv->being_destroyed = true;
-	mutex_lock(&priv->mutex);
-
-	tbl = rht_dereference(priv->tbl, priv);
-	for (i = 0; i < tbl->size; i++) {
-		rht_for_each_entry_safe(he, pos, next, tbl, i, node)
-			nft_hash_elem_destroy(set, he);
-	}
-	mutex_unlock(&priv->mutex);
+	nft_hash_elem_destroy((const struct nft_set *)arg, ptr);
+}
 
-	rhashtable_destroy(priv);
+static void nft_hash_destroy(const struct nft_set *set)
+{
+	rhashtable_free_and_destroy(nft_set_priv(set), nft_free_element,
+				    (void *)set);
 }
 
 static bool nft_hash_estimate(const struct nft_set_desc *desc, u32 features,
Signature · 1 line
1.9.3