[PATCH 0/3] net/netfilter: minor refactoring of conntrack ecache

[PATCH 0/3] net/netfilter: minor refactoring of conntrack ecache

[Tony Zelenoff]

Just saw some issues that better to refactor and save a bit of CPU cycles while
looking around conntracks. So, here they are.

Patches based on net-next tree.

Tony Zelenoff (3):
  net/netfilter: whitespace removed
  net/netfilter: refactor notifier registration
  net/netfilter: refactor nf_ct_deliver_cached_events

 net/netfilter/nf_conntrack_ecache.c |   81 +++++++++++++++++------------------
 1 files changed, 39 insertions(+), 42 deletions(-)

[PATCH 1/3] net/netfilter: whitespace removed

[Tony Zelenoff]

Signed-off-by: Tony Zelenoff <antonz@parallels.com>
---
 net/netfilter/nf_conntrack_ecache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index 14af632..aa15977 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -70,7 +70,7 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct)
 			else
 				e->missed &= ~missed;
 			spin_unlock_bh(&ct->lock);
-		} 
+		}
 	}
 
 out_unlock:
-- 
1.7.1

Re: [PATCH 1/3] net/netfilter: whitespace removed

[Pablo Neira Ayuso]

On Wed, Feb 22, 2012 at 10:47:59AM +0400, Tony Zelenoff wrote:
> Signed-off-by: Tony Zelenoff <antonz@parallels.com>

Applied, thanks.

[PATCH 2/3] net/netfilter: refactor notifier registration

[Tony Zelenoff]

* ret variable initialization removed as useless
* Similar code strings concatenated and functions code
  flow became more plain

Signed-off-by: Tony Zelenoff <antonz@parallels.com>
---
 net/netfilter/nf_conntrack_ecache.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index aa15977..9b8e986 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -81,21 +81,18 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
 int nf_conntrack_register_notifier(struct net *net,
 				   struct nf_ct_event_notifier *new)
 {
-	int ret = 0;
+	int ret;
 	struct nf_ct_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
 	notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
 					   lockdep_is_held(&nf_ct_ecache_mutex));
-	if (notify != NULL) {
+	if (likely(!notify)) {
+		rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
+		ret = 0;
+	} else
 		ret = -EBUSY;
-		goto out_unlock;
-	}
-	rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
-	mutex_unlock(&nf_ct_ecache_mutex);
-	return ret;
 
-out_unlock:
 	mutex_unlock(&nf_ct_ecache_mutex);
 	return ret;
 }
@@ -118,21 +115,18 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
 int nf_ct_expect_register_notifier(struct net *net,
 				   struct nf_exp_event_notifier *new)
 {
-	int ret = 0;
+	int ret;
 	struct nf_exp_event_notifier *notify;
 
 	mutex_lock(&nf_ct_ecache_mutex);
 	notify = rcu_dereference_protected(net->ct.nf_expect_event_cb,
 					   lockdep_is_held(&nf_ct_ecache_mutex));
-	if (notify != NULL) {
+	if (likely(!notify)) {
+		rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
+		ret = 0;
+	} else
 		ret = -EBUSY;
-		goto out_unlock;
-	}
-	rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
-	mutex_unlock(&nf_ct_ecache_mutex);
-	return ret;
 
-out_unlock:
 	mutex_unlock(&nf_ct_ecache_mutex);
 	return ret;
 }
-- 
1.7.1

Re: [PATCH 2/3] net/netfilter: refactor notifier registration

[Eric Dumazet]

Le mercredi 22 février 2012 à 10:48 +0400, Tony Zelenoff a écrit :
> * ret variable initialization removed as useless
> * Similar code strings concatenated and functions code
>   flow became more plain
> 
> Signed-off-by: Tony Zelenoff <antonz@parallels.com>
> ---
>  net/netfilter/nf_conntrack_ecache.c |   26 ++++++++++----------------
>  1 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index aa15977..9b8e986 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,21 +81,18 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct net *net,
>  				   struct nf_ct_event_notifier *new)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct nf_ct_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
>  	notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
>  					   lockdep_is_held(&nf_ct_ecache_mutex));
> -	if (notify != NULL) {
> +	if (likely(!notify)) {
> +		rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
> +		ret = 0;
> +	} else
>  		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> -	rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
> -	mutex_unlock(&nf_ct_ecache_mutex);
> -	return ret;
>  
> -out_unlock:
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  	return ret;
>  }
> @@ -118,21 +115,18 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>  int nf_ct_expect_register_notifier(struct net *net,
>  				   struct nf_exp_event_notifier *new)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct nf_exp_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
>  	notify = rcu_dereference_protected(net->ct.nf_expect_event_cb,
>  					   lockdep_is_held(&nf_ct_ecache_mutex));
> -	if (notify != NULL) {
> +	if (likely(!notify)) {
> +		rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
> +		ret = 0;
> +	} else
>  		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> -	rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
> -	mutex_unlock(&nf_ct_ecache_mutex);
> -	return ret;
>  
> -out_unlock:
>  	mutex_unlock(&nf_ct_ecache_mutex);
>  	return ret;
>  }

