netfilter: nat: revert rhashtable conversion
cover netfilter: nat: revert rhashtable conversion
These two patches get rid of rhashtable the conversion.
First patch is an implicit dependency, rhlist remove interface is a no-op in case element is not yet in the table, but hlist_del_rcu() must only be called for conntracks that already have been inserted.
Second patch is the actual revert.
PATCH 1/2 netfilter: nat: avoid use of nf_conn_nat extension
commit 6e699867f84c0f358fed233fe6162173aca28e04 upstream.
successful insert into the bysource hash sets IPS_SRC_NAT_DONE status bit so we can check that instead of presence of nat extension which requires extra deref.
--- net/netfilter/nf_conntrack_core.c | 2 +- net/netfilter/nf_nat_core.c | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-)
net/netfilter/nf_conntrack_core.c +1 -1
@@ -689,7 +689,7 @@ static int nf_ct_resolve_clash(struct net *net, struct sk_buff *skb, l4proto =__nf_ct_l4proto_find (nf_ct_l3num (ct),nf_ct_protonum (ct));if (l4proto->allow_clash && - !nfct_nat (ct) && + ((ct->status &IPS_NAT_DONE_MASK ) ==0 ) && !nf_ct_is_dying (ct) &&atomic_inc_not_zero (&ct->ct_general.use)) {nf_ct_acct_merge (ct, ctinfo, (struct nf_conn *)skb->nfct);
net/netfilter/nf_nat_core.c +4 -14
@@ -550,10 +550,6 @@ struct nf_nat_proto_clean {static int nf_nat_proto_remove (struct nf_conn *i,void *data) {const struct nf_nat_proto_clean *clean = data; -struct nf_conn_nat *nat =nfct_nat (i); - -if (!nat) -return 0 ;if ((clean->l3proto &&nf_ct_l3num (i) != clean->l3proto) || (clean->l4proto &&nf_ct_protonum (i) != clean->l4proto))
@@ -564,12 +560,10 @@ static int nf_nat_proto_remove(struct nf_conn *i, void *data)static int nf_nat_proto_clean (struct nf_conn *ct,void *data) { -struct nf_conn_nat *nat =nfct_nat (ct); -if (nf_nat_proto_remove (ct, data))return 1 ; -if (!nat) +if ((ct->status &IPS_SRC_NAT_DONE ) ==0 )return 0 ;/* This netns is being destroyed, and conntrack has nat null binding.
@@ -705,13 +699,9 @@ EXPORT_SYMBOL_GPL(nf_nat_l3proto_unregister);/* No one using conntrack by the time this called. */ static void nf_nat_cleanup_conntrack (struct nf_conn *ct) { -struct nf_conn_nat *nat =nf_ct_ext_find (ct,NF_CT_EXT_NAT ); - -if (!nat) -return ; - -rhltable_remove (&nf_nat_bysource_table, &ct->nat_bysource, - nf_nat_bysource_params); +if (ct->status &IPS_SRC_NAT_DONE ) +rhltable_remove (&nf_nat_bysource_table, &ct->nat_bysource, + nf_nat_bysource_params); }static struct nf_ct_ext_type nat_extend__read_mostly = {
Signature · 1 line
2.13.6
PATCH 2/2 netfilter: nat: Revert "netfilter: nat: convert nat bysrc hash to rhashtable"
In reply to Florian Westphalcommit e1bf1687740ce1a3598a1c5e452b852ff2190682 upstream.
This reverts commit 870190a9ec9075205c0fa795a09fa931694a3ff1.
It was not a good idea. The custom hash table was a much better fit for this purpose.
A fast lookup is not essential, in fact for most cases there is no lookup at all because original tuple is not taken and can be used as-is. What needs to be fast is insertion and deletion.
rhlist removal however requires a rhlist walk.
We can have thousands of entries in such a list if source port/addresses are reused for multiple flows, if this happens removal requests are so expensive that deletions of a few thousand flows can take several seconds(!).
The advantages that we got from rhashtable are:
1) table auto-sizing
2) multiple locks
1) would be nice to have, but it is not essential as we have at most one lookup per new flow, so even a million flows in the bysource table are not a problem compared to current deletion cost.
2) is easy to add to custom hash table.
I tried to add hlist_node to rhlist to speed up rhltable_remove but this isn't doable without changing semantics. rhltable_remove_fast will check that the to-be-deleted object is part of the table and that requires a list walk that we want to avoid.
Furthermore, using hlist_node increases size of struct rhlist_head, which in turn increases nf_conn size.
--- include/net/netfilter/nf_conntrack.h | 3 +- include/net/netfilter/nf_nat.h | 1 - net/netfilter/nf_nat_core.c | 134 +++++++++++++++-------------------- 3 files changed, 57 insertions(+), 81 deletions(-)
include/net/netfilter/nf_conntrack.h +1 -2
@@ -17,7 +17,6 @@#include <linux/bitops.h> #include <linux/compiler.h> #include <linux/atomic.h> -#include <linux/rhashtable.h> #include <linux/netfilter/nf_conntrack_tcp.h> #include <linux/netfilter/nf_conntrack_dccp.h>
@@ -101,7 +100,7 @@ struct nf_conn {possible_net_t ct_net;#if IS_ENABLED (CONFIG_NF_NAT ) -struct rhlist_head nat_bysource; +struct hlist_node nat_bysource;#endif /* all members below initialized via memset */ u8 __nfct_init_offset[0 ];
include/net/netfilter/nf_nat.h +0 -1
@@ -1,6 +1,5 @@#ifndef _NF_NAT_H#define _NF_NAT_H -#include <linux/rhashtable.h> #include <linux/netfilter_ipv4.h> #include <linux/netfilter/nf_nat.h> #include <net/netfilter/nf_conntrack_tuple.h>
net/netfilter/nf_nat_core.c +56 -78
217 lines not on this page · raw message
Signature · 1 line
2.13.6
Quoted 8 lines · Florian Westphal
These two patches get rid of rhashtable the conversion.
First patch is an implicit dependency, rhlist remove interface is a no-op in case element is not yet in the table, but
hlist_del_rcu()must only be called for conntracks that already have been inserted.Second patch is the actual revert.
This version is very close to what we're running here. Thanks for your work Florian.
Quoted 7 lines · Florian Westphal
These two patches get rid of rhashtable the conversion.
First patch is an implicit dependency, rhlist remove interface is a no-op in case element is not yet in the table, but
hlist_del_rcu()must only be called for conntracks that already have been inserted.Second patch is the actual revert.
Thanks for the patches, now queued up.
greg k-h