[PATCH] clean up for conntrack stats

[PATCH] clean up for conntrack stats

[Pablo Neira]

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

Hi Martin, Hi Patrick,

Attached a clean up for current conntrack stats, it applies to current 
bk 2.6 tree. Some comments:

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

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;

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

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.

Wait for your comments.

regards,
Pablo

[-- Attachment #2: acct-cleanup.patch --]
[-- Type: text/x-patch, Size: 5390 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	Tue Oct  5 16:03:41 2004
@@ -292,6 +292,7 @@
 
 extern unsigned int ip_conntrack_htable_size;
  
+#ifdef CONFIG_IP_NF_CT_ACCT
 struct ip_conntrack_stat
 {
 	unsigned int searched;
@@ -312,6 +313,9 @@
 };
 
 #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)                        \
===== 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	Tue Oct  5 16:03:41 2004
@@ -76,7 +76,9 @@
 struct ip_conntrack ip_conntrack_untracked;
 unsigned int ip_ct_log_invalid;
 
+#ifdef CONFIG_IP_NF_CT_ACCT
 DEFINE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
+#endif
 
 inline void 
 ip_conntrack_put(struct ip_conntrack *ct)
@@ -352,16 +354,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;
@@ -1074,18 +1074,21 @@
 	synchronize_net();
 }
 
+#ifdef CONFIG_IP_NF_CT_ACCT
 static inline void ct_add_counters(struct ip_conntrack *ct,
 				   enum ip_conntrack_info ctinfo,
 				   const struct sk_buff *skb)
 {
-#ifdef CONFIG_IP_NF_CT_ACCT
 	if (skb) {
 		ct->counters[CTINFO2DIR(ctinfo)].packets++;
 		ct->counters[CTINFO2DIR(ctinfo)].bytes += 
 					ntohs(skb->nh.iph->tot_len);
 	}
-#endif
 }
+#define CT_ADD_COUNTERS(ct, ctinfo, skb) ct_add_counters(ct, ctinfo, skb)
+#else
+#define CT_ADD_COUNTERS(ct, ctinfo, skb)
+#endif
 
 /* Refresh conntrack for this many jiffies and do accounting (if skb != NULL) */
 void ip_ct_refresh_acct(struct ip_conntrack *ct, 
@@ -1098,7 +1101,7 @@
 	/* If not in hash table, timer will not be active yet */
 	if (!is_confirmed(ct)) {
 		ct->timeout.expires = extra_jiffies;
-		ct_add_counters(ct, ctinfo, skb);
+		CT_ADD_COUNTERS(ct, ctinfo, skb);
 	} else {
 		WRITE_LOCK(&ip_conntrack_lock);
 		/* Need del_timer for race avoidance (may already be dying). */
@@ -1106,7 +1109,7 @@
 			ct->timeout.expires = jiffies + extra_jiffies;
 			add_timer(&ct->timeout);
 		}
-		ct_add_counters(ct, ctinfo, skb);
+		CT_ADD_COUNTERS(ct, ctinfo, skb);
 		WRITE_UNLOCK(&ip_conntrack_lock);
 	}
 }
===== 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	Tue Oct  5 16:12:04 2004
@@ -46,7 +46,9 @@
 MODULE_LICENSE("GPL");
 
 extern atomic_t ip_conntrack_count;
+#ifdef CONFIG_IP_NF_CT_ACCT
 DECLARE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
+#endif
 
 static int kill_proto(const struct ip_conntrack *i, void *data)
 {
@@ -264,6 +266,7 @@
 	.release = seq_release
 };
 
+#ifdef CONFIG_IP_NF_CT_ACCT
 static void *ct_cpu_seq_start(struct seq_file *seq, loff_t *pos)
 {
 	int cpu;
@@ -351,6 +354,7 @@
 	.llseek  = seq_lseek,
 	.release = seq_release_private,
 };