Please leave the code as is, I find it more readable.

It is standard coding practice, and permits stacking of new init code,
with proper error path.

Dont add likely()/unlikely() clauses in slow path, this obfuscate code
for litle gain.




--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] net/netfilter: refactor notifier registration

[Tony Zelenoff]

On 22.02.2012 11:54, Eric Dumazet wrote:
> Le mercredi 22 février 2012 à 10:48 +0400, Tony Zelenoff a écrit :
>> * ret variable initialization removed as useless
>> * Similar code strings concatenated and functions code
>>    flow became more plain
>>
>> Signed-off-by: Tony Zelenoff<antonz@parallels.com>
>> ---
>>   net/netfilter/nf_conntrack_ecache.c |   26 ++++++++++----------------
>>   1 files changed, 10 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
>> index aa15977..9b8e986 100644
>> --- a/net/netfilter/nf_conntrack_ecache.c
>> +++ b/net/netfilter/nf_conntrack_ecache.c
>> @@ -81,21 +81,18 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>>   int nf_conntrack_register_notifier(struct net *net,
>>   				   struct nf_ct_event_notifier *new)
>>   {
>> -	int ret = 0;
>> +	int ret;
>>   	struct nf_ct_event_notifier *notify;
>>
>>   	mutex_lock(&nf_ct_ecache_mutex);
>>   	notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
>>   					   lockdep_is_held(&nf_ct_ecache_mutex));
>> -	if (notify != NULL) {
>> +	if (likely(!notify)) {
>> +		rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
>> +		ret = 0;
>> +	} else
>>   		ret = -EBUSY;
>> -		goto out_unlock;
>> -	}
>> -	rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
>> -	mutex_unlock(&nf_ct_ecache_mutex);
>> -	return ret;
>>
>> -out_unlock:
>>   	mutex_unlock(&nf_ct_ecache_mutex);
>>   	return ret;
>>   }
>> @@ -118,21 +115,18 @@ EXPORT_SYMBOL_GPL(nf_conntrack_unregister_notifier);
>>   int nf_ct_expect_register_notifier(struct net *net,
>>   				   struct nf_exp_event_notifier *new)
>>   {
>> -	int ret = 0;
>> +	int ret;
>>   	struct nf_exp_event_notifier *notify;
>>
>>   	mutex_lock(&nf_ct_ecache_mutex);
>>   	notify = rcu_dereference_protected(net->ct.nf_expect_event_cb,
>>   					   lockdep_is_held(&nf_ct_ecache_mutex));
>> -	if (notify != NULL) {
>> +	if (likely(!notify)) {
>> +		rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
>> +		ret = 0;
>> +	} else
>>   		ret = -EBUSY;
>> -		goto out_unlock;
>> -	}
>> -	rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
>> -	mutex_unlock(&nf_ct_ecache_mutex);
>> -	return ret;
>>
>> -out_unlock:
>>   	mutex_unlock(&nf_ct_ecache_mutex);
>>   	return ret;
>>   }
>
> Please leave the code as is, I find it more readable.
>
> It is standard coding practice, and permits stacking of new init code,
> with proper error path.

Do not agree a bit. Of course, the code stacking and so on is good, but 
there is no reason to write:

	rcu_assign_pointer(net->ct.nf_expect_event_cb, new);
	mutex_unlock(&nf_ct_ecache_mutex);
	return ret;

out_unlock:
    	mutex_unlock(&nf_ct_ecache_mutex);
    	return ret;

if you can do it (without breaking logic and stacking ability and as it 
done everywhere) this way:

	rcu_assign_pointer(net->ct.nf_expect_event_cb, new);

out_unlock:
    	mutex_unlock(&nf_ct_ecache_mutex);
    	return ret;

with only one exit with proper locks freeing or deinitialization.

Ok, after that i've remove ret initialization at start and without goto 
this label became unused and compiler warn about it. Thus it was removed.

>
> Dont add likely()/unlikely() clauses in slow path, this obfuscate code
> for litle gain.
ok
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] net/netfilter: refactor notifier registration

[Pablo Neira Ayuso]

