Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> 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.
The gc is removed in your first patch.
> - NF_CONNCOUNT_SKIP, /* list is already reclaimed by gc */
I would keep this, and rename it to
NF_CONNCOUNT_LOCK, /* fall back to full locking */
> };
>
> 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;
> };
Ack, dead should be removed.
> 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 68788b979c1d..b33df77bd7a3 100644
> --- a/net/netfilter/nf_conncount.c
> +++ b/net/netfilter/nf_conncount.c
> @@ -103,11 +103,6 @@ nf_conncount_add(struct nf_conncount_list *list,
> conn->cpu = raw_smp_processor_id();
> conn->jiffies32 = (u32)jiffies;
> 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;
Make this return NF_CONNCOUNT_LOCK.
> @@ -310,13 +285,18 @@ static void tree_nodes_free(struct rb_root *root,
> unsigned int gc_count)
> {
> struct nf_conncount_rb *rbconn;
> + bool release = false;
>
> while (gc_count) {
> rbconn = gc_nodes[--gc_count];
> spin_lock(&rbconn->list.list_lock);
> + if (!rbconn->list.count) {
> + release = true;
> + rb_erase(&rbconn->node, root);
> + }
> spin_unlock(&rbconn->list.list_lock);
> + if (release)
> + call_rcu(&rbconn->rcu_head, __tree_nodes_free);
This looks good.
> @@ -360,11 +340,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;
I would alter the comment:
/* NF_CONNCOUNT_LOCK, rbconn might be on a gc list already,
* fall back to locked version.
*/
insert_tree() has a good chance of re-using the as is.
I also think its not worth to optimize that case in any way.
In normal operation tree node insert/removal should be rare
compared to changes to lists/rb_nodes.
