Re: [PATCH] Merge __load_pointer() and load_pointer() in net/core/filter.c; kernel 2.6.14

From: Mitchell Blank Jr
Cc:
herbert@gondor.apana.org.au;jschlst@samba.org;davem@davemloft.net;
acme@ghostprotocols.net;netdev@vger.kernel.org

quoted

(I trimmed the cc: list a bit; no need for this to be on LKML in my opinion)

Kris Katterjohn wrote:

quoted

Forgive me because this is one of my first attempts at anything related to the
kernel, but...

1) How would I go about benchmarking this?

The first thing to do is run "size net/core/filter.o" before and after and
see if your change makes the "text" section larger.

The risk of putting more stuff into the inlined function is just that
the rarely-executed path will end up duplicated in a bunch of places, bloating
the generated code.  It's hard to directly benchmark the effects of this but
it will generally make the fast-path code less compact in the L1 I-cache
which (if you do it enough) slows everything down.  Thats one reason why
kernel developers try to keep a close eye on bloat.

-Mitch

Before patch:

   text	   data	    bss	    dec	    hex	filename
   2399	      0	      0	   2399	    95f	x/net/core/filter.o

After patch:

   text	   data	    bss	    dec	    hex	filename
   2589	      0	      0	   2589	    a1d	y/net/core/filter.o

After patch without inlining the function:

   text	   data	    bss	    dec	    hex	filename
   2127	      0	      0	   2127	    84f	y/net/core/filter.o


So I guess use my patch and take "inline" off? What do you think?


Thanks

Here's the new patch. It's the same as the first one, just with "inline" removed.

Maybe "static" should be removed, too? Oh well.

---

--- x/net/core/filter.c	2005-10-27 19:02:08.000000000 -0500
+++ y/net/core/filter.c	2005-11-03 00:05:42.000000000 -0600

@@ -13,6 +13,8 @@
  * 2 of the License, or (at your option) any later version.
  *
  * Andi Kleen - Fix a few bad bugs and races.
+ * Kris Katterjohn - Merged __load_pointer() into load_pointer() and
+ *                   cleaned it up a little bit - 2005-11-01
  */
 
 #include <linux/module.h>

@@ -35,31 +37,27 @@
 #include <asm/uaccess.h>
 #include <linux/filter.h>
 
-/* No hurry in this branch */
-static void *__load_pointer(struct sk_buff *skb, int k)
+static void *load_pointer(struct sk_buff *skb, int offset,
+                          unsigned int size, void *buffer)
 {
 	u8 *ptr = NULL;
 
-	if (k >= SKF_NET_OFF)
-		ptr = skb->nh.raw + k - SKF_NET_OFF;
-	else if (k >= SKF_LL_OFF)
-		ptr = skb->mac.raw + k - SKF_LL_OFF;
+	if (offset >= 0)
+		return skb_header_pointer(skb, offset, size, buffer);
+
+	if (offset >= SKF_AD_OFF)
+		return NULL;
+
+	if (offset >= SKF_NET_OFF)
+		ptr = skb->nh.raw + offset - SKF_NET_OFF;
+
+	else if (offset >= SKF_LL_OFF)
+		ptr = skb->mac.raw + offset - SKF_LL_OFF;
 
 	if (ptr >= skb->head && ptr < skb->tail)
 		return ptr;
-	return NULL;
-}
 
-static inline void *load_pointer(struct sk_buff *skb, int k,
-                                 unsigned int size, void *buffer)
-{
-	if (k >= 0)
-		return skb_header_pointer(skb, k, size, buffer);
-	else {
-		if (k >= SKF_AD_OFF)
-			return NULL;
-		return __load_pointer(skb, k);
-	}
+	return NULL;
 }
 
 /**

Kris Katterjohn wrote:

quoted

From: Mitchell Blank Jr

quoted

(I trimmed the cc: list a bit; no need for this to be on LKML in my opinion)

I see you just added it back.  Oh well.

quoted

So I guess use my patch and take "inline" off? What do you think?

Well the original author presumably thought that the fast-path of
load_pointer() was critical enough to keep inline (since it can be run many
times per packet)  So they made the deliberate choice of separating it
into two functions - one inline, one non-inline.

So my personal feeling is that the code is probably fine as it stands today.

Maybe "static" should be removed, too? Oh well.

Uh, why?  It's clearly a file-local function.

-Mitch

Mitchell Blank Jr wrote:

Kris Katterjohn wrote:

quoted

quoted

So I guess use my patch and take "inline" off? What do you think?

Well the original author presumably thought that the fast-path of
load_pointer() was critical enough to keep inline (since it can be run many
times per packet)  So they made the deliberate choice of separating it
into two functions - one inline, one non-inline.

Exactly. __load_pointer is only called rarely, while load_pointer is
called whenever data needs to be read from the packet. It shouldn't
be changed without any justification.