[PATCH] add compare_ether_addr_unaligned

[PATCH] add compare_ether_addr_unaligned

[Daniel Drake]

David Miller found a problem in a wireless driver where I was using
compare_ether_addr() on potentially unaligned data. Document that
compare_ether_addr() is not safe for use everywhere, and add an equivalent
function that works regardless of alignment.

Signed-off-by: Daniel Drake <dsd@gentoo.org>

Index: linux-2.6.24-rc3-git1/include/linux/etherdevice.h
===================================================================
--- linux-2.6.24-rc3-git1.orig/include/linux/etherdevice.h
+++ linux-2.6.24-rc3-git1/include/linux/etherdevice.h
@@ -123,11 +123,13 @@ static inline void random_ether_addr(u8 
 }
 
 /**
- * compare_ether_addr - Compare two Ethernet addresses
+ * compare_ether_addr - Compare two 16-bit-aligned Ethernet addresses
  * @addr1: Pointer to a six-byte array containing the Ethernet address
  * @addr2: Pointer other six-byte array containing the Ethernet address
  *
- * Compare two ethernet addresses, returns 0 if equal
+ * Compare two ethernet addresses, returns 0 if equal. Both addresses must
+ * be 16-bit aligned. For unaligned or potentially unaligned address
+ * comparisons, use compare_ether_addr_unaligned() instead.
  */
 static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2)
 {
@@ -137,6 +139,21 @@ static inline unsigned compare_ether_add
 	BUILD_BUG_ON(ETH_ALEN != 6);
 	return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0;
 }
+
+/**
+ * compare_ether_addr_unaligned - Compare two Ethernet addresses
+ * @addr1: Pointer to a six-byte array containing the Ethernet address
+ * @addr2: Pointer other six-byte array containing the Ethernet address
+ *
+ * Compare two ethernet addresses, returns 0 if equal. compare_ether_addr()
+ * is faster than this function, but unless you can ensure alignment you
+ * must use this function instead.
+ */
+static inline unsigned compare_ether_addr_unaligned(const u8 *addr1,
+						    const u8 *addr2)
+{
+	return memcmp(addr1, addr2, ETH_ALEN);
+}
 #endif	/* __KERNEL__ */
 
 #endif	/* _LINUX_ETHERDEVICE_H */

Re: [PATCH] add compare_ether_addr_unaligned

[Stephen Hemminger]

On Fri, 23 Nov 2007 00:09:22 +0000 (GMT)
Daniel Drake <dsd@gentoo.org> wrote:

> David Miller found a problem in a wireless driver where I was using
> compare_ether_addr() on potentially unaligned data. Document that
> compare_ether_addr() is not safe for use everywhere, and add an equivalent
> function that works regardless of alignment.
> 
> Signed-off-by: Daniel Drake <dsd@gentoo.org>
> 
> Index: linux-2.6.24-rc3-git1/include/linux/etherdevice.h
> ===================================================================
> --- linux-2.6.24-rc3-git1.orig/include/linux/etherdevice.h
> +++ linux-2.6.24-rc3-git1/include/linux/etherdevice.h
> @@ -123,11 +123,13 @@ static inline void random_ether_addr(u8 
>  }
>  
>  /**
> - * compare_ether_addr - Compare two Ethernet addresses
> + * compare_ether_addr - Compare two 16-bit-aligned Ethernet addresses
>   * @addr1: Pointer to a six-byte array containing the Ethernet address
>   * @addr2: Pointer other six-byte array containing the Ethernet address
>   *
> - * Compare two ethernet addresses, returns 0 if equal
> + * Compare two ethernet addresses, returns 0 if equal. Both addresses must
> + * be 16-bit aligned. For unaligned or potentially unaligned address
> + * comparisons, use compare_ether_addr_unaligned() instead.
>   */
>  static inline unsigned compare_ether_addr(const u8 *addr1, const u8 *addr2)
>  {
> @@ -137,6 +139,21 @@ static inline unsigned compare_ether_add
>  	BUILD_BUG_ON(ETH_ALEN != 6);
>  	return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2])) != 0;
>  }
> +
> +/**
> + * compare_ether_addr_unaligned - Compare two Ethernet addresses
> + * @addr1: Pointer to a six-byte array containing the Ethernet address
> + * @addr2: Pointer other six-byte array containing the Ethernet address
> + *
> + * Compare two ethernet addresses, returns 0 if equal. compare_ether_addr()
> + * is faster than this function, but unless you can ensure alignment you
> + * must use this function instead.
> + */
> +static inline unsigned compare_ether_addr_unaligned(const u8 *addr1,
> +						    const u8 *addr2)
> +{
> +	return memcmp(addr1, addr2, ETH_ALEN);
> +}
>  #endif	/* __KERNEL__ */


