Re: [PATCH] clean up for conntrack stats

[Pablo Neira <pablo@eurodev.net>]

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

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

[-- Attachment #2: conntrack_find-stats-cleanup.patch --]
[-- Type: text/x-patch, Size: 789 bytes --]

===== net/ipv4/netfilter/ip_conntrack_core.c 1.68 vs edited =====
--- 1.68/net/ipv4/netfilter/ip_conntrack_core.c	Wed Sep 29 05:34:27 2004
+++ edited/net/ipv4/netfilter/ip_conntrack_core.c	Wed Oct  6 23:53:17 2004
@@ -352,16 +352,14 @@
 {
 	struct ip_conntrack_tuple_hash *h;
 	unsigned int hash = hash_conntrack(tuple);
-	/* 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;
 		}
-		per_cpu(ip_conntrack_stat, cpu).searched++;
+		CONNTRACK_STAT_INC(searched);
 	}
 
 	return NULL;

[-- Attachment #3: ct-stat-macro-vanishes.patch --]
[-- Type: text/x-patch, Size: 564 bytes --]

===== include/linux/netfilter_ipv4/ip_conntrack.h 1.23 vs edited =====
--- 1.23/include/linux/netfilter_ipv4/ip_conntrack.h	Thu Sep 23 23:45:10 2004
+++ edited/include/linux/netfilter_ipv4/ip_conntrack.h	Wed Oct  6 23:57:04 2004
@@ -311,7 +311,11 @@
 	unsigned int expect_delete;
 };
 
+#ifdef CONFIG_IP_NF_CT_ACCT
 #define CONNTRACK_STAT_INC(count) (__get_cpu_var(ip_conntrack_stat).count++)
+#else /* !CONNTRACK STATS */
+#define CONNTRACK_STAT_INC(count)
+#endif 
 
 /* eg. PROVIDES_CONNTRACK(ftp); */
 #define PROVIDES_CONNTRACK(name)                        \

[-- Attachment #4: fix-remove-ip_conntrack_expect.patch --]
[-- Type: text/x-patch, Size: 523 bytes --]

===== net/ipv4/netfilter/ip_conntrack_standalone.c 1.50 vs edited =====
--- 1.50/net/ipv4/netfilter/ip_conntrack_standalone.c	Tue Sep 28 23:22:13 2004
+++ edited/net/ipv4/netfilter/ip_conntrack_standalone.c	Wed Oct  6 23:46:59 2004
@@ -815,8 +815,8 @@
  cleanup_proc_stat:
 #ifdef CONFIG_PROC_FS
 	proc_net_remove("ip_conntrack_stat");
-cleanup_proc_exp:
-	proc_net_remove("ip_conntrack_exp");
+ cleanup_proc_exp:
+	proc_net_remove("ip_conntrack_expect");
  cleanup_proc:
 	proc_net_remove("ip_conntrack");
  cleanup_init: