expectation use a slab instead of kmalloc

PATCH Applied Pablo Neira 2 participants 4 messages May 31, 2004 – Jun 2, 2004

Subsystems (2)

Networking netfilter

lore

Share & tools

lore

Pablo Neira

author

2004-05-31 13:56 UTC #0 lore raw

PATCH 1/1 expectation use a slab instead of kmalloc

Hi,

Attached a patch to use a slab for expectations, I think that this could increase a bit the performance in a system which has lots of expectations.

I also moved the increment of an expectation refcount to ip_conntrack_expect_alloc because we could use ip_conntrack_expect_put to release an expectation which we just allocated in a helper (well, in current helpers I didn't find any case but there are weird protocols outthere so I think that it's not a bad idea).

Patrick, if you consider that it's ok, would you be willing to add this to pom-ng? If I'm missing something, please let me know.

regards,
Pablo

net/ipv4/netfilter/ip_conntrack_core.c +23 -9

diff -u -r1.1.1.1 ip_conntrack_core.c
@@ -67,6 +67,7 @@
 static atomic_t ip_conntrack_count = ATOMIC_INIT(0);
 struct list_head *ip_conntrack_hash;
 static kmem_cache_t *ip_conntrack_cachep;
+static kmem_cache_t *ip_conntrack_expect_cachep;
 struct ip_conntrack ip_conntrack_untracked;
 
 extern struct ip_conntrack_protocol ip_conntrack_generic_protocol;

@@ -176,7 +177,7 @@
 	IP_NF_ASSERT(atomic_read(&exp->use));
 	IP_NF_ASSERT(!timer_pending(&exp->timeout));
 
-	kfree(exp);
+	kmem_cache_free(ip_conntrack_expect_cachep, exp);
 }

@@ -335,7 +336,7 @@
 			list_del(&ct->master->expected_list);
 			master = ct->master->expectant;
 		}
-		kfree(ct->master);
+		kmem_cache_free(ip_conntrack_expect_cachep, ct->master);
 	}
 	WRITE_UNLOCK(&ip_conntrack_lock);

@@ -923,16 +924,17 @@
 ip_conntrack_expect_alloc()
 {
 	struct ip_conntrack_expect *new;
-	
-	new = (struct ip_conntrack_expect *)
-		kmalloc(sizeof(struct ip_conntrack_expect), GFP_ATOMIC);
+
+	new = kmem_cache_alloc(ip_conntrack_expect_cachep, GFP_ATOMIC);
 	if (!new) {
 		DEBUGP("expect_related: OOM allocating expect\n");
 		return NULL;
 	}
 
+	DEBUGP("new expectation %p allocated\n", new);
 	/* tuple_cmp compares whole union, we have to initialized cleanly */
 	memset(new, 0, sizeof(struct ip_conntrack_expect));
+	atomic_set(&new->use, 1);
 
 	return new;
 }

@@ -944,7 +946,6 @@
 	DEBUGP("new expectation %p of conntrack %p\n", new, related_to);
 	new->expectant = related_to;
 	new->sibling = NULL;
-	atomic_set(&new->use, 1);
 
 	/* add to expected list for this connection */
 	list_add(&new->expected_list, &related_to->sibling_list);

@@ -998,7 +999,8 @@
 		}
 
 		WRITE_UNLOCK(&ip_conntrack_lock);
-		kfree(expect);
+		/* This expectation is not inserted so no need to lock */
+		kmem_cache_free(ip_conntrack_expect_cachep, expect);
 		return -EEXIST;
 
 	} else if (related_to->helper->max_expected &&

@@ -1017,7 +1019,7 @@
 				       related_to->helper->name,
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip),
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip));
-			kfree(expect);
+			kmem_cache_free(ip_conntrack_expect_cachep, expect);
 			return -EPERM;
 		}
 		DEBUGP("ip_conntrack: max number of expected "

@@ -1058,7 +1060,7 @@
 		WRITE_UNLOCK(&ip_conntrack_lock);
 		DEBUGP("expect_related: busy!\n");
 
-		kfree(expect);
+		kmem_cache_free(ip_conntrack_expect_cachep, expect);
 		return -EBUSY;
 	}