You could probably get the similar speedup by doing xor by byte, rather
than falling back to memcmp.

Re: [PATCH] add compare_ether_addr_unaligned

[Herbert Xu]

On Fri, Nov 23, 2007 at 12:09:22AM +0000, Daniel Drake wrote:
> David Miller found a problem in a wireless driver where I was using
> compare_ether_addr() on potentially unaligned data. Document that
> compare_ether_addr() is not safe for use everywhere, and add an equivalent
> function that works regardless of alignment.
> 
> Signed-off-by: Daniel Drake <dsd@gentoo.org>

Patch applied to net-2.6.  Thanks.

I presume there will be some follow-on work to use this function?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] add compare_ether_addr_unaligned

[Herbert Xu]

On Fri, Nov 23, 2007 at 09:26:31PM +0800, Herbert Xu wrote:
> On Fri, Nov 23, 2007 at 12:09:22AM +0000, Daniel Drake wrote:
> > David Miller found a problem in a wireless driver where I was using
> > compare_ether_addr() on potentially unaligned data. Document that
> > compare_ether_addr() is not safe for use everywhere, and add an equivalent
> > function that works regardless of alignment.
> > 
> > Signed-off-by: Daniel Drake <dsd@gentoo.org>
> 
> Patch applied to net-2.6.  Thanks.

Since it turned out that this function wouldn't be useful in
the case that you originally intended it for, I'm going to drop
it until such a time when a new need arises.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

wireless vs. alignment requirements

[Johannes Berg]

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


> David Miller found a problem in a wireless driver where I was using
> compare_ether_addr() on potentially unaligned data. Document that
> compare_ether_addr() is not safe for use everywhere, and add an equivalent
> function that works regardless of alignment.

FWIW, as I've said in the other thread I cannot believe this to be true
since the IP header is required to be aligned anyway. So *iff* this
really was a problem then we'd have much more problems and better fixed
the alignment of the received USB urb in zd1211rw.

Let me explain... This is going to take a bit of text...

Marking optional fields with < > for better readability, you have this
802.11 header layout (including byte sizes):

| FC (2) | durid (2) | A1 (6) | A2 (6) | A3 (6) | seq (2)
...	< optional A4 (6) > < optional qos (2) >
...	< optional HT (4) > < optional mesh (4 or 16) >
(NOTE: not sure about the order of the last two, this will probably
depend on the order in which 11n/11s are accepted! logically I would
expect the order as I wrote it)

(which makes it 24, 26, 30 or 32 bytes long plus the optional 11n/11s
additions which are multiples of four)

After that comes the data which may contain a SNAP header or such and
then the rest. The SNAP header is six bytes plus two byte ethertype, or
there could be a bridge tunnel header with six bytes plus two byte
ethertype, or it could be missing completely, in all of these cases it's
evenly divisible by four.

Now, the IP stack actually assumes that its header is four-byte aligned
(see comment at NET_IP_ALIGN, although it is not said explicitly that
the alignment requirement for an IP header is four) so that is actually
something for the hardware/firmware (!) to do, for example Broadcom
firmware will insert two bytes padding before the PLCP header (6 bytes,
sitting before the 802.11 header) if either the QoS field or A4 was
present (and the header length thus wasn't a multiple of four). Atheros
hardware will insert two bytes padding after the QoS field in the same
cases (IIRC).

Of course, you need to consider the RX header that sits before the
802.11 stuff. Broadcom's is 30 bytes, but if you add the six byte PLCP
header you have 36 bytes which is divisible by four. In the QoS XOR A4
case they add two bytes which makes it 38 with two bytes to spare for
4-byte alignment which are eaten up by the QoS/A4 field.

I don't know about any other hardware but considering that things work
as well as they do I'd think they do similar things.

Hence, going back to the 802.11 header and the IP header alignment
requirement, if we get the IP header alignment requirement right now I
cannot possibly see any way we would use compare_ether_addr() on an
address that is not at least two-byte aligned as required.

Whew. Or so I think.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
>
> Now, the IP stack actually assumes that its header is four-byte aligned
> (see comment at NET_IP_ALIGN, although it is not said explicitly that
> the alignment requirement for an IP header is four) so that is actually
> something for the hardware/firmware (!) to do, for example Broadcom

Good point.  In fact IIRC we've always had the policy that drivers
should do their best to generate aligned packets but it is not a
requirement since on some platforms it's more important for the DMA
to be aligned.

So it's up the platform code to fix up any exceptions should they
show up.

Daniel, what's the specific case that you had in mind with this
patch?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Johannes Berg]

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


> > Now, the IP stack actually assumes that its header is four-byte aligned
> > (see comment at NET_IP_ALIGN, although it is not said explicitly that
> > the alignment requirement for an IP header is four) so that is actually
> > something for the hardware/firmware (!) to do, for example Broadcom
> 
> Good point.  In fact IIRC we've always had the policy that drivers
> should do their best to generate aligned packets but it is not a
> requirement since on some platforms it's more important for the DMA
> to be aligned.

We still require four-byte alignment, no?

> So it's up the platform code to fix up any exceptions should they
> show up.
> 
> Daniel, what's the specific case that you had in mind with this
> patch?

Well. This goes back to a user reporting unaligned accesses on sparc64.
Davem thought this came from the ether addr comparisons but the user
later reported that the patch from davem didn't fix it, and I think
Daniel just made a sweep over all ether addr comparisons replacing them
with unaligned ones.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sat, Nov 24, 2007 at 09:33:36AM +0100, Johannes Berg wrote:
>
> We still require four-byte alignment, no?

Not at all.  If NET_IP_ALIGN is zero then it won't be four-byte
aligned (since the Ethernet header is 14 bytes long).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Johannes Berg]

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


On Sat, 2007-11-24 at 21:32 +0800, Herbert Xu wrote:
> On Sat, Nov 24, 2007 at 09:33:36AM +0100, Johannes Berg wrote:
> >
> > We still require four-byte alignment, no?
> 
> Not at all.  If NET_IP_ALIGN is zero then it won't be four-byte
> aligned (since the Ethernet header is 14 bytes long).

Right. I just didn't think that would be a valid value for an
architecture to set.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Sat, 24 Nov 2007 14:49:36 +0100

> 
> On Sat, 2007-11-24 at 21:32 +0800, Herbert Xu wrote:
> > On Sat, Nov 24, 2007 at 09:33:36AM +0100, Johannes Berg wrote:
> > >
> > > We still require four-byte alignment, no?
> > 
> > Not at all.  If NET_IP_ALIGN is zero then it won't be four-byte
> > aligned (since the Ethernet header is 14 bytes long).
> 
> Right. I just didn't think that would be a valid value for an
> architecture to set.

It is, and explicitly used by powerpc to get more of the
DMA transfers 64-byte aligned which is critical for
performance on some powerpc boxes.

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sat, Nov 24, 2007 at 02:49:36PM +0100, Johannes Berg wrote:
> 
> Right. I just didn't think that would be a valid value for an
> architecture to set.

OK.  Let me clarify this a bit more.  We require at least one
of the following rules to be met:

* the IPv4/IPv6 header is aligned by 8 bytes on reception;
* or the platform provides unaligned exception handlers.

So if your platform violates both rules then it won't work with
the IP stack, simple as that.  Fortunately I don't think such a
platform exists currently on Linux.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Stephen Hemminger]

