Re: IP header alignment

2008-01-15 Thread Vladimir Kondratiev
  Thus, I believe, for IP stack it is wise to have IP header aligned by
  drivers. I warn once, so it do not flood system log with messages.

 It is wise only on systems that need it, and most can (albeit
 expensively) handle the unaligned access, so it is absolutely
 not a requirement in those cases.

 Look, the point still stands, this thing is going to crap into
 people's kernel logs and that is not acceptable.

 Furthermore, there are drivers that do alignment conditionally
 based upon whether the platform can handle it transparently or
 not.  They do this because fixing the alignment might result
 in expensive copies.

 So these drivers will always generate warnings, even though it
 is entirely intentional.

 This is just a bad idea, sorry.  And anyone who is truly interested in
 this kind of work can add this as a temporary debugging patch to their
 tree during development.


There are several aspects.

1:
I agree that there is platforms that can handle unaligned access. But,
there are others that can't. And, on platforms that can't unaligned IP
header will lead to incorrect data processing. I want to deal with
this issue. So, in this case, I can check alignment and, if platform
can't handle unaligned loads, issue error message once and drop
unaligned frames always.
2:
Even for platforms that can do unaligned loads, overhead is not zero.
If it is easy to modify driver to align data without overhead for
frame copy, it is worth to do so. In this case, warn once don't seems
to be that bad. Drivers that intentionally deliver unaligned data will
generate one line in syslog, is it critical? This warning may be put
under CONFIG_DEBUG_KERNEL. It will help to identify problem in driver
for those who have no platform w/o unaligned loads.

Comments?
-
To unsubscribe from this list: send the line unsubscribe linux-net in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: IP header alignment

2008-01-14 Thread Stephen Hemminger
On Mon, 14 Jan 2008 10:33:52 +0200
Vladimir Kondratiev [EMAIL PROTECTED] wrote:

 Following patch made against 2.6.24-rc7
 It warns if IP stack gets frame with IP header not aligned on dword.
 Idea is this will help to identify mis-behaving drivers that do not
 enforce this alignment.
 -
 commit d9ff4a47ca7bd902947eb22ed3b01682dce2752f
 Author: Vladimir Kondratiev [EMAIL PROTECTED]
 Date:   Mon Jan 14 10:08:09 2008 +0200
 
 [NET] warn on non-aligned IP header
 
 IP stack silently assumes that IP header is aligned on dword.
 While IP stack may be modified to overcome this requirement, it will
 impose performance penalty. Better off to force drivers to properly align
 data in received frame.
 
 This patch warns once, hoping that frame will be handled properly any way.
 Drop frame in this case would be unappropriate - this will break some
 wireless drivers.
 
 diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
 index 168c871..88c8369 100644
 --- a/net/ipv4/ip_input.c
 +++ b/net/ipv4/ip_input.c
 @@ -400,7 +400,23 @@ int ip_rcv(struct sk_buff *skb, struct net_device
 *dev, struct packet_type *pt,
  goto inhdr_error;
 
  iph = ip_hdr(skb);
 -
 +/**
 + * IP stack silently assumes that IP header aligned on dword.
 + * If it is not:
 + * 1. extra time for non-aligned access will be paid for every
 + * architecture;
 + * 2. various things will mis-behave on hardware that do not
 + * support non-aligned access.
 + * Driver code should be fixed.
 + */
 +if (unlikely((u32)iph % 4)) {
 +static int __warned;
 +if (unlikely(!__warned)) {
 +__warned = 1;
 +dev_warn(orig_dev-dev,Non aligned IP header from [%s]\n,
 + orig_dev-name);
 +}
 +}
  /*
   *RFC1122: 3.1.2.2 MUST silently discard any IP frame that
 fails the checksum.
   *

No.  There is hardware like Yukon-EC used in mac mini where the cost
of the CPU unaligned access is trivial and the cost of copying every
frame to shut up the stupid warning would be high.




-- 
Stephen Hemminger [EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe linux-net in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html