clean up for conntrack stats

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

------------------------------------------------------------------------

===== include/linux/netfilter_ipv4/ip_conntrack.h 1.23 vs edited =====

include/linux/netfilter_ipv4/ip_conntrack.h +1 -0

@@ -292,6 +292,7 @@
 
 extern unsigned int ip_conntrack_htable_size;
 
+#ifdef CONFIG_IP_NF_CT_ACCT

struct ip_conntrack_stat

{
	unsigned int searched;

include/linux/netfilter_ipv4/ip_conntrack.h +3 -0

@@ -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 =====

net/ipv4/netfilter/ip_conntrack_core.c +11 -8

@@ -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 =====

net/ipv4/netfilter/ip_conntrack_standalone.c +22 -13

@@ -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();

Review

Quoted 6 lines

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++)

(quoted diff) +3 -0

+#else /* !CONNTRACK STATS */
+#define CONNTRACK_STAT_INC(count)
+#endif

I don't think we should make these configurable, they don't cost much.

Review

Quoted 13 lines

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() */

(quoted diff) +1 -2

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

Review

Quoted 7 lines

c) This is similar to a)

+#define CT_ADD_COUNTERS(ct, ctinfo, skb) ct_add_counters(ct, ctinfo, skb)

(quoted diff) +3 -0

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

Quoted 3 lines

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.

Quoted 14 lines · Patrick McHardy

Quoted 13 lines

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() */

net/ipv4/netfilter/ip_conntrack_core.c +1 -2

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

Quoted 1 line

Please send this part as a seperate patch.

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

Quoted 3 lines

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

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.