Herbert Xu wrote:
> On Sat, Nov 24, 2007 at 02:49:36PM +0100, Johannes Berg wrote:
>   
>> Right. I just didn't think that would be a valid value for an
>> architecture to set.
>>     
>
> OK.  Let me clarify this a bit more.  We require at least one
> of the following rules to be met:
>
> * the IPv4/IPv6 header is aligned by 8 bytes on reception;
> * or the platform provides unaligned exception handlers.
>
> So if your platform violates both rules then it won't work with
> the IP stack, simple as that.  Fortunately I don't think such a
> platform exists currently on Linux.
>
> Cheers,
>   

Then what about hardware that can't dma ethernet to non-aligned address.
Sky2 hardware breaks if DMA is not 8 byte aligned.  IMHO the IP stack should
handle any alignment, and do the appropriate memove if the CPU requires 
alignment.

Re: wireless vs. alignment requirements

[Johannes Berg]

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


> Then what about hardware that can't dma ethernet to non-aligned address.
> Sky2 hardware breaks if DMA is not 8 byte aligned.  IMHO the IP stack should
> handle any alignment, and do the appropriate memove if the CPU requires 
> alignment.

Wouldn't that better be handled in the driver rather than having the
test in the generic RX path?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sat, Nov 24, 2007 at 12:11:08PM -0800, Stephen Hemminger wrote:
>
> Then what about hardware that can't dma ethernet to non-aligned address.
> Sky2 hardware breaks if DMA is not 8 byte aligned.  IMHO the IP stack should
> handle any alignment, and do the appropriate memove if the CPU requires 
> alignment.

Luckily all sky2 users have been on x86 so far :)

Here's an idea.  Put the data of the packet into the page frags
where alignment is not an issue but copy the header so that it
is aligned.

Would that work?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Stephen Hemminger]

Herbert Xu wrote:
> On Sat, Nov 24, 2007 at 12:11:08PM -0800, Stephen Hemminger wrote:
>   
>> Then what about hardware that can't dma ethernet to non-aligned address.
>> Sky2 hardware breaks if DMA is not 8 byte aligned.  IMHO the IP stack should
>> handle any alignment, and do the appropriate memove if the CPU requires 
>> alignment.
>>     
>
> Luckily all sky2 users have been on x86 so far :)
>
> Here's an idea.  Put the data of the packet into the page frags
> where alignment is not an issue but copy the header so that it
> is aligned.
>
> Would that work?
>
> Cheers,
>   
No too wasteful. I'm working a patch to eth_type_trans to realign if 
needed for any device.

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sun, Nov 25, 2007 at 01:21:44PM -0800, Stephen Hemminger wrote:
>
> No too wasteful. I'm working a patch to eth_type_trans to realign if 
> needed for any device.

If you're going to do that you can probably compute the checksum
at the same time.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[H. Peter Anvin]

Herbert Xu wrote:
> 
> Luckily all sky2 users have been on x86 so far :)
> 

Hardly so.  My previous employer was MIPS and we used it there (with my 
patch.)

	-hpa

Re: wireless vs. alignment requirements

[H. Peter Anvin]

Stephen Hemminger wrote:
> Herbert Xu wrote:
>> On Sat, Nov 24, 2007 at 02:49:36PM +0100, Johannes Berg wrote:
>>  
>>> Right. I just didn't think that would be a valid value for an
>>> architecture to set.
>>>     
>>
>> OK.  Let me clarify this a bit more.  We require at least one
>> of the following rules to be met:
>>
>> * the IPv4/IPv6 header is aligned by 8 bytes on reception;
>> * or the platform provides unaligned exception handlers.
>>
>> So if your platform violates both rules then it won't work with
>> the IP stack, simple as that.  Fortunately I don't think such a
>> platform exists currently on Linux.
>>
>> Cheers,
>>   
> 
> Then what about hardware that can't dma ethernet to non-aligned address.
> Sky2 hardware breaks if DMA is not 8 byte aligned.  IMHO the IP stack 
> should handle any alignment, and do the appropriate memove if the CPU requires 
> alignment.

I wrote a patch for the IP stack to realign packets if necessary at one 
point.  I should dredge it up again and submit it for collective flamage.

	-hpa

Re: wireless vs. alignment requirements

