Unaligend accesses nulldevname
Subsystems (2)
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;
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 theip_tablesmodule. indev and outdev are not properly aligned if set to nulldevname inipt_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