Skip to contentSkip to conversation

Unaligend accesses nulldevname

PATCH In review Kurt Garloff 2 participants 2 messages Jan 7, 2004 – Jan 11, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Unaligend accesses nulldevname

Hi,

I found an excessive amount of unaligned accesses on my AXP workstation and tracked it down to ip_packet_match() in the ip_tables module. indev and outdev are not properly aligned if set to nulldevname in ipt_do_table().
This destroys the benefits of comparing names in units of (long) and on architectures with expensive unaligned accesses (such as ia64 or alpha), it even hurts a lot.

Find attached a patch against 2.6.0. A similar patch is needed for 2.4, also attached.

Please consider merging them.

Looking at ip_packet_match(), I have two more thoughts:
* It should not be inlined. It's too large to benefit from inlining, IMHO. (OTOH, it's only called from one place, so it does not really matter.)
* There's a comment about the compiler being able to unroll the 2/4 (64/32bit) iter loop which is not completely appropriate: We don't pass -funroll-loops, so gcc does not do it :-(
It would be beneficial though.

Regards,

Signature · 2 lines
Kurt Garloff  <[email protected]>                            Cologne, DE 
SUSE LINUX AG, Nuernberg, DE                          SUSE Labs (Head)
net/ipv4/netfilter/ip_tables.c renamed from net/ipv4/netfilter/ip_tables.c.orig +1 -1
@@ -260,7 +260,7 @@
 	     struct ipt_table *table,
 	     void *userdata)
 {
-	static const char nulldevname[IFNAMSIZ];
+	static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long))));
 	u_int16_t offset;
 	struct iphdr *ip;
 	u_int16_t datalen;
net/ipv4/netfilter/ip_tables.c +1 -1
@@ -259,7 +264,7 @@
 	     struct ipt_table *table,
 	     void *userdata)
 {
-	static const char nulldevname[IFNAMSIZ] = { 0 };
+	static const char nulldevname[IFNAMSIZ] __attribute__((aligned(sizeof(long)))) = { 0 };
 	u_int16_t offset;
 	struct iphdr *ip;
 	void *protohdr;
#1 lore raw
Quoted 9 lines · Kurt Garloff

Hi,

I found an excessive amount of unaligned accesses on my AXP workstation and tracked it down to ip_packet_match() in the ip_tables module. indev and outdev are not properly aligned if set to nulldevname in ipt_do_table().
This destroys the benefits of comparing names in units of (long) and on architectures with expensive unaligned accesses (such as ia64 or alpha), it even hurts a lot.

Thanks for submitting your patch, this is the same bug report as
https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=84

Quoted 2 lines

Find attached a patch against 2.6.0. A similar patch is needed for 2.4, also attached.

the fix is already in our patch-o-matic CVS tree, and I'm going to submit this to davem together with some other fixes I have pending.

Quoted 4 lines

Looking at ip_packet_match(), I have two more thoughts:
* It should not be inlined. It's too large to benefit from inlining, IMHO. (OTOH, it's only called from one place, so it does not really matter.)

That is the idea. It's just split in two functions to make it more readable. It's never intended to be called from somewhere else.

Quoted 4 lines

* There's a comment about the compiler being able to unroll the 2/4 (64/32bit) iter loop which is not completely appropriate: We don't pass -funroll-loops, so gcc does not do it :-(
It would be beneficial though.

I'm not a compiler geek. Thanks for pointing this out, I will do some testing and look at the compiler output to see what is happening.

Quoted 3 lines

Regards,

Signature · 1 line
Kurt Garloff  <[email protected]>                            Cologne, DE 
Signature · 5 lines
- Harald Welte <[email protected]>             http://www.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