[Stephen Hemminger]

On Tue, 27 Nov 2007 09:16:07 -0800
"H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:

> Stephen Hemminger wrote:
> > Herbert Xu wrote:
> >> On Sat, Nov 24, 2007 at 02:49:36PM +0100, Johannes Berg wrote:
> >>  
> >>> Right. I just didn't think that would be a valid value for an
> >>> architecture to set.
> >>>     
> >>
> >> OK.  Let me clarify this a bit more.  We require at least one
> >> of the following rules to be met:
> >>
> >> * the IPv4/IPv6 header is aligned by 8 bytes on reception;
> >> * or the platform provides unaligned exception handlers.
> >>
> >> So if your platform violates both rules then it won't work with
> >> the IP stack, simple as that.  Fortunately I don't think such a
> >> platform exists currently on Linux.
> >>
> >> Cheers,
> >>   
> > 
> > Then what about hardware that can't dma ethernet to non-aligned address.
> > Sky2 hardware breaks if DMA is not 8 byte aligned.  IMHO the IP stack 
> > should handle any alignment, and do the appropriate memove if the CPU requires 
> > alignment.
> 
> I wrote a patch for the IP stack to realign packets if necessary at one 
> point.  I should dredge it up again and submit it for collective flamage.
> 
> 	-hpa
> 

Is there any standard kernel config define for "this platform can't do
unaligned accesses"?

-- 
Stephen Hemminger <shemminger-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>

Re: wireless vs. alignment requirements

[H. Peter Anvin]

Stephen Hemminger wrote:
> 
> Is there any standard kernel config define for "this platform can't do
> unaligned accesses"?
> 

Not that I know of, but there should be.

To be somewhat more complexicated, some platforms (e.g. MIPS) can do 
unaligned references quite cheaply, but they need different opcodes to 
do so.

	-hpa

Re: wireless vs. alignment requirements

[Herbert Xu]

On Tue, Nov 27, 2007 at 09:16:07AM -0800, H. Peter Anvin wrote:
> 
> I wrote a patch for the IP stack to realign packets if necessary at one 
> point.  I should dredge it up again and submit it for collective flamage.

As long as it doesn't penalise Ethernet (e.g., the 10Gb crowd :) it would
be good to have.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[H. Peter Anvin]

Herbert Xu wrote:
> On Tue, Nov 27, 2007 at 09:16:07AM -0800, H. Peter Anvin wrote:
>> I wrote a patch for the IP stack to realign packets if necessary at one 
>> point.  I should dredge it up again and submit it for collective flamage.
> 
> As long as it doesn't penalise Ethernet (e.g., the 10Gb crowd :) it would
> be good to have.
> 
> Thanks,

Uhm, most cards affected *ARE* Ethernet cards, due to the bloody 14-byte 
header.

But it doesn't affect architectures which have alignment, and the cost 
of scanning a properly-aligned packet is minimal.

I'll try to find it some time today.

	-hpa

Re: wireless vs. alignment requirements

[Herbert Xu]

On Thu, Nov 29, 2007 at 09:50:35AM -0800, H. Peter Anvin wrote:
>
> Uhm, most cards affected *ARE* Ethernet cards, due to the bloody 14-byte 
> header.

Well most Ethernet drivers are using NET_IP_ALIGN which means that IP
stack gets aligned packets only.

sky2 is the exception here, not the rule.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[H. Peter Anvin]

Herbert Xu wrote:
> On Thu, Nov 29, 2007 at 09:50:35AM -0800, H. Peter Anvin wrote:
>> Uhm, most cards affected *ARE* Ethernet cards, due to the bloody 14-byte 
>> header.
> 
> Well most Ethernet drivers are using NET_IP_ALIGN which means that IP
> stack gets aligned packets only.
> 
> sky2 is the exception here, not the rule.
> 

It is, but it's not unique.  Several USB adapters have the same problem, 
for example.

	-hpa

Re: wireless vs. alignment requirements

[Herbert Xu]

On Thu, Nov 29, 2007 at 04:28:34PM -0800, H. Peter Anvin wrote:
>
> >sky2 is the exception here, not the rule.
> 
> It is, but it's not unique.  Several USB adapters have the same problem, 
> for example.

