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

[Tony Zelenoff <antonz@parallels.com>]

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