netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht
@ 2016-07-30 11:42 Liping Zhang
  2016-08-12 10:34 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 5+ messages in thread
From: Liping Zhang @ 2016-07-30 11:42 UTC (permalink / raw)
  To: pablo; +Cc: fw, netfilter-devel, Liping Zhang

From: Liping Zhang <liping.zhang@spreadtrum.com>

Since Commit 64b87639c9cb ("netfilter: conntrack: fix race between
nf_conntrack proc read and hash resize") introdue the
nf_conntrack_get_ht, so there's no need to check nf_conntrack_generation
again and again to get the hash table and hash size.

But keep ____nf_conntrack_find unchanged, because it is performance
critical path, increase the overhead of the function call is not so
good.

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 V2: remove "inline void" declared to nf_conntrack_get_ht, and keep
     ____nf_conntrack_find unchanged suggested by Florian.

 net/netfilter/nf_conntrack_core.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 2d46225..23c6db0 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -801,18 +801,15 @@ nf_conntrack_tuple_taken(const struct nf_conntrack_tuple *tuple,
 	const struct nf_conntrack_zone *zone;
 	struct nf_conntrack_tuple_hash *h;
 	struct hlist_nulls_head *ct_hash;
-	unsigned int hash, sequence;
+	unsigned int hash, hsize;
 	struct hlist_nulls_node *n;
 	struct nf_conn *ct;
 
 	zone = nf_ct_zone(ignored_conntrack);
 
 	rcu_read_lock();
-	do {
-		sequence = read_seqcount_begin(&nf_conntrack_generation);
-		hash = hash_conntrack(net, tuple);
-		ct_hash = nf_conntrack_hash;
-	} while (read_seqcount_retry(&nf_conntrack_generation, sequence));
+	nf_conntrack_get_ht(&ct_hash, &hsize);
+	hash = __hash_conntrack(net, tuple, hsize);
 
 	hlist_nulls_for_each_entry_rcu(h, n, &ct_hash[hash], hnnode) {
 		ct = nf_ct_tuplehash_to_ctrack(h);
@@ -878,14 +875,11 @@ static noinline int early_drop(struct net *net, unsigned int _hash)
 
 	for (i = 0; i < NF_CT_EVICTION_RANGE; i++) {
 		struct hlist_nulls_head *ct_hash;
-		unsigned hash, sequence, drops;
+		unsigned int hash, hsize, drops;
 
 		rcu_read_lock();
-		do {
-			sequence = read_seqcount_begin(&nf_conntrack_generation);
-			hash = scale_hash(_hash++);
-			ct_hash = nf_conntrack_hash;
-		} while (read_seqcount_retry(&nf_conntrack_generation, sequence));
+		nf_conntrack_get_ht(&ct_hash, &hsize);
+		hash = reciprocal_scale(_hash++, hsize);
 
 		drops = early_drop_list(net, &ct_hash[hash]);
 		rcu_read_unlock();
-- 
2.5.5



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht
  2016-07-30 11:42 [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang
@ 2016-08-12 10:34 ` Pablo Neira Ayuso
  2016-08-12 11:12   ` Liping Zhang
  0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-12 10:34 UTC (permalink / raw)
  To: Liping Zhang; +Cc: fw, netfilter-devel, Liping Zhang

On Sat, Jul 30, 2016 at 07:42:53PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Since Commit 64b87639c9cb ("netfilter: conntrack: fix race between
> nf_conntrack proc read and hash resize") introdue the
> nf_conntrack_get_ht, so there's no need to check nf_conntrack_generation
> again and again to get the hash table and hash size.
> 
> But keep ____nf_conntrack_find unchanged, because it is performance
> critical path, increase the overhead of the function call is not so
> good.

I'm not very happy with this solution.

I think it is a good time to kill compat /proc/net/ip_conntrack*. That
has been there for so long already. So we can inline this function,
this is the only one that needs it to export it, right?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht
  2016-08-12 10:34 ` Pablo Neira Ayuso
@ 2016-08-12 11:12   ` Liping Zhang
  2016-08-12 11:49     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 5+ messages in thread
From: Liping Zhang @ 2016-08-12 11:12 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Liping Zhang, Florian Westphal, netfilter-devel, Liping Zhang

Hi pablo,

2016-08-12 18:34 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Sat, Jul 30, 2016 at 07:42:53PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <liping.zhang@spreadtrum.com>
>>
>> Since Commit 64b87639c9cb ("netfilter: conntrack: fix race between
>> nf_conntrack proc read and hash resize") introdue the
>> nf_conntrack_get_ht, so there's no need to check nf_conntrack_generation
>> again and again to get the hash table and hash size.
>>
>> But keep ____nf_conntrack_find unchanged, because it is performance
>> critical path, increase the overhead of the function call is not so
>> good.
>
> I'm not very happy with this solution.

Yes.
>
> I think it is a good time to kill compat /proc/net/ip_conntrack*. That
> has been there for so long already. So we can inline this function,
> this is the only one that needs it to export it, right?

If just for the purpose of using nf_conntrack_get_ht to simply the source code,
I'm not sure is it worth to delete the compat /proc/net/ip_conntrack*?

So I'm inclined to keep the original source codes unchanged :)

Thanks

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht
  2016-08-12 11:12   ` Liping Zhang
@ 2016-08-12 11:49     ` Pablo Neira Ayuso
  2016-08-12 13:55       ` Liping Zhang
  0 siblings, 1 reply; 5+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-12 11:49 UTC (permalink / raw)
  To: Liping Zhang
  Cc: Liping Zhang, Florian Westphal, netfilter-devel, Liping Zhang

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Fri, Aug 12, 2016 at 07:12:32PM +0800, Liping Zhang wrote:
> 2016-08-12 18:34 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
> >
> > I think it is a good time to kill compat /proc/net/ip_conntrack*. That
> > has been there for so long already. So we can inline this function,
> > this is the only one that needs it to export it, right?
> 
> If just for the purpose of using nf_conntrack_get_ht to simply the source code,
> I'm not sure is it worth to delete the compat /proc/net/ip_conntrack*?
> 
> So I'm inclined to keep the original source codes unchanged :)

Just sent a patch to kill that compat code. It is also missing new
supported layer 4 protocols, as well as IPv6. We have too many
interfaces already, actually I'd be happy to kill nf_conntrack sysctl
entries at some point and leave just the ctnetlink interface.

I'm attaching an incomplete patch that moves nf_conntrack_get_ht() as
inline. It applies on top of:

        http://patchwork.ozlabs.org/patch/658620/

Feel free to take it over and finish it. Thanks.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 3411 bytes --]

commit 4e3a8f9347923d39392660c150068e1b8f937dfe
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Fri Aug 12 13:20:56 2016 +0200

    x
    
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 445b019..a817575 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -303,9 +303,29 @@ struct kernel_param;
 
 int nf_conntrack_set_hashsize(const char *val, struct kernel_param *kp);
 int nf_conntrack_hash_resize(unsigned int hashsize);
+
+extern struct hlist_nulls_head *nf_conntrack_hash;
 extern unsigned int nf_conntrack_htable_size;
+extern seqcount_t nf_conntrack_generation;
 extern unsigned int nf_conntrack_max;
 
+/* must be called with rcu read lock held. */
+static inline void nf_conntrack_get_ht(struct hlist_nulls_head **hash,
+                                       unsigned int *hsize)
+{
+	struct hlist_nulls_head *hptr;
+	unsigned int sequence, hsz;
+
+	do {
+		sequence = read_seqcount_begin(&nf_conntrack_generation);
+		hsz = nf_conntrack_htable_size;
+		hptr = nf_conntrack_hash;
+	} while (read_seqcount_retry(&nf_conntrack_generation, sequence));
+
+	*hash = hptr;
+	*hsize = hsz;
+}
+
 struct nf_conn *nf_ct_tmpl_alloc(struct net *net,
 				 const struct nf_conntrack_zone *zone,
 				 gfp_t flags);
diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index 79d7ac5..23db857 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -83,7 +83,6 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
 
 #define CONNTRACK_LOCKS 1024
 
-extern struct hlist_nulls_head *nf_conntrack_hash;
 extern spinlock_t nf_conntrack_locks[CONNTRACK_LOCKS];
 void nf_conntrack_lock(spinlock_t *lock);
 
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 22558b7..497d037 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -74,7 +74,6 @@ EXPORT_SYMBOL_GPL(nf_conntrack_hash);
 
 static __read_mostly struct kmem_cache *nf_conntrack_cachep;
 static __read_mostly spinlock_t nf_conntrack_locks_all_lock;
-static __read_mostly seqcount_t nf_conntrack_generation;
 static __read_mostly DEFINE_SPINLOCK(nf_conntrack_locks_all_lock);
 static __read_mostly bool nf_conntrack_locks_all;
 
@@ -162,6 +161,7 @@ static void nf_conntrack_all_unlock(void)
 
 unsigned int nf_conntrack_htable_size __read_mostly;
 unsigned int nf_conntrack_max __read_mostly;
+seqcount_t nf_conntrack_generation __read_mostly;
 
 DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
 EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
@@ -478,23 +478,6 @@ nf_ct_key_equal(struct nf_conntrack_tuple_hash *h,
 	       net_eq(net, nf_ct_net(ct));
 }
 
-/* must be called with rcu read lock held */
-void nf_conntrack_get_ht(struct hlist_nulls_head **hash, unsigned int *hsize)
-{
-	struct hlist_nulls_head *hptr;
-	unsigned int sequence, hsz;
-
-	do {
-		sequence = read_seqcount_begin(&nf_conntrack_generation);
-		hsz = nf_conntrack_htable_size;
-		hptr = nf_conntrack_hash;
-	} while (read_seqcount_retry(&nf_conntrack_generation, sequence));
-
-	*hash = hptr;
-	*hsize = hsz;
-}
-EXPORT_SYMBOL_GPL(nf_conntrack_get_ht);
-
 /*
  * Warning :
  * - Caller must take a reference on returned object

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht
  2016-08-12 11:49     ` Pablo Neira Ayuso
@ 2016-08-12 13:55       ` Liping Zhang
  0 siblings, 0 replies; 5+ messages in thread
From: Liping Zhang @ 2016-08-12 13:55 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Liping Zhang, Florian Westphal, netfilter-devel, Liping Zhang

2016-08-12 19:49 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Fri, Aug 12, 2016 at 07:12:32PM +0800, Liping Zhang wrote:
>> 2016-08-12 18:34 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> [...]
>> >
>> > I think it is a good time to kill compat /proc/net/ip_conntrack*. That
>> > has been there for so long already. So we can inline this function,
>> > this is the only one that needs it to export it, right?
>>
>> If just for the purpose of using nf_conntrack_get_ht to simply the source code,
>> I'm not sure is it worth to delete the compat /proc/net/ip_conntrack*?
>>
>> So I'm inclined to keep the original source codes unchanged :)
>
> Just sent a patch to kill that compat code. It is also missing new
> supported layer 4 protocols, as well as IPv6. We have too many
> interfaces already, actually I'd be happy to kill nf_conntrack sysctl
> entries at some point and leave just the ctnetlink interface.
>
> I'm attaching an incomplete patch that moves nf_conntrack_get_ht() as
> inline. It applies on top of:
>
>         http://patchwork.ozlabs.org/patch/658620/
>
> Feel free to take it over and finish it. Thanks.

OK. Will be happy to follow up on this:)

Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-08-12 13:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-30 11:42 [PATCH nf-next V2] netfilter: conntrack: simplify the code by using nf_conntrack_get_ht Liping Zhang
2016-08-12 10:34 ` Pablo Neira Ayuso
2016-08-12 11:12   ` Liping Zhang
2016-08-12 11:49     ` Pablo Neira Ayuso
2016-08-12 13:55       ` Liping Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).