Notice the common theme here that slow (or slower, i.e., certainly nowhere
near 10Gb) NICs are the norm for violating alignment :)

So I'd prefer something that only penalised them rather than everybody
else.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[H. Peter Anvin]

Herbert Xu wrote:
> On Thu, Nov 29, 2007 at 04:28:34PM -0800, H. Peter Anvin wrote:
>>> sky2 is the exception here, not the rule.
>> It is, but it's not unique.  Several USB adapters have the same problem, 
>> for example.
> 
> Notice the common theme here that slow (or slower, i.e., certainly nowhere
> near 10Gb) NICs are the norm for violating alignment :)
> 
> So I'd prefer something that only penalised them rather than everybody
> else.

Obviously, and this should be a configure option anyway.

	-hpa

Re: wireless vs. alignment requirements

[Johannes Berg]

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


> OK.  Let me clarify this a bit more.  We require at least one
> of the following rules to be met:
> 
> * the IPv4/IPv6 header is aligned by 8 bytes on reception;
> * or the platform provides unaligned exception handlers.
> 
> So if your platform violates both rules then it won't work with
> the IP stack, simple as that.  Fortunately I don't think such a
> platform exists currently on Linux.

Ok, thanks for the clarification.

Eight bytes really sucks for wireless, many things are multiples of four
and QoS vs. non-QoS frames have a multiple of four and common hardware
only adds two padding bytes to get it aligned on four bytes so there's
no easy way to get hardware to align it properly. Hmm.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sat, Nov 24, 2007 at 10:13:19PM +0100, Johannes Berg wrote:
>
> Eight bytes really sucks for wireless, many things are multiples of four
> and QoS vs. non-QoS frames have a multiple of four and common hardware
> only adds two padding bytes to get it aligned on four bytes so there's
> no easy way to get hardware to align it properly. Hmm.

Sorry I was wrong about the 8 bytes requirement.  Although the
IPv6 protocol does try to maintain an 8-byte alignment the Linux
stack never does anything that requires that.

So 4 bytes is enough.

However, the wireless core is definitely not out of the woods.
It needs to support variable hardware header lengths that are
not always a multiple of 4.

So here's my suggestion.  Modify the wireless core to fix up any
packets which aren't aligned correctly.  That should make it
work albeit in a way that's less than optimal.

Then for each driver where you care about this performance
(seriously I wouldn't for the speeds these things run at :),
pick the most common wireless hardware header length and have
the IP (or any other upper-level protocol) header aligned to
at least 4 bytes.  Or better if you know what hardware header
length that you're going to get (e.g., based on what mode you're
in) then do the skb_reserve accordingly.

It's a good thing these things aren't running at 10Gb :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Johannes Berg]

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


> Sorry I was wrong about the 8 bytes requirement.  Although the
> IPv6 protocol does try to maintain an 8-byte alignment the Linux
> stack never does anything that requires that.
> 
> So 4 bytes is enough.

Whew. Good to hear.

> However, the wireless core is definitely not out of the woods.
> It needs to support variable hardware header lengths that are
> not always a multiple of 4.
> 
> So here's my suggestion.  Modify the wireless core to fix up any
> packets which aren't aligned correctly.  That should make it
> work albeit in a way that's less than optimal.

Not sure. On the one hand, yeah, that's something we should probably do,
on the other hand this will suck because for most drivers either nothing
needs to be done or the fixup is trivial. I suppose we should do this
but stick in a WARN_ON_ONCE() or something, at least with mac80211 debug
enabled.

Also, we do plan to run these things on rather smallish embedded devices
like APs that receive a lot of frames from many stations, and with 11n
we're pushing speeds up by quite a bit. I'm wary of putting more code
into the generic receive path.

> Then for each driver where you care about this performance
> (seriously I wouldn't for the speeds these things run at :),
> pick the most common wireless hardware header length and have
> the IP (or any other upper-level protocol) header aligned to
> at least 4 bytes.  Or better if you know what hardware header
> length that you're going to get (e.g., based on what mode you're
> in) then do the skb_reserve accordingly.

