[Devel,rh7,2/4] net: Skip allocation of conntrack if there are no rules

Submitted by Kirill Tkhai on Sept. 6, 2016, 6:31 a.m.

Details

Message ID 147314349360.331.1549732089017435462.stgit@pro
State New
Series "Create conntrack structures only if they are really needed"
Headers show

Commit Message

Kirill Tkhai Sept. 6, 2016, 6:31 a.m.
HW node may have many CTs, which are not having conntrack
users. Do not allocate them to save resources and decrease
performance penalty.

Next two patches will actually mark conntrack users.

https://jira.sw.ru/browse/PSBM-51050

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 net/netfilter/nf_conntrack_core.c    |    7 ++++++-
 net/netfilter/nf_conntrack_netlink.c |    2 ++
 net/netfilter/nf_synproxy_core.c     |    2 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index b5de5aa..c46bede 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -841,6 +841,11 @@  __nf_conntrack_alloc(struct net *net, u16 zone,
 	unsigned int ct_max = net->ct.max ? net->ct.max : init_net.ct.max;
 	struct nf_conn *ct;
 
+	if (!atomic_read(&net->ct.users)) {
+		/* No rules loaded */
+		return NULL;
+	}
+
 	if (unlikely(!nf_conntrack_hash_rnd)) {
 		init_nf_conntrack_hash_rnd();
 		/* recompute the hash as nf_conntrack_hash_rnd is initialized */
@@ -963,7 +968,7 @@  init_conntrack(struct net *net, struct nf_conn *tmpl,
 
 	ct = __nf_conntrack_alloc(net, zone, tuple, &repl_tuple, GFP_ATOMIC,
 				  hash);
-	if (IS_ERR(ct))
+	if (IS_ERR_OR_NULL(ct))
 		return (struct nf_conntrack_tuple_hash *)ct;
 
 	if (tmpl && nfct_synproxy(tmpl)) {
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index d6b6465..87f5091 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1617,7 +1617,9 @@  ctnetlink_create_conntrack(struct net *net, u16 zone,
 	struct nf_conntrack_helper *helper;
 	struct nf_conn_tstamp *tstamp;
 
+	inc_conntrack_users(net);
 	ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC);
+	dec_conntrack_users(net);
 	if (IS_ERR(ct))
 		return ERR_PTR(-ENOMEM);
 
diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 52e20c9..7bef49e 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -353,7 +353,9 @@  static int __net_init synproxy_net_init(struct net *net)
 	int err = -ENOMEM;
 
 	memset(&t, 0, sizeof(t));
+	inc_conntrack_users(net);
 	ct = nf_conntrack_alloc(net, 0, &t, &t, GFP_KERNEL);
+	dec_conntrack_users(net);
 	if (IS_ERR(ct)) {
 		err = PTR_ERR(ct);
 		goto err1;

Comments

Andrey Vagin Sept. 8, 2016, 5:31 a.m.
On Tue, Sep 06, 2016 at 09:31:33AM +0300, Kirill Tkhai wrote:
> HW node may have many CTs, which are not having conntrack
> users. Do not allocate them to save resources and decrease
> performance penalty.
> 
> Next two patches will actually mark conntrack users.
> 
> https://jira.sw.ru/browse/PSBM-51050
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  net/netfilter/nf_conntrack_core.c    |    7 ++++++-
>  net/netfilter/nf_conntrack_netlink.c |    2 ++
>  net/netfilter/nf_synproxy_core.c     |    2 ++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index b5de5aa..c46bede 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -841,6 +841,11 @@ __nf_conntrack_alloc(struct net *net, u16 zone,
>  	unsigned int ct_max = net->ct.max ? net->ct.max : init_net.ct.max;
>  	struct nf_conn *ct;
>  
> +	if (!atomic_read(&net->ct.users)) {
> +		/* No rules loaded */
> +		return NULL;
> +	}
> +
>  	if (unlikely(!nf_conntrack_hash_rnd)) {
>  		init_nf_conntrack_hash_rnd();
>  		/* recompute the hash as nf_conntrack_hash_rnd is initialized */
> @@ -963,7 +968,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
>  
>  	ct = __nf_conntrack_alloc(net, zone, tuple, &repl_tuple, GFP_ATOMIC,
>  				  hash);
> -	if (IS_ERR(ct))
> +	if (IS_ERR_OR_NULL(ct))
>  		return (struct nf_conntrack_tuple_hash *)ct;
>  
>  	if (tmpl && nfct_synproxy(tmpl)) {
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index d6b6465..87f5091 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1617,7 +1617,9 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
>  	struct nf_conntrack_helper *helper;
>  	struct nf_conn_tstamp *tstamp;
>  
> +	inc_conntrack_users(net);
>  	ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC);
> +	dec_conntrack_users(net);

When I look at this, I'm starting thinking that we have to start
tracking connections when someone tries to use them, but we should not
stop tracking connections.

We should not forget about the fact that conntrack can be used from
user-space http://conntrack-tools.netfilter.org/manual.html

>  	if (IS_ERR(ct))
>  		return ERR_PTR(-ENOMEM);
>  
> diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
> index 52e20c9..7bef49e 100644
> --- a/net/netfilter/nf_synproxy_core.c
> +++ b/net/netfilter/nf_synproxy_core.c
> @@ -353,7 +353,9 @@ static int __net_init synproxy_net_init(struct net *net)
>  	int err = -ENOMEM;
>  
>  	memset(&t, 0, sizeof(t));
> +	inc_conntrack_users(net);
>  	ct = nf_conntrack_alloc(net, 0, &t, &t, GFP_KERNEL);
> +	dec_conntrack_users(net);
>  	if (IS_ERR(ct)) {
>  		err = PTR_ERR(ct);
>  		goto err1;
>
Kirill Tkhai Sept. 8, 2016, 8:52 a.m.
On 08.09.2016 08:31, Andrei Vagin wrote:
> On Tue, Sep 06, 2016 at 09:31:33AM +0300, Kirill Tkhai wrote:
>> HW node may have many CTs, which are not having conntrack
>> users. Do not allocate them to save resources and decrease
>> performance penalty.
>>
>> Next two patches will actually mark conntrack users.
>>
>> https://jira.sw.ru/browse/PSBM-51050
>>
>> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
>> ---
>>  net/netfilter/nf_conntrack_core.c    |    7 ++++++-
>>  net/netfilter/nf_conntrack_netlink.c |    2 ++
>>  net/netfilter/nf_synproxy_core.c     |    2 ++
>>  3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>> index b5de5aa..c46bede 100644
>> --- a/net/netfilter/nf_conntrack_core.c
>> +++ b/net/netfilter/nf_conntrack_core.c
>> @@ -841,6 +841,11 @@ __nf_conntrack_alloc(struct net *net, u16 zone,
>>  	unsigned int ct_max = net->ct.max ? net->ct.max : init_net.ct.max;
>>  	struct nf_conn *ct;
>>  
>> +	if (!atomic_read(&net->ct.users)) {
>> +		/* No rules loaded */
>> +		return NULL;
>> +	}
>> +
>>  	if (unlikely(!nf_conntrack_hash_rnd)) {
>>  		init_nf_conntrack_hash_rnd();
>>  		/* recompute the hash as nf_conntrack_hash_rnd is initialized */
>> @@ -963,7 +968,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
>>  
>>  	ct = __nf_conntrack_alloc(net, zone, tuple, &repl_tuple, GFP_ATOMIC,
>>  				  hash);
>> -	if (IS_ERR(ct))
>> +	if (IS_ERR_OR_NULL(ct))
>>  		return (struct nf_conntrack_tuple_hash *)ct;
>>  
>>  	if (tmpl && nfct_synproxy(tmpl)) {
>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>> index d6b6465..87f5091 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -1617,7 +1617,9 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
>>  	struct nf_conntrack_helper *helper;
>>  	struct nf_conn_tstamp *tstamp;
>>  
>> +	inc_conntrack_users(net);
>>  	ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC);
>> +	dec_conntrack_users(net);
> 
> When I look at this, I'm starting thinking that we have to start
> tracking connections when someone tries to use them, but we should not
> stop tracking connections.

I didn't completely understand you point. Is it related to the hunk above?
If so, we don't stop tracking the conntracks there.
 
> We should not forget about the fact that conntrack can be used from
> user-space http://conntrack-tools.netfilter.org/manual.html

The above hunk is about this, isn't it?
 
>>  	if (IS_ERR(ct))
>>  		return ERR_PTR(-ENOMEM);
>>  
>> diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
>> index 52e20c9..7bef49e 100644
>> --- a/net/netfilter/nf_synproxy_core.c
>> +++ b/net/netfilter/nf_synproxy_core.c
>> @@ -353,7 +353,9 @@ static int __net_init synproxy_net_init(struct net *net)
>>  	int err = -ENOMEM;
>>  
>>  	memset(&t, 0, sizeof(t));
>> +	inc_conntrack_users(net);
>>  	ct = nf_conntrack_alloc(net, 0, &t, &t, GFP_KERNEL);
>> +	dec_conntrack_users(net);
>>  	if (IS_ERR(ct)) {
>>  		err = PTR_ERR(ct);
>>  		goto err1;
>>