Instead of removing a empty list node that might be reintroduced soon
thereafter, tentatively place the empty list node in the garbage
collector, then re-check if the list is empty again before deleting it.
This patch is aiming to simplify the garbage collection interaction
between the packet path and the workqueue to delete empty lists.
Fixes: 5c789e131cbb9 ("netfilter: nf_conncount: Add list lock and gc worker, and RCU for init tree search")
Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
---
@Shawn: This is an alternative proposal to your patch 1/3 and 2/3.
include/net/netfilter/nf_conntrack_count.h | 2 -
net/netfilter/nf_conncount.c | 69 +++++++++---------------------
2 files changed, 20 insertions(+), 51 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_count.h b/include/net/netfilter/nf_conntrack_count.h
index 4b2b2baf8ab4..66e7bf9006e1 100644
--- a/include/net/netfilter/nf_conntrack_count.h
+++ b/include/net/netfilter/nf_conntrack_count.h
@@ -8,14 +8,12 @@ struct nf_conncount_data;
enum nf_conncount_list_add {
NF_CONNCOUNT_ADDED, /* list add was ok */
NF_CONNCOUNT_ERR, /* -ENOMEM, must drop skb */
- NF_CONNCOUNT_SKIP, /* list is already reclaimed by gc */
};
struct nf_conncount_list {
spinlock_t list_lock;
struct list_head head; /* connections with the same filtering key */
unsigned int count; /* length of list */
- bool dead;
};
struct nf_conncount_data *nf_conncount_init(struct net *net, unsigned int family,
diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
index 9cd180bda092..7180877ff942 100644
--- a/net/netfilter/nf_conncount.c
+++ b/net/netfilter/nf_conncount.c
@@ -109,11 +109,6 @@ nf_conncount_add(struct nf_conncount_list *list,
conn->jiffies32 = (u32)jiffies;
conn->dead = false;
spin_lock_bh(&list->list_lock);
- if (list->dead == true) {
- kmem_cache_free(conncount_conn_cachep, conn);
- spin_unlock_bh(&list->list_lock);
- return NF_CONNCOUNT_SKIP;
- }
list_add_tail(&conn->node, &list->head);
list->count++;
spin_unlock_bh(&list->list_lock);
@@ -129,34 +124,27 @@ static void __conn_free(struct rcu_head *h)
kmem_cache_free(conncount_conn_cachep, conn);
}
-static bool conn_free(struct nf_conncount_list *list,
+static void conn_free(struct nf_conncount_list *list,
struct nf_conncount_tuple *conn)
{
- bool free_entry = false;
-
spin_lock_bh(&list->list_lock);
if (conn->dead) {
spin_unlock_bh(&list->list_lock);
- return free_entry;
+ return;
}
list->count--;
conn->dead = true;
list_del_rcu(&conn->node);
- if (list->count == 0) {
- list->dead = true;
- free_entry = true;
- }
spin_unlock_bh(&list->list_lock);
call_rcu(&conn->rcu_head, __conn_free);
- return free_entry;
}
static const struct nf_conntrack_tuple_hash *
find_or_evict(struct net *net, struct nf_conncount_list *list,
- struct nf_conncount_tuple *conn, bool *free_entry)
+ struct nf_conncount_tuple *conn)
{
const struct nf_conntrack_tuple_hash *found;
unsigned long a, b;
@@ -176,7 +164,7 @@ find_or_evict(struct net *net, struct nf_conncount_list *list,
*/
age = a - b;
if (conn->cpu == cpu || age >= 2) {
- *free_entry = conn_free(list, conn);
+ conn_free(list, conn);
return ERR_PTR(-ENOENT);
}
@@ -193,7 +181,6 @@ void nf_conncount_lookup(struct net *net,
struct nf_conncount_tuple *conn, *conn_n;
struct nf_conn *found_ct;
unsigned int collect = 0;
- bool free_entry = false;
/* best effort only */
*addit = tuple ? true : false;
@@ -203,7 +190,7 @@ void nf_conncount_lookup(struct net *net,
if (collect > CONNCOUNT_GC_MAX_NODES)
break;
- found = find_or_evict(net, list, conn, &free_entry);
+ found = find_or_evict(net, list, conn);
if (IS_ERR(found)) {
/* Not found, but might be about to be confirmed */
if (PTR_ERR(found) == -EAGAIN) {
@@ -251,7 +238,6 @@ void nf_conncount_list_init(struct nf_conncount_list *list)
spin_lock_init(&list->list_lock);
INIT_LIST_HEAD(&list->head);
list->count = 0;
- list->dead = false;
}
EXPORT_SYMBOL_GPL(nf_conncount_list_init);
@@ -263,17 +249,14 @@ bool nf_conncount_gc_list(struct net *net,
struct nf_conncount_tuple *conn, *conn_n;
struct nf_conn *found_ct;
unsigned int collected = 0;
- bool free_entry = false;
bool ret = false;
list_for_each_entry_safe(conn, conn_n, &list->head, node) {
- found = find_or_evict(net, list, conn, &free_entry);
+ found = find_or_evict(net, list, conn);
if (IS_ERR(found)) {
- if (PTR_ERR(found) == -ENOENT) {
- if (free_entry)
- return true;
+ if (PTR_ERR(found) == -ENOENT)
collected++;
- }
+
continue;
}
@@ -284,8 +267,7 @@ bool nf_conncount_gc_list(struct net *net,
* closed already -> ditch it
*/
nf_ct_put(found_ct);
- if (conn_free(list, conn))
- return true;
+ conn_free(list, conn);
collected++;
continue;
}
@@ -296,10 +278,8 @@ bool nf_conncount_gc_list(struct net *net,
}
spin_lock_bh(&list->list_lock);
- if (!list->count) {
- list->dead = true;
+ if (!list->count)
ret = true;
- }
spin_unlock_bh(&list->list_lock);
return ret;
@@ -314,17 +294,19 @@ static void __tree_nodes_free(struct rcu_head *h)
kmem_cache_free(conncount_rb_cachep, rbconn);
}
-static void tree_nodes_free(struct rb_root *root,
- struct nf_conncount_rb *gc_nodes[],
- unsigned int gc_count)
+static void gc_tree_run(struct rb_root *root,
+ struct nf_conncount_rb *gc_nodes[],
+ unsigned int gc_count)
{
struct nf_conncount_rb *rbconn;
while (gc_count) {
rbconn = gc_nodes[--gc_count];
spin_lock(&rbconn->list.list_lock);
- rb_erase(&rbconn->node, root);
- call_rcu(&rbconn->rcu_head, __tree_nodes_free);
+ if (rbconn->list.count == 0) {
+ rb_erase(&rbconn->node, root);
+ call_rcu(&rbconn->rcu_head, __tree_nodes_free);
+ }
spin_unlock(&rbconn->list.list_lock);
}
}
@@ -375,11 +357,6 @@ insert_tree(struct net *net,
count = 0; /* hotdrop */
} else if (ret == NF_CONNCOUNT_ADDED) {
count = rbconn->list.count;
- } else {
- /* NF_CONNCOUNT_SKIP, rbconn is already
- * reclaimed by gc, insert a new tree node
- */
- node_found = false;
}
break;
}
@@ -392,7 +369,7 @@ insert_tree(struct net *net,
}
if (gc_count) {
- tree_nodes_free(root, gc_nodes, gc_count);
+ gc_tree_run(root, gc_nodes, gc_count);
/* tree_node_free before new allocation permits
* allocator to re-use newly free'd object.
*
@@ -476,11 +453,6 @@ count_tree(struct net *net,
return 0; /* hotdrop */
} else if (ret == NF_CONNCOUNT_ADDED) {
return rbconn->list.count;
- } else {
- /* NF_CONNCOUNT_SKIP, rbconn is already
- * reclaimed by gc, insert a new tree node
- */
- break;
}
}
}
@@ -512,9 +484,8 @@ static void tree_gc_worker(struct work_struct *work)
spin_lock_bh(&nf_conncount_locks[tree]);
- if (gc_count) {
- tree_nodes_free(root, gc_nodes, gc_count);
- }
+ if (gc_count)
+ gc_tree_run(root, gc_nodes, gc_count);
clear_bit(tree, data->pending_trees);
--
2.11.0
