Hi Patrick,

Patrick McHardy wrote:

>> a) I combined ifdef's checkings + macro, so if conntrack stats are 
>> compiled in the kernel we increase the counters.
>>
>> #define CONNTRACK_STAT_INC(count) 
>> (__get_cpu_var(ip_conntrack_stat).count++)
>> +#else /* !CONNTRACK STATS */
>> +#define CONNTRACK_STAT_INC(count)
>> +#endif
>
>
> I don't think we should make these configurable, they don't
> cost much.


I talk about this below

>> b) Martin, I've killed this to make them use the macro, are you ok 
>> with this? I know that we call twice smp_proccesor_id now, but it's 
>> clean I think.
>>
>> -       /* use per_cpu() to avoid multiple calls to 
>> smp_processor_id() */
>> -       unsigned int cpu = smp_processor_id();
>>
>>        MUST_BE_READ_LOCKED(&ip_conntrack_lock);
>>        list_for_each_entry(h, &ip_conntrack_hash[hash], list) {
>>                if (conntrack_tuple_cmp(h, tuple, ignored_conntrack)) {
>> -                       per_cpu(ip_conntrack_stat, cpu).found++;
>> +                       CONNTRACK_STAT_INC(found);
>>                        return h;
>>
>
> Please send this part as a seperate patch.


attached to this email, conntrack_find-stats-cleanup.patch file.

>> c) This is similar to a)
>>
>> +#define CT_ADD_COUNTERS(ct, ctinfo, skb) ct_add_counters(ct, ctinfo, 
>> skb)
>> +#else
>> +#define CT_ADD_COUNTERS(ct, ctinfo, skb)
>> +#endif
>
>
>
> Why this change ? ct_add_counters is an inline function with an
> empty body when CONFIG_IP_NF_CT_ACCT is undefined.


True, it changes nothing. Packet counting vanishes if 
CONFIG_IP_NF_CT_ACCT is unset, to make the system consistent maybe we 
should make vanish normal stats counting as well. See 
ct-stat-macro-vanishes.patch file. If you don't like it anyway, drop it :-).

>> d) You can see that I've play around with ip_conntrack_standalone 
>> reordering the creation of /proc entries. I did it just to remove 
>> warnings about unused labels when unsetting conntrack stats.
>>
>> To conclude, this patch adds up to 8 ifdef's CONFIG_IP_NF_CT_ACCT 
>> checkings, I must confess that I don't love them, I'll think if 
>> there's a way to do cleaner.
>
>
>
> I thought this was a cleanup-patch. Adding ifdefs is
> usually not considered cleanup :) It costs alot more time
> to review patches with multiple (mostly) unrelated changes,
> please send stuff like this as multiple small patches in
> the future.


Thanks for the tip :-).

I've attached another patch, see fix-remove-ip_conntrack_expect.patch 
file, if conntrack stats are set, ip_conntrack is compiled as module and 
you unload it, ip_conntrack_expect entry in /proc stays there forever.

regards,
Pablo