On Wed, Feb 22, 2012 at 10:48:00AM +0400, Tony Zelenoff wrote:
> * ret variable initialization removed as useless
> * Similar code strings concatenated and functions code
>   flow became more plain
> 
> Signed-off-by: Tony Zelenoff <antonz@parallels.com>
> ---
>  net/netfilter/nf_conntrack_ecache.c |   26 ++++++++++----------------
>  1 files changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
> index aa15977..9b8e986 100644
> --- a/net/netfilter/nf_conntrack_ecache.c
> +++ b/net/netfilter/nf_conntrack_ecache.c
> @@ -81,21 +81,18 @@ EXPORT_SYMBOL_GPL(nf_ct_deliver_cached_events);
>  int nf_conntrack_register_notifier(struct net *net,
>  				   struct nf_ct_event_notifier *new)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct nf_ct_event_notifier *notify;
>  
>  	mutex_lock(&nf_ct_ecache_mutex);
>  	notify = rcu_dereference_protected(net->ct.nf_conntrack_event_cb,
>  					   lockdep_is_held(&nf_ct_ecache_mutex));
> -	if (notify != NULL) {
> +	if (likely(!notify)) {
> +		rcu_assign_pointer(net->ct.nf_conntrack_event_cb, new);
> +		ret = 0;

I agree with Eric here.

1) Code readability is worst after this change.
2) More important, event notifier registration is not in the hot path,
so this likely is not worth to have.

Sorry, I won't take this patch.

[PATCH 3/3] net/netfilter: refactor nf_ct_deliver_cached_events

[Tony Zelenoff]

* identation lowered
* some CPU cycles saved at delayed item variable initialization

Signed-off-by: Tony Zelenoff <antonz@parallels.com>
---
 net/netfilter/nf_conntrack_ecache.c |   55 ++++++++++++++++++----------------
 1 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c
index 9b8e986..577a0e8 100644
--- a/net/netfilter/nf_conntrack_ecache.c
+++ b/net/netfilter/nf_conntrack_ecache.c
@@ -32,9 +32,11 @@ static DEFINE_MUTEX(nf_ct_ecache_mutex);
 void nf_ct_deliver_cached_events(struct nf_conn *ct)
 {
 	struct net *net = nf_ct_net(ct);
-	unsigned long events;
+	unsigned long events, missed;
 	struct nf_ct_event_notifier *notify;
 	struct nf_conntrack_ecache *e;
+	struct nf_ct_event item;
+	int ret;
 
 	rcu_read_lock();
 	notify = rcu_dereference(net->ct.nf_conntrack_event_cb);
@@ -47,31 +49,32 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct)
 
 	events = xchg(&e->cache, 0);
 
-	if (nf_ct_is_confirmed(ct) && !nf_ct_is_dying(ct) && events) {
-		struct nf_ct_event item = {
-			.ct	= ct,
-			.pid	= 0,
-			.report	= 0
-		};
-		int ret;
-		/* We make a copy of the missed event cache without taking
-		 * the lock, thus we may send missed events twice. However,
-		 * this does not harm and it happens very rarely. */
-		unsigned long missed = e->missed;
-
-		if (!((events | missed) & e->ctmask))
-			goto out_unlock;
-
-		ret = notify->fcn(events | missed, &item);
-		if (unlikely(ret < 0 || missed)) {
-			spin_lock_bh(&ct->lock);
-			if (ret < 0)
-				e->missed |= events;
-			else
-				e->missed &= ~missed;
-			spin_unlock_bh(&ct->lock);
-		}
-	}
+	if (!nf_ct_is_confirmed(ct) || nf_ct_is_dying(ct) || !events)
+		goto out_unlock;
+
+	/* We make a copy of the missed event cache without taking
+	 * the lock, thus we may send missed events twice. However,
+	 * this does not harm and it happens very rarely. */
+	missed = e->missed;
+
+	if (!((events | missed) & e->ctmask))
+		goto out_unlock;
+
+	item.ct = ct;
+	item.pid = 0;
+	item.report = 0;
+
+	ret = notify->fcn(events | missed, &item);
+
+	if (likely(ret >= 0 && !missed))
+		goto out_unlock;
+
+	spin_lock_bh(&ct->lock);
+	if (ret < 0)
+		e->missed |= events;
+	else
+		e->missed &= ~missed;
+	spin_unlock_bh(&ct->lock);
 
 out_unlock:
 	rcu_read_unlock();
-- 
1.7.1

Re: [PATCH 3/3] net/netfilter: refactor nf_ct_deliver_cached_events

[Pablo Neira Ayuso]

On Wed, Feb 22, 2012 at 10:48:01AM +0400, Tony Zelenoff wrote:
> * identation lowered
> * some CPU cycles saved at delayed item variable initialization
> 
> Signed-off-by: Tony Zelenoff <antonz@parallels.com>

Applied, thanks Tony.