@@ -1379,6 +1381,7 @@
 	}
 
 	kmem_cache_destroy(ip_conntrack_cachep);
+	kmem_cache_destroy(ip_conntrack_expect_cachep);
 	vfree(ip_conntrack_hash);
 	nf_unregister_sockopt(&so_getorigdst);
 }

@@ -1431,6 +1434,15 @@
 		printk(KERN_ERR "Unable to create ip_conntrack slab cache\n");
 		goto err_free_hash;
 	}
+
+        ip_conntrack_expect_cachep = kmem_cache_create("ip_conntrack_expect",
+                                          sizeof(struct ip_conntrack_expect),
+					   0, SLAB_HWCACHE_ALIGN, NULL, NULL);
+        if (!ip_conntrack_expect_cachep) {
+                printk(KERN_ERR "Unable to create ip_expect slab cache\n");
+                goto err_free_conntrack_slab;
+        }
+
 	/* Don't NEED lock here, but good form anyway. */
 	WRITE_LOCK(&ip_conntrack_lock);
 	/* Sew in builtin protocols. */

@@ -1458,6 +1470,8 @@
 
 	return ret;
 
+err_free_conntrack_slab:
+	kmem_cache_destroy(ip_conntrack_cachep);
 err_free_hash:
 	vfree(ip_conntrack_hash);
 err_unreg_sockopt:

Patrick McHardy

2004-06-01 23:05 UTC #1 lore raw

Hi Pablo,

Quoted 3 lines · Pablo Neira

Hi,
Attached a patch to use a slab for expectations, I think that this could increase a bit the performance in a system which has lots of expectations.

I like it, mainly because it gives usage statistics and leak-checks for free.

Quoted 2 lines

I also moved the increment of an expectation refcount to ip_conntrack_expect_alloc because we could use ip_conntrack_expect_put to release an expectation which we just allocated in a helper (well, in current helpers I didn't find any case but there are weird protocols outthere so I think that it's not a bad idea).

I also like this better.

Quoted 2 lines

Patrick, if you consider that it's ok, would you be willing to add this to pom-ng? If I'm missing something, please let me know.

I'm going to add it, but please fix the two minor issues noted below first.

Quoted 41 lines

regards,
Pablo

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

diff -u -r1.1.1.1 ip_conntrack_core.c

net/ipv4/netfilter/ip_conntrack_core.c +3 -3

@@ -67,6 +67,7 @@
 static atomic_t ip_conntrack_count = ATOMIC_INIT(0);
 struct list_head *ip_conntrack_hash;
 static kmem_cache_t *ip_conntrack_cachep;
+static kmem_cache_t *ip_conntrack_expect_cachep;
 struct ip_conntrack ip_conntrack_untracked;
 
 extern struct ip_conntrack_protocol ip_conntrack_generic_protocol;

@@ -176,7 +177,7 @@
 	IP_NF_ASSERT(atomic_read(&exp->use));
 	IP_NF_ASSERT(!timer_pending(&exp->timeout));
 
-	kfree(exp);
+	kmem_cache_free(ip_conntrack_expect_cachep, exp);
 }

@@ -335,7 +336,7 @@
 			list_del(&ct->master->expected_list);
 			master = ct->master->expectant;
 		}
-		kfree(ct->master);
+		kmem_cache_free(ip_conntrack_expect_cachep, ct->master);
 	}
 	WRITE_UNLOCK(&ip_conntrack_lock);

@@ -923,16 +924,17 @@
 ip_conntrack_expect_alloc()
 {
 	struct ip_conntrack_expect *new;
-

net/ipv4/netfilter/ip_conntrack_core.c line 934

Quoted 10 lines

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

-	new = (struct ip_conntrack_expect *)
-		kmalloc(sizeof(struct ip_conntrack_expect), GFP_ATOMIC);
+
+	new = kmem_cache_alloc(ip_conntrack_expect_cachep, GFP_ATOMIC);
 	if (!new) {
 		DEBUGP("expect_related: OOM allocating expect\n");
 		return NULL;
 	}
 
+	DEBUGP("new expectation %p allocated\n", new);

^^^ I don't think we need that

Quoted 52 lines

net/ipv4/netfilter/ip_conntrack_core.c +6 -4

 	/* tuple_cmp compares whole union, we have to initialized cleanly */
 	memset(new, 0, sizeof(struct ip_conntrack_expect));
+	atomic_set(&new->use, 1);
 
 	return new;
 }

