[PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[Jesper Juhl]

Hi,

Here's a small patch to decrease the number of pointer derefs in
net/netfilter/nfnetlink_queue.c

Benefits of the patch:
 - Fewer pointer dereferences should make the code slightly faster.
 - Size of generated code is smaller
 - improved readability

Please consider applying.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 net/netfilter/nfnetlink_queue.c |   79 ++++++++++++++++++++++------------------
 1 files changed, 44 insertions(+), 35 deletions(-)

orig:
   text    data     bss     dec     hex filename
   5357     244      64    5665    1621 net/netfilter/nfnetlink_queue.o

patched:
   text    data     bss     dec     hex filename
   5335     244      64    5643    160b net/netfilter/nfnetlink_queue.o

--- linux-2.6.15-rc5-git1-orig/net/netfilter/nfnetlink_queue.c	2005-12-04 18:48:58.000000000 +0100
+++ linux-2.6.15-rc5-git1/net/netfilter/nfnetlink_queue.c	2005-12-08 19:47:22.000000000 +0100
@@ -345,6 +345,10 @@ nfqnl_build_packet_message(struct nfqnl_
 	struct nfqnl_msg_packet_hdr pmsg;
 	struct nlmsghdr *nlh;
 	struct nfgenmsg *nfmsg;
+	struct nf_info *entinf = entry->info;
+	struct sk_buff *entskb = entry->skb;
+	struct net_device *indev;
+	struct net_device *outdev;
 	unsigned int tmp_uint;
 
 	QDEBUG("entered\n");
@@ -361,6 +365,8 @@ nfqnl_build_packet_message(struct nfqnl_
 		+ NLMSG_SPACE(sizeof(struct nfqnl_msg_packet_hw))
 		+ NLMSG_SPACE(sizeof(struct nfqnl_msg_packet_timestamp));
 
+	outdev = entinf->outdev;
+
 	spin_lock_bh(&queue->lock);
 	
 	switch (queue->copy_mode) {
@@ -370,15 +376,15 @@ nfqnl_build_packet_message(struct nfqnl_
 		break;
 	
 	case NFQNL_COPY_PACKET:
-		if (entry->skb->ip_summed == CHECKSUM_HW &&
-		    (*errp = skb_checksum_help(entry->skb,
-		                               entry->info->outdev == NULL))) {
+		if (entskb->ip_summed == CHECKSUM_HW &&
+		    (*errp = skb_checksum_help(entskb,
+		                               outdev == NULL))) {
 			spin_unlock_bh(&queue->lock);
 			return NULL;
 		}
 		if (queue->copy_range == 0 
-		    || queue->copy_range > entry->skb->len)
-			data_len = entry->skb->len;
+		    || queue->copy_range > entskb->len)
+			data_len = entskb->len;
 		else
 			data_len = queue->copy_range;
 		
@@ -402,29 +408,30 @@ nfqnl_build_packet_message(struct nfqnl_
 			NFNL_SUBSYS_QUEUE << 8 | NFQNL_MSG_PACKET,
 			sizeof(struct nfgenmsg));
 	nfmsg = NLMSG_DATA(nlh);
-	nfmsg->nfgen_family = entry->info->pf;
+	nfmsg->nfgen_family = entinf->pf;
 	nfmsg->version = NFNETLINK_V0;
 	nfmsg->res_id = htons(queue->queue_num);
 
 	pmsg.packet_id 		= htonl(entry->id);
-	pmsg.hw_protocol	= htons(entry->skb->protocol);
-	pmsg.hook		= entry->info->hook;
+	pmsg.hw_protocol	= htons(entskb->protocol);
+	pmsg.hook		= entinf->hook;
 
 	NFA_PUT(skb, NFQA_PACKET_HDR, sizeof(pmsg), &pmsg);
 
-	if (entry->info->indev) {
-		tmp_uint = htonl(entry->info->indev->ifindex);
+	indev = entinf->indev;
+	if (indev) {
+		tmp_uint = htonl(indev->ifindex);
 #ifndef CONFIG_BRIDGE_NETFILTER
 		NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint), &tmp_uint);
 #else
-		if (entry->info->pf == PF_BRIDGE) {
+		if (entinf->pf == PF_BRIDGE) {
 			/* Case 1: indev is physical input device, we need to
 			 * look for bridge group (when called from 
 			 * netfilter_bridge) */
 			NFA_PUT(skb, NFQA_IFINDEX_PHYSINDEV, sizeof(tmp_uint), 
 				&tmp_uint);
 			/* this is the bridge group "brX" */
-			tmp_uint = htonl(entry->info->indev->br_port->br->dev->ifindex);
+			tmp_uint = htonl(indev->br_port->br->dev->ifindex);
 			NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint),
 				&tmp_uint);
 		} else {
@@ -432,9 +439,9 @@ nfqnl_build_packet_message(struct nfqnl_
 			 * physical device (when called from ipv4) */
 			NFA_PUT(skb, NFQA_IFINDEX_INDEV, sizeof(tmp_uint),
 				&tmp_uint);
-			if (entry->skb->nf_bridge
-			    && entry->skb->nf_bridge->physindev) {
-				tmp_uint = htonl(entry->skb->nf_bridge->physindev->ifindex);
+			if (entskb->nf_bridge
+			    && entskb->nf_bridge->physindev) {
+				tmp_uint = htonl(entskb->nf_bridge->physindev->ifindex);
 				NFA_PUT(skb, NFQA_IFINDEX_PHYSINDEV,
 					sizeof(tmp_uint), &tmp_uint);
 			}
@@ -442,19 +449,19 @@ nfqnl_build_packet_message(struct nfqnl_
 #endif
 	}
 
-	if (entry->info->outdev) {
-		tmp_uint = htonl(entry->info->outdev->ifindex);
+	if (outdev) {
+		tmp_uint = htonl(outdev->ifindex);
 #ifndef CONFIG_BRIDGE_NETFILTER
 		NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint), &tmp_uint);
 #else
-		if (entry->info->pf == PF_BRIDGE) {
+		if (entinf->pf == PF_BRIDGE) {
 			/* Case 1: outdev is physical output device, we need to
 			 * look for bridge group (when called from 
 			 * netfilter_bridge) */
 			NFA_PUT(skb, NFQA_IFINDEX_PHYSOUTDEV, sizeof(tmp_uint),
 				&tmp_uint);
 			/* this is the bridge group "brX" */
-			tmp_uint = htonl(entry->info->outdev->br_port->br->dev->ifindex);
+			tmp_uint = htonl(outdev->br_port->br->dev->ifindex);
 			NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint),
 				&tmp_uint);
 		} else {
@@ -462,9 +469,9 @@ nfqnl_build_packet_message(struct nfqnl_
 			 * physical output device (when called from ipv4) */
 			NFA_PUT(skb, NFQA_IFINDEX_OUTDEV, sizeof(tmp_uint),
 				&tmp_uint);
-			if (entry->skb->nf_bridge
-			    && entry->skb->nf_bridge->physoutdev) {
-				tmp_uint = htonl(entry->skb->nf_bridge->physoutdev->ifindex);
+			if (entskb->nf_bridge
+			    && entskb->nf_bridge->physoutdev) {
+				tmp_uint = htonl(entskb->nf_bridge->physoutdev->ifindex);
 				NFA_PUT(skb, NFQA_IFINDEX_PHYSOUTDEV,
 					sizeof(tmp_uint), &tmp_uint);
 			}
@@ -472,27 +479,27 @@ nfqnl_build_packet_message(struct nfqnl_
 #endif
 	}
 
-	if (entry->skb->nfmark) {
-		tmp_uint = htonl(entry->skb->nfmark);
+	if (entskb->nfmark) {
+		tmp_uint = htonl(entskb->nfmark);
 		NFA_PUT(skb, NFQA_MARK, sizeof(u_int32_t), &tmp_uint);
 	}
 
-	if (entry->info->indev && entry->skb->dev
-	    && entry->skb->dev->hard_header_parse) {
+	if (indev && entskb->dev
+	    && entskb->dev->hard_header_parse) {
 		struct nfqnl_msg_packet_hw phw;
 
 		phw.hw_addrlen =
-			entry->skb->dev->hard_header_parse(entry->skb,
+			entskb->dev->hard_header_parse(entskb,
 			                                   phw.hw_addr);
 		phw.hw_addrlen = htons(phw.hw_addrlen);
 		NFA_PUT(skb, NFQA_HWADDR, sizeof(phw), &phw);
 	}
 
-	if (entry->skb->tstamp.off_sec) {
+	if (entskb->tstamp.off_sec) {
 		struct nfqnl_msg_packet_timestamp ts;
 
-		ts.sec = cpu_to_be64(entry->skb->tstamp.off_sec);
-		ts.usec = cpu_to_be64(entry->skb->tstamp.off_usec);
+		ts.sec = cpu_to_be64(entskb->tstamp.off_sec);
+		ts.usec = cpu_to_be64(entskb->tstamp.off_usec);
 
 		NFA_PUT(skb, NFQA_TIMESTAMP, sizeof(ts), &ts);
 	}
@@ -510,7 +517,7 @@ nfqnl_build_packet_message(struct nfqnl_
 		nfa->nfa_type = NFQA_PAYLOAD;
 		nfa->nfa_len = size;
 
-		if (skb_copy_bits(entry->skb, 0, NFA_DATA(nfa), data_len))
+		if (skb_copy_bits(entskb, 0, NFA_DATA(nfa), data_len))
 			BUG();
 	}
 		
@@ -667,12 +674,14 @@ nfqnl_set_mode(struct nfqnl_instance *qu
 static int
 dev_cmp(struct nfqnl_queue_entry *entry, unsigned long ifindex)
 {
-	if (entry->info->indev)
-		if (entry->info->indev->ifindex == ifindex)
+	struct nf_info *entinf = entry->info;
+	
+	if (entinf->indev)
+		if (entinf->indev->ifindex == ifindex)
 			return 1;
 			
-	if (entry->info->outdev)
-		if (entry->info->outdev->ifindex == ifindex)
+	if (entinf->outdev)
+		if (entinf->outdev->ifindex == ifindex)
 			return 1;
 
 	return 0;

Re: [PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[David S. Miller]

From: Jesper Juhl <jesper.juhl@gmail.com>
Date: Thu, 8 Dec 2005 23:36:01 +0100

> Here's a small patch to decrease the number of pointer derefs in
> net/netfilter/nfnetlink_queue.c
> 
> Benefits of the patch:
>  - Fewer pointer dereferences should make the code slightly faster.
>  - Size of generated code is smaller
>  - improved readability

And you verified the compiler isn't making these transformations
already?  It should be doing so via Common Subexpression Elimination
unless the derefs are scattered around with interspersed function
calls in which case the compiler cannot prove that the memory
behind the pointer does not change.

Re: [PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[Jesper Juhl]

On 12/8/05, David S. Miller <davem@davemloft.net> wrote:
> From: Jesper Juhl <jesper.juhl@gmail.com>
> Date: Thu, 8 Dec 2005 23:36:01 +0100
>
> > Here's a small patch to decrease the number of pointer derefs in
> > net/netfilter/nfnetlink_queue.c
> >
> > Benefits of the patch:
> >  - Fewer pointer dereferences should make the code slightly faster.
> >  - Size of generated code is smaller
> >  - improved readability
>
> And you verified the compiler isn't making these transformations
> already?  It should be doing so via Common Subexpression Elimination
> unless the derefs are scattered around with interspersed function
> calls in which case the compiler cannot prove that the memory
> behind the pointer does not change.
>
I've not verified that by actually looking at the generated asm, no.
But given the (sometimes rather large) savings in size for some of the
files I patched I concluded that the compiler was not optimizing that
away.
Even if the compiler does optimize it, it obviously doesn't do as good
a job as it could (or I wouldn't be saving all those bytes by doing
this), and it's not just my gcc 3.3.6, Ingo tested my initial "test
the waters" patch with gcc 4.0.2 and saw similar savings (80 bytes in
that case - kernel/exit.c).


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[Steven Rostedt]

On Thu, 2005-12-08 at 14:51 -0800, David S. Miller wrote:
> From: Jesper Juhl <jesper.juhl@gmail.com>
> Date: Thu, 8 Dec 2005 23:36:01 +0100
> 
> > Here's a small patch to decrease the number of pointer derefs in
> > net/netfilter/nfnetlink_queue.c
> > 
> > Benefits of the patch:
> >  - Fewer pointer dereferences should make the code slightly faster.
> >  - Size of generated code is smaller
> >  - improved readability
> 
> And you verified the compiler isn't making these transformations
> already?  It should be doing so via Common Subexpression Elimination
> unless the derefs are scattered around with interspersed function
> calls in which case the compiler cannot prove that the memory
> behind the pointer does not change.

Even if point one and two are not true, if it doesn't hurt size or
performance, point three (improved readability) is definitely worth
adding.

-- Steve

Re: [PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[Jeff Garzik]

David S. Miller wrote:
> From: Jesper Juhl <jesper.juhl@gmail.com>
> Date: Thu, 8 Dec 2005 23:36:01 +0100
> 
> 
>>Here's a small patch to decrease the number of pointer derefs in
>>net/netfilter/nfnetlink_queue.c
>>
>>Benefits of the patch:
>> - Fewer pointer dereferences should make the code slightly faster.
>> - Size of generated code is smaller
>> - improved readability
> 
> 
> And you verified the compiler isn't making these transformations
> already?

Didn't the provided size(1) output would verify this?

In any case, I like the changes because it makes the code more readable, 
with the smaller code size as a pleasant side effect.  Due to increase 
readability, I would only be inclined to NAK if the code 
performance/size was adversely effected.

	Jeff

Re: [PATCH] Decrease number of pointer derefs in nfnetlink_queue.c

[Harald Welte]

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

On Thu, Dec 08, 2005 at 11:36:01PM +0100, Jesper Juhl wrote:
 
> Here's a small patch to decrease the number of pointer derefs in
> net/netfilter/nfnetlink_queue.c

Thanks, the patch looks fine to me.  

Also, if you feel like it, I would recommend doing a similar patch for
nfnetlink_log (which has a similar architecture to nfnetlink_queue).

Patrick: Please merge into your queue of pending patches, unless you
have any objections. Thanks!

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]