+#endif /* CONFIG_IP_NF_CT_ACCT */
 #endif
 
 static unsigned int ip_confirm(unsigned int hooknum,
@@ -725,7 +729,10 @@
 static int init_or_cleanup(int init)
 {
 #ifdef CONFIG_PROC_FS
-	struct proc_dir_entry *proc, *proc_exp, *proc_stat;
+	struct proc_dir_entry *proc, *proc_exp;
+#ifdef CONFIG_IP_NF_CT_ACCT
+	struct proc_dir_entry *proc_stat;
+#endif
 #endif
 	int ret = 0;
 
@@ -736,19 +743,19 @@
 		goto cleanup_nothing;
 
 #ifdef CONFIG_PROC_FS
-	proc = proc_net_fops_create("ip_conntrack", 0440, &ct_file_ops);
-	if (!proc) goto cleanup_init;
-
 	proc_exp = proc_net_fops_create("ip_conntrack_expect", 0440,
 					&exp_file_ops);
-	if (!proc_exp) goto cleanup_proc;
-
-	proc_stat = create_proc_entry("ip_conntrack", S_IRUGO, proc_net_stat);
-	if (!proc_stat)
-		goto cleanup_proc_exp;
+	if (!proc_exp) goto cleanup_init;
+	
+	proc = proc_net_fops_create("ip_conntrack", 0440, &ct_file_ops);
 
-	proc_stat->proc_fops = &ct_cpu_seq_fops;
-	proc_stat->owner = THIS_MODULE;
+	if (!proc) goto cleanup_proc_expect;
+	
+#ifdef CONFIG_IP_NF_CT_ACCT
+	proc_stat = proc_net_fops_create("ip_conntrack_stat", 0440, 
+					 &ct_cpu_seq_fops);
+	if (!proc_stat) goto cleanup_proc;
+#endif /* CONFIG_IP_NF_CT_ACCT */
 #endif
 
 	ret = nf_register_hook(&ip_conntrack_defrag_ops);
@@ -814,11 +821,13 @@
 	nf_unregister_hook(&ip_conntrack_defrag_ops);
  cleanup_proc_stat:
 #ifdef CONFIG_PROC_FS
+#ifdef CONFIG_IP_NF_CT_ACCT
 	proc_net_remove("ip_conntrack_stat");
-cleanup_proc_exp:
-	proc_net_remove("ip_conntrack_exp");
  cleanup_proc:
+#endif /* CONFIG_IP_NF_CT_ACCT */
 	proc_net_remove("ip_conntrack");
+ cleanup_proc_expect:
+	proc_net_remove("ip_conntrack_expect");
  cleanup_init:
 #endif /* CONFIG_PROC_FS */
 	ip_conntrack_cleanup();

Re: [PATCH] clean up for conntrack stats

[Patrick McHardy]

Hi Pablo.

Pablo Neira wrote:

> Hi Martin, Hi Patrick,
>
> Attached a clean up for current conntrack stats, it applies to current 
> bk 2.6 tree. Some comments:
>
> 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.

>
> 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.

> 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.

>
> 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.

Regards
Patrick

>
> Wait for your comments.
>
> regards,
> Pablo
>
>------------------------------------------------------------------------
>
>===== 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	Tue Oct  5 16:03:41 2004
>@@ -292,6 +292,7 @@
> 
> extern unsigned int ip_conntrack_htable_size;
>  
>+#ifdef CONFIG_IP_NF_CT_ACCT
> struct ip_conntrack_stat
> {
> 	unsigned int searched;
>@@ -312,6 +313,9 @@
> };
> 
> #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)                        \
>===== 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	Tue Oct  5 16:03:41 2004
>@@ -76,7 +76,9 @@
> struct ip_conntrack ip_conntrack_untracked;
> unsigned int ip_ct_log_invalid;
> 
>+#ifdef CONFIG_IP_NF_CT_ACCT
> DEFINE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
>+#endif
> 
> inline void 
> ip_conntrack_put(struct ip_conntrack *ct)
>@@ -352,16 +354,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;
>@@ -1074,18 +1074,21 @@
> 	synchronize_net();
> }
> 
>+#ifdef CONFIG_IP_NF_CT_ACCT
> static inline void ct_add_counters(struct ip_conntrack *ct,
> 				   enum ip_conntrack_info ctinfo,
> 				   const struct sk_buff *skb)
> {
>-#ifdef CONFIG_IP_NF_CT_ACCT
> 	if (skb) {
> 		ct->counters[CTINFO2DIR(ctinfo)].packets++;
> 		ct->counters[CTINFO2DIR(ctinfo)].bytes += 
> 					ntohs(skb->nh.iph->tot_len);
> 	}
>-#endif
> }
>+#define CT_ADD_COUNTERS(ct, ctinfo, skb) ct_add_counters(ct, ctinfo, skb)
>+#else
>+#define CT_ADD_COUNTERS(ct, ctinfo, skb)
>+#endif
> 
> /* Refresh conntrack for this many jiffies and do accounting (if skb != NULL) */
> void ip_ct_refresh_acct(struct ip_conntrack *ct, 
>@@ -1098,7 +1101,7 @@
> 	/* If not in hash table, timer will not be active yet */
> 	if (!is_confirmed(ct)) {
> 		ct->timeout.expires = extra_jiffies;
>-		ct_add_counters(ct, ctinfo, skb);
>+		CT_ADD_COUNTERS(ct, ctinfo, skb);
> 	} else {
> 		WRITE_LOCK(&ip_conntrack_lock);
> 		/* Need del_timer for race avoidance (may already be dying). */
>@@ -1106,7 +1109,7 @@
> 			ct->timeout.expires = jiffies + extra_jiffies;
> 			add_timer(&ct->timeout);
> 		}
>-		ct_add_counters(ct, ctinfo, skb);
>+		CT_ADD_COUNTERS(ct, ctinfo, skb);
> 		WRITE_UNLOCK(&ip_conntrack_lock);
> 	}
> }
>===== 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	Tue Oct  5 16:12:04 2004
>@@ -46,7 +46,9 @@
> MODULE_LICENSE("GPL");
> 
> extern atomic_t ip_conntrack_count;
>+#ifdef CONFIG_IP_NF_CT_ACCT
> DECLARE_PER_CPU(struct ip_conntrack_stat, ip_conntrack_stat);
>+#endif
> 
> static int kill_proto(const struct ip_conntrack *i, void *data)
> {
>@@ -264,6 +266,7 @@
> 	.release = seq_release
> };
> 
>+#ifdef CONFIG_IP_NF_CT_ACCT
> static void *ct_cpu_seq_start(struct seq_file *seq, loff_t *pos)
> {
> 	int cpu;
>@@ -351,6 +354,7 @@
> 	.llseek  = seq_lseek,
> 	.release = seq_release_private,
> };
>+#endif /* CONFIG_IP_NF_CT_ACCT */
> #endif
> 
> static unsigned int ip_confirm(unsigned int hooknum,
>@@ -725,7 +729,10 @@
> static int init_or_cleanup(int init)
> {
> #ifdef CONFIG_PROC_FS
>-	struct proc_dir_entry *proc, *proc_exp, *proc_stat;
>+	struct proc_dir_entry *proc, *proc_exp;
>+#ifdef CONFIG_IP_NF_CT_ACCT
>+	struct proc_dir_entry *proc_stat;
>+#endif
> #endif
> 	int ret = 0;
> 
>@@ -736,19 +743,19 @@
> 		goto cleanup_nothing;
> 
> #ifdef CONFIG_PROC_FS
>-	proc = proc_net_fops_create("ip_conntrack", 0440, &ct_file_ops);
>-	if (!proc) goto cleanup_init;
>-
> 	proc_exp = proc_net_fops_create("ip_conntrack_expect", 0440,
> 					&exp_file_ops);
>-	if (!proc_exp) goto cleanup_proc;
>-
>-	proc_stat = create_proc_entry("ip_conntrack", S_IRUGO, proc_net_stat);
>-	if (!proc_stat)
>-		goto cleanup_proc_exp;
>+	if (!proc_exp) goto cleanup_init;
>+	
>+	proc = proc_net_fops_create("ip_conntrack", 0440, &ct_file_ops);
> 
>-	proc_stat->proc_fops = &ct_cpu_seq_fops;
>-	proc_stat->owner = THIS_MODULE;
>+	if (!proc) goto cleanup_proc_expect;
>+	
>+#ifdef CONFIG_IP_NF_CT_ACCT
>+	proc_stat = proc_net_fops_create("ip_conntrack_stat", 0440, 
>+					 &ct_cpu_seq_fops);
>+	if (!proc_stat) goto cleanup_proc;
>+#endif /* CONFIG_IP_NF_CT_ACCT */
> #endif
> 
> 	ret = nf_register_hook(&ip_conntrack_defrag_ops);
>@@ -814,11 +821,13 @@
> 	nf_unregister_hook(&ip_conntrack_defrag_ops);
>  cleanup_proc_stat:
> #ifdef CONFIG_PROC_FS
>+#ifdef CONFIG_IP_NF_CT_ACCT
> 	proc_net_remove("ip_conntrack_stat");
>-cleanup_proc_exp:
>-	proc_net_remove("ip_conntrack_exp");
>  cleanup_proc:
>+#endif /* CONFIG_IP_NF_CT_ACCT */
> 	proc_net_remove("ip_conntrack");
>+ cleanup_proc_expect:
>+	proc_net_remove("ip_conntrack_expect");
>  cleanup_init:
> #endif /* CONFIG_PROC_FS */
> 	ip_conntrack_cleanup();
>  
>

Re: [PATCH] clean up for conntrack stats

[Pablo Neira]

[-- 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:

Re: [PATCH] clean up for conntrack stats

[Patrick McHardy]

Pablo Neira wrote:

> Patrick McHardy wrote:
>
>>> 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.

Applied, thanks. smp_processor_id doesn't involve atomic
operations (at least on x86), so I don't expect it harms performance
noticeable.

>>
>> 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 :-).


If we make it configurable it should use a different config
option. CONFIG_IP_NF_CT_ACCT is per-conntrack accounting,
conntrack stats is something quite different. I'm still not
convinced it is worth adding yet another config option,
considering it doesn't cost much, and it provides some useful
debugging information for people complaining about long delays
(lots of expectations) and such.

> 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.

Thanks alot, also applied. But please do two things next time
you submit patches:

- Sign off your patches. I've signed your name this time,
if you don't protest I'm going to submit them tomorrow.

- Please send each patch in a seperate mail. This makes
it easier to review and reply.

Thanks.

Regards,
Patrick