@@ -944,7 +946,6 @@
 	DEBUGP("new expectation %p of conntrack %p\n", new, related_to);
 	new->expectant = related_to;
 	new->sibling = NULL;
-	atomic_set(&new->use, 1);
 
 	/* add to expected list for this connection */
 	list_add(&new->expected_list, &related_to->sibling_list);

@@ -998,7 +999,8 @@
 		}
 
 		WRITE_UNLOCK(&ip_conntrack_lock);
-		kfree(expect);
+		/* This expectation is not inserted so no need to lock */
+		kmem_cache_free(ip_conntrack_expect_cachep, expect);
 		return -EEXIST;
 
 	} else if (related_to->helper->max_expected &&

@@ -1017,7 +1019,7 @@
 				       related_to->helper->name,
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip),
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip));
-			kfree(expect);
+			kmem_cache_free(ip_conntrack_expect_cachep, expect);
 			return -EPERM;
 		}
 		DEBUGP("ip_conntrack: max number of expected "

@@ -1058,7 +1060,7 @@
 		WRITE_UNLOCK(&ip_conntrack_lock);
 		DEBUGP("expect_related: busy!\n");
 
-		kfree(expect);
+		kmem_cache_free(ip_conntrack_expect_cachep, expect);
 		return -EBUSY;
 	}

@@ -1379,6 +1381,7 @@
 	}
 
 	kmem_cache_destroy(ip_conntrack_cachep);
+	kmem_cache_destroy(ip_conntrack_expect_cachep);
 	vfree(ip_conntrack_hash);
 	nf_unregister_sockopt(&so_getorigdst);
 }

@@ -1431,6 +1434,15 @@
 		printk(KERN_ERR "Unable to create ip_conntrack slab cache\n");

net/ipv4/netfilter/ip_conntrack_core.c line 1444

Quoted 10 lines

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

 		goto err_free_hash;
 	}
+
+        ip_conntrack_expect_cachep = kmem_cache_create("ip_conntrack_expect",
+                                          sizeof(struct ip_conntrack_expect),
+					   0, SLAB_HWCACHE_ALIGN, NULL, NULL);
+        if (!ip_conntrack_expect_cachep) {
+                printk(KERN_ERR "Unable to create ip_expect slab cache\n");
+                goto err_free_conntrack_slab;
+        }

^^^ Indentation is broken (spaces instead of tabs)

net/ipv4/netfilter/ip_conntrack_core.c line 1477

Quoted 13 lines

net/ipv4/netfilter/ip_conntrack_core.c +3 -0

+
 	/* Don't NEED lock here, but good form anyway. */
 	WRITE_LOCK(&ip_conntrack_lock);
 	/* Sew in builtin protocols. */

@@ -1458,6 +1470,8 @@
 
 	return ret;
 
+err_free_conntrack_slab:
+	kmem_cache_destroy(ip_conntrack_cachep);
 err_free_hash:
 	vfree(ip_conntrack_hash);
 err_unreg_sockopt:

Regards
Patrick

Pablo Neira

author

2004-06-02 11:10 UTC #2 lore raw

Hi Patrick,

Quoted 1 line · Patrick McHardy

please fix the two minor issues noted below first.

I fixed them, attached the patch.

thanks!
Pablo

net/ipv4/netfilter/ip_conntrack_core.c +22 -9

diff -u -r1.1.1.1 ip_conntrack_core.c
@@ -67,6 +67,7 @@
 static atomic_t ip_conntrack_count = ATOMIC_INIT(0);
 struct list_head *ip_conntrack_hash;
 static kmem_cache_t *ip_conntrack_cachep;
+static kmem_cache_t *ip_conntrack_expect_cachep;
 struct ip_conntrack ip_conntrack_untracked;
 
 extern struct ip_conntrack_protocol ip_conntrack_generic_protocol;

@@ -176,7 +177,7 @@
 	IP_NF_ASSERT(atomic_read(&exp->use));
 	IP_NF_ASSERT(!timer_pending(&exp->timeout));
 
-	kfree(exp);
+	kmem_cache_free(ip_conntrack_expect_cachep, exp);
 }

@@ -335,7 +336,7 @@
 			list_del(&ct->master->expected_list);
 			master = ct->master->expectant;
 		}
-		kfree(ct->master);
+		kmem_cache_free(ip_conntrack_expect_cachep, ct->master);
 	}
 	WRITE_UNLOCK(&ip_conntrack_lock);

@@ -923,9 +924,8 @@
 ip_conntrack_expect_alloc()
 {
 	struct ip_conntrack_expect *new;
-	
-	new = (struct ip_conntrack_expect *)
-		kmalloc(sizeof(struct ip_conntrack_expect), GFP_ATOMIC);
+
+	new = kmem_cache_alloc(ip_conntrack_expect_cachep, GFP_ATOMIC);
 	if (!new) {
 		DEBUGP("expect_related: OOM allocating expect\n");
 		return NULL;

@@ -933,6 +933,7 @@
 
 	/* tuple_cmp compares whole union, we have to initialized cleanly */
 	memset(new, 0, sizeof(struct ip_conntrack_expect));
+	atomic_set(&new->use, 1);
 
 	return new;
 }

@@ -944,7 +945,6 @@
 	DEBUGP("new expectation %p of conntrack %p\n", new, related_to);
 	new->expectant = related_to;
 	new->sibling = NULL;
-	atomic_set(&new->use, 1);
 
 	/* add to expected list for this connection */
 	list_add(&new->expected_list, &related_to->sibling_list);

@@ -998,7 +998,8 @@
 		}
 
 		WRITE_UNLOCK(&ip_conntrack_lock);
-		kfree(expect);
+		/* This expectation is not inserted so no need to lock */
+		kmem_cache_free(ip_conntrack_expect_cachep, expect);
 		return -EEXIST;
 
 	} else if (related_to->helper->max_expected &&

@@ -1017,7 +1018,7 @@
 				       related_to->helper->name,
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip),
  		    	       	       NIPQUAD(related_to->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip));
-			kfree(expect);
+			kmem_cache_free(ip_conntrack_expect_cachep, expect);
 			return -EPERM;
 		}
 		DEBUGP("ip_conntrack: max number of expected "

@@ -1058,7 +1059,7 @@
 		WRITE_UNLOCK(&ip_conntrack_lock);
 		DEBUGP("expect_related: busy!\n");
 
-		kfree(expect);
+		kmem_cache_free(ip_conntrack_expect_cachep, expect);
 		return -EBUSY;
 	}

@@ -1379,6 +1380,7 @@
 	}
 
 	kmem_cache_destroy(ip_conntrack_cachep);
+	kmem_cache_destroy(ip_conntrack_expect_cachep);
 	vfree(ip_conntrack_hash);
 	nf_unregister_sockopt(&so_getorigdst);
 }

@@ -1431,6 +1433,15 @@
 		printk(KERN_ERR "Unable to create ip_conntrack slab cache\n");
 		goto err_free_hash;
 	}
+
+	ip_conntrack_expect_cachep = kmem_cache_create("ip_conntrack_expect",
+					sizeof(struct ip_conntrack_expect),
+					0, SLAB_HWCACHE_ALIGN, NULL, NULL);
+	if (!ip_conntrack_expect_cachep) {
+		printk(KERN_ERR "Unable to create ip_expect slab cache\n");
+		goto err_free_conntrack_slab;
+	}
+
 	/* Don't NEED lock here, but good form anyway. */
 	WRITE_LOCK(&ip_conntrack_lock);
 	/* Sew in builtin protocols. */

@@ -1458,6 +1469,8 @@
 
 	return ret;
 
+err_free_conntrack_slab:
+	kmem_cache_destroy(ip_conntrack_cachep);
 err_free_hash:
 	vfree(ip_conntrack_hash);
 err_unreg_sockopt:

Patrick McHardy

Applied

2004-06-02 16:12 UTC #3 lore raw

Quoted 2 lines · Pablo Neira

I fixed them, attached the patch.

Thanks, applied.

Regards
Patrick