Well, you can receive non-QoS frames even in QoS or WPS frames in any
other mode etc. so you can't really make any promises as which will be
more common. Bu in practice all (sane) hardware makes sure things are
aligned properly.

> It's a good thing these things aren't running at 10Gb :)

For sure!

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sun, Nov 25, 2007 at 12:00:28PM +0100, Johannes Berg wrote:
>
> Not sure. On the one hand, yeah, that's something we should probably do,
> on the other hand this will suck because for most drivers either nothing
> needs to be done or the fixup is trivial. I suppose we should do this
> but stick in a WARN_ON_ONCE() or something, at least with mac80211 debug
> enabled.

I don't see how you can get a WARN_ON to work when you're expecting
there to be unaligned packets from time to time as the hardware
header changes.

> Also, we do plan to run these things on rather smallish embedded devices
> like APs that receive a lot of frames from many stations, and with 11n
> we're pushing speeds up by quite a bit. I'm wary of putting more code
> into the generic receive path.

Well you don't have a choice if the hardware header is really
unpredictable.  It's either that or we go and modify the entire
IP stack which penalises all the high-speed Ethernet NICs that
already get the alignment correctly.

> Well, you can receive non-QoS frames even in QoS or WPS frames in any
> other mode etc. so you can't really make any promises as which will be
> more common. Bu in practice all (sane) hardware makes sure things are
> aligned properly.

Here's an idea.  Even if you can't predict the header length of
all packets, can you at least predict the header length of the
majority of data (ones carrying IP etc.) packets?

If so then you can do the skb_reserve based on that and the fix-up
in the wireless core would be minimised.

Since I know next to nothing about the wireless transport layer,
one of you experts will need to tell me whether such a prediction
could work :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Johannes Berg]

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


> > Not sure. On the one hand, yeah, that's something we should probably do,
> > on the other hand this will suck because for most drivers either nothing
> > needs to be done or the fixup is trivial. I suppose we should do this
> > but stick in a WARN_ON_ONCE() or something, at least with mac80211 debug
> > enabled.
> 
> I don't see how you can get a WARN_ON to work when you're expecting
> there to be unaligned packets from time to time as the hardware
> header changes.

Well if the hardware header changes and the hardware is dumb enough not
to do padding I'd expect the driver to fix that up so I don't penalise
hardware that gets it correct.

> > Also, we do plan to run these things on rather smallish embedded devices
> > like APs that receive a lot of frames from many stations, and with 11n
> > we're pushing speeds up by quite a bit. I'm wary of putting more code
> > into the generic receive path.
> 
> Well you don't have a choice if the hardware header is really
> unpredictable.  It's either that or we go and modify the entire
> IP stack which penalises all the high-speed Ethernet NICs that
> already get the alignment correctly.

But I do have a choice where to fix it up and I'd prefer the drivers to
do it where necessary. For that, the warning would work because it'd
show driver authors that they need to fix something.

> Here's an idea.  Even if you can't predict the header length of
> all packets, can you at least predict the header length of the
> majority of data (ones carrying IP etc.) packets?
> 
> If so then you can do the skb_reserve based on that and the fix-up
> in the wireless core would be minimised.
> 
> Since I know next to nothing about the wireless transport layer,
> one of you experts will need to tell me whether such a prediction
> could work :)

Hmm. I don't think so. Take an AP for example. It gets a lot of packets
from stations. Now, if you're not QoS capable then all is well. But i
you are and some stations are as well then all those stations send QoS
packets (+2 bytes). Or take an AP connected via wireless (WPS), WPS has
+6 bytes so I get all incoming upstream traffic with such unaligned
headers.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sun, Nov 25, 2007 at 02:54:24PM +0100, Johannes Berg wrote:
>
> But I do have a choice where to fix it up and I'd prefer the drivers to
> do it where necessary. For that, the warning would work because it'd
> show driver authors that they need to fix something.

Fair enough.

> Hmm. I don't think so. Take an AP for example. It gets a lot of packets
> from stations. Now, if you're not QoS capable then all is well. But i
> you are and some stations are as well then all those stations send QoS
> packets (+2 bytes). Or take an AP connected via wireless (WPS), WPS has
> +6 bytes so I get all incoming upstream traffic with such unaligned
> headers.

