Skip to contentSkip to conversation

Decrease number of pointer derefs in nfnetlink_queue.c

PATCH In review Jesper Juhl 5 participants 6 messages Dec 8, 2005 – Dec 9, 2005

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Jesper Juhl

author
#0 lore raw

PATCH 1/1 Decrease number of pointer derefs in nfnetlink_queue.c

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 <[email protected]>

---

 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

net/netfilter/nfnetlink_queue.c +44 -35
@@ -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;
maintainer
#1 lore raw

From: Jesper Juhl <[email protected]>
Date: Thu, 8 Dec 2005 23:36:01 +0100

Quoted 7 lines

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.

author
#2 lore raw
Quoted 11 lines · David S. Miller

From: Jesper Juhl <[email protected]>
Date: Thu, 8 Dec 2005 23:36:01 +0100

Quoted 7 lines

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

Quoted 6 lines

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 <[email protected]>
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

#3 lore raw
In reply to David S. Miller
Quoted 11 lines · David S. Miller

From: Jesper Juhl <[email protected]>
Date: Thu, 8 Dec 2005 23:36:01 +0100

Quoted 7 lines

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

Quoted 5 lines

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

#4 lore raw
In reply to David S. Miller
Quoted 7 lines · David S. Miller

From: Jesper Juhl <[email protected]>
Date: Thu, 8 Dec 2005 23:36:01 +0100

Quoted 3 lines

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

Quoted 8 lines
Quoted 4 lines

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

#5 lore raw
In reply to Jesper Juhl
Quoted 2 lines · Jesper Juhl

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!

Signature · 5 lines
- Harald Welte <[email protected]>                 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