rhashtable updates on top of Herbert's work
Subsystems (6)
Thomas Graf
maintainercover 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
Thomas Graf
maintainerPATCH 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.
--- include/linux/rhashtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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;
Signature · 1 line
1.9.3
Thomas Graf
maintainerRe: [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.
Thomas Graf
maintainerPATCH 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
@@ -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
@@ -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
Thomas Graf
maintainerPATCH 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
@@ -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
Thomas Graf
maintainerPATCH 4/5 rhashtable: Disable automatic shrinking by default
In reply to Thomas GrafUsing 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
Herbert Xu
maintainerRe: [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 usingrhashtable_remove_and_shrink()Signed-off-by Thomas Graf <[email protected]>
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
Thomas Graf
maintainerRe: [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 usingrhashtable_remove_and_shrink()Signed-off-by Thomas Graf <[email protected]>Note that this patch doesn't completely disable automatic shrinking since the worker thread can still trigger it.
Right. I'll send a v2.
David Miller
maintainerRe: [PATCH 4/5 net-next] rhashtable: Disable automatic shrinking by default
In reply to Herbert XuFrom: 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 usingrhashtable_remove_and_shrink()Signed-off-by Thomas Graf <[email protected]>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.
Thomas Graf
maintainerPATCH v2 4/5 rhashtable: Disable automatic shrinking by default
In reply to Herbert XuIntroduce 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
David Miller
maintainerRe: [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_shrinkingto require the user to explicitly opt-in to automatic shrinking of tables.Signed-off-by Thomas Graf <[email protected]>---
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!
Thomas Graf
maintainerPATCH 5/5 rhashtable: Add rhashtable_free_and_destroy()
In reply to Thomas Grafrhashtable_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
@@ -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
@@ -187,26 +187,15 @@ static int nft_hash_init(const struct nft_set *set,return rhashtable_init (priv, ¶ms); } -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