The question is does this actually change all the time.  Let's
say you took a random sample of a second worth of IP packets
over wireless, what proportion of them are going to have the
same hardware header length modulo 4?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Johannes Berg]

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


> > Hmm. I don't think so. Take an AP for example. It gets a lot of packets
> > from stations. Now, if you're not QoS capable then all is well. But i
> > you are and some stations are as well then all those stations send QoS
> > packets (+2 bytes). Or take an AP connected via wireless (WPS), WPS has
> > +6 bytes so I get all incoming upstream traffic with such unaligned
> > headers.
> 
> The question is does this actually change all the time.  Let's
> say you took a random sample of a second worth of IP packets
> over wireless, what proportion of them are going to have the
> same hardware header length modulo 4?

I'd think that totally depends on the traffic. If you have a non-QoS AP
with WPS upstream connection, then the traffic to stations will be
four-byte aligned while the WPS upstream will be at a 2-byte-mod-4
boundary. And you'll have all packets from stations come in aligned and
all response packets from wherever come in as WPS.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: wireless vs. alignment requirements

[Herbert Xu]

On Sun, Nov 25, 2007 at 06:04:17PM +0100, Johannes Berg wrote:
>
> I'd think that totally depends on the traffic. If you have a non-QoS AP
> with WPS upstream connection, then the traffic to stations will be
> four-byte aligned while the WPS upstream will be at a 2-byte-mod-4
> boundary. And you'll have all packets from stations come in aligned and
> all response packets from wherever come in as WPS.

OK, sounds like you'll just have to fix them up after DMA.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert-lOAM2aK0SrRLBo1qDEOMRrpzq4S04n8Q@public.gmane.org>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: wireless vs. alignment requirements

[Ulrich Kunitz]

Johannes,

> Hence, going back to the 802.11 header and the IP header alignment
> requirement, if we get the IP header alignment requirement right now I
> cannot possibly see any way we would use compare_ether_addr() on an
> address that is not at least two-byte aligned as required.

ACK. I agree completely.

The problem with the zd1211rw driver is, that it copies the
complete frame received from the device into the SKB and pulls
later the five bytes ZD1211 uses for the PLCP information.
This causes the 802.11 header to be on an odd address. The
reported problems are caused by this.

The zd1211rw-mac80211 is not affected, because the PLCP header is
not copied into the skb and this way the 802.11 header becomes
correctly aligned.

Here is a patch, which should solve the zd1211rw alignment issues.
It compiles, but it is not tested right now, because I got the
idea while writing this e-mail. An official submission will
follow.

Uli

diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index a903645..fb54cd7 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -1166,15 +1166,22 @@ static void do_rx(unsigned long mac_ptr)
 int zd_mac_rx_irq(struct zd_mac *mac, const u8 *buffer, unsigned int length)
 {
 	struct sk_buff *skb;
+	unsigned int length_to_reserve;
 
-	skb = dev_alloc_skb(sizeof(struct zd_rt_hdr) + length);
+	/* This ensures that there is enough place for the radiotap header
+	 * and the the 802.11 header is aligned by four following the
+	 * five-byte ZD1211-specific PLCP header.
+	 */
+	length_to_reserve = ((sizeof(struct zd_rt_hdr) + 3) & ~3) + 3;
+
+	skb = dev_alloc_skb(length_to_reserve + length);
 	if (!skb) {
 		struct ieee80211_device *ieee = zd_mac_to_ieee80211(mac);
 		dev_warn(zd_mac_dev(mac), "Could not allocate skb.\n");
 		ieee->stats.rx_dropped++;
 		return -ENOMEM;
 	}
-	skb_reserve(skb, sizeof(struct zd_rt_hdr));
+	skb_reserve(skb, length_to_reserve);
 	memcpy(__skb_put(skb, length), buffer, length);
 	skb_queue_tail(&mac->rx_queue, skb);
 	tasklet_schedule(&mac->rx_tasklet);

-- 
Uli Kunitz