arm64: mm: define NET_IP_ALIGN to 0
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override NET_IP_ALIGN to 0 to get rid of this padding.
--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/arm64/include/asm/processor.h +8 -0
@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1) +/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0 +#ifndef __ASSEMBLY__#ifdef __KERNEL__
Signature · 1 line
2.19.1
Mark Rutland
reviewerPATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
Quoted 17 lines · Ard Biesheuvel
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +0
-0
arch/arm64/include/asm/processor.h line 33
Quoted 10 lines
arch/arm64/include/asm/processor.h +7
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1) +/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0
Might it be better for core code to do this based on CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?
We already set that for arm64.
Thanks,
Mark.
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
(+ Arnd)
Quoted 21 lines · Mark Rutland
Quoted 20 lines · Ard Biesheuvel
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +0
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1)
Quoted 11 lines
arch/arm64/include/asm/processor.h line 33
Quoted 7 lines
arch/arm64/include/asm/processor.h +7
-0+/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0Might it be better for core code to do this based on
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?
In general, yes. But that will cause performance issues on ARMv6+, since we will end up using ldrd instructions that require alignment fixups.
In my opinion, CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS should signify that unaligned accessors are cheap, not that they are not required at all.
At the moment it was introduced, it could mean either, but in the mean time, Documentation has been added that suggests that CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS means that you can freely cast pointers around and dereference them
For the crypto parts of the kernel, I suggested some fixes for this, but I never got around to following up on the feedback.
https://marc.info/?l=linux-crypto-vger&m=153903336818193&w=2
Mark Rutland
reviewerPATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
Quoted 35 lines · Ard Biesheuvel
(+ Arnd)
Quoted 21 lines · Mark Rutland
Quoted 20 lines · Ard Biesheuvel
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +0
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1)Quoted 10 lines
arch/arm64/include/asm/processor.h line 33
Quoted 7 lines
arch/arm64/include/asm/processor.h +7
-0+/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0Might it be better for core code to do this based on
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?
Quoted 3 lines
In general, yes. But that will cause performance issues on ARMv6+, since we will end up using ldrd instructions that require alignment fixups.
Ok.
Quoted 3 lines
In my opinion,
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESSshould signify that unaligned accessors are cheap, not that they are not required at all.
I'm not sure I follow the distinction you're trying to make.
I do see that *certain* unaligned accesses may be cheap while others are not, so maybe that's worth addressing (e.g. HAVE_EFFICIENT_UNALIGNED_32BIT_ACCESS), but that is a more general thing.
FWIW, for this as-is:
Thanks,
Mark.
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
Quoted 40 lines · Mark Rutland
Quoted 35 lines · Ard Biesheuvel
(+ Arnd)
Quoted 21 lines · Mark Rutland
Quoted 20 lines · Ard Biesheuvel
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +0
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1)Quoted 10 lines
arch/arm64/include/asm/processor.h line 33
Quoted 7 lines
arch/arm64/include/asm/processor.h +7
-0+/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0Might it be better for core code to do this based on
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS?Quoted 3 lines
In general, yes. But that will cause performance issues on ARMv6+, since we will end up using ldrd instructions that require alignment fixups.
Quoted 8 lines
Ok.
Quoted 3 lines
In my opinion,
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESSshould signify that unaligned accessors are cheap, not that they are not required at all.I'm not sure I follow the distinction you're trying to make.
What would be better IMHO is if CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS means that you don't have to go out of your way (e.g., by shifting all ethernet packets by 2 bytes) to avoid having to call get_unaligned_xxx
get_unaligned_xx() is essentially already a noop on most archs that define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, and so there is no overhead in using it in that case. On ARM, however, it makes all the difference, since ldr+ldr tolerates any alignment while ldrd goes through the alignment fixup handler.
Quoted 10 lines
I do see that *certain* unaligned accesses may be cheap while others are not, so maybe that's worth addressing (e.g.
HAVE_EFFICIENT_UNALIGNED_32BIT_ACCESS), but that is a more general thing.FWIW, for this as-is:
Acked-by Mark Rutland <[email protected]>Thanks,
Mark.
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
In reply to Ard BiesheuvelQuoted 17 lines · Ard Biesheuvel
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +0
-0
arch/arm64/include/asm/processor.h line 33
Quoted 10 lines
arch/arm64/include/asm/processor.h +7
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1) +/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0
If you end up refactoring this in the core code as Mark suggested, that's great, but I don't think it should hold this patch up.
Will
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
In reply to Ard BiesheuvelHi,
Quoted 34 lines
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +8
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1) +/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0 +#ifndef __ASSEMBLY__#ifdef __KERNEL__Signature · 1 line
2.19.1
I saw no impact on network performance testing this on socionext synquasher. It makes sense to keep DMA aligned knowing unaligned accesses by the CPU are cheap
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
In reply to Ard BiesheuvelQuoted 8 lines · Ard Biesheuvel
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>
Queued for 4.20. Thanks
Signature · 1 line
Catalin
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
In reply to Ard BiesheuvelHi Ard,
I found that arm64 board that had AVE controller couldn't receive any packets after this change.
AVE controller (hardware) forces to ignore lower 2bits of buffer start address, that is, the address is always aligned with 4bytes.
And more, the controller puts received datas with "2byte headroom" [1].
In case that NET_IP_ALIGN is 2, the driver proceeds skb->data by 2byte with calling netdev_alloc_skb_ip_align(), and then, skb->data is equivalent to start address of data reception.
+0x00 00 buffer start address
+0x01 00
+0x02 XX skb->data == start address of data reception
+0x03 XX
...However, in case that NET_IP_ALIGN is zero, skb->data is the same address as buffer start address, so skb->data differs from start address of data reception. Finally the driver can't receive any packets collectly.
+0x00 00 skb->data == buffer start address
+0x01 00
+0x02 XX start address of data reception
+0x03 XX
...Although I might be able to replace NET_IP_ALIGN with an own macro without calling netdev_alloc_skb_ip_align(), it seems clumsy. Is that any ideas?
---
Best Regards,
Kunihiko Hayashi
Quoted 39 lines · On Wed, 7 Nov 2018 18:10:38 +0100 Ard Biesheuvel <[email protected]>…
On arm64, there is no need to add 2 bytes of padding to the start of each network buffer just to make the IP header appear 32-bit aligned.
Since this might actually adversely affect DMA performance some platforms, let's override
NET_IP_ALIGNto 0 to get rid of this padding.Signed-off-by Ard Biesheuvel <[email protected]>--- arch/arm64/include/asm/processor.h | 8 ++++++++ 1 file changed, 8 insertions(+)arch/arm64/include/asm/processor.h +8
-0@@ -24,6 +24,14 @@#define KERNEL_DS UL(-1)#define USER_DS (TASK_SIZE_64 - 1) +/* +* On arm64 systems, unaligned accesses by the CPU are cheap, and so there is +* no point in shifting all network buffers by 2 bytes just to make some IP +* header fields appear aligned in memory, potentially sacrificing some DMA +* performance on some platforms. +*/ +#define NET_IP_ALIGN 0 +#ifndef __ASSEMBLY__#ifdef __KERNEL__Signature · 7 lines
2.19.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
Hello Kunihiko,
Quoted 32 lines · Kunihiko Hayashi
Hi Ard,
I found that arm64 board that had AVE controller couldn't receive any packets after this change.
AVE controller (hardware) forces to ignore lower 2bits of buffer start address, that is, the address is always aligned with 4bytes.
And more, the controller puts received datas with "2byte headroom" [1].In case that
NET_IP_ALIGNis 2, the driver proceedsskb->databy 2byte with callingnetdev_alloc_skb_ip_align(), and then,skb->datais equivalent to start address of data reception.+0x00 00 buffer start address +0x01 00 +0x02 XX skb->data == start address of data reception +0x03 XX ...However, in case that
NET_IP_ALIGNis zero,skb->datais the same address as buffer start address, soskb->datadiffers from start address of data reception. Finally the driver can't receive any packets collectly.+0x00 00 skb->data == buffer start address +0x01 00 +0x02 XX start address of data reception +0x03 XX ...
Quoted 4 lines
Although I might be able to replace
NET_IP_ALIGNwith an own macro without callingnetdev_alloc_skb_ip_align(), it seems clumsy. Is that any ideas?
If your hardware assumes NET_IP_ALIGN == 2, then yes, I suggest you introduce your own macros here instead.
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
Hi Ard,
Quoted 36 lines · On Wed, 28 Nov 2018 12:58:54 +0100 <[email protected]> wrote:
Hello Kunihiko,
Quoted 32 lines · Kunihiko Hayashi
Hi Ard,
I found that arm64 board that had AVE controller couldn't receive any packets after this change.
AVE controller (hardware) forces to ignore lower 2bits of buffer start address, that is, the address is always aligned with 4bytes.
And more, the controller puts received datas with "2byte headroom" [1].In case that
NET_IP_ALIGNis 2, the driver proceedsskb->databy 2byte with callingnetdev_alloc_skb_ip_align(), and then,skb->datais equivalent to start address of data reception.+0x00 00 buffer start address +0x01 00 +0x02 XX skb->data == start address of data reception +0x03 XX ...However, in case that
NET_IP_ALIGNis zero,skb->datais the same address as buffer start address, soskb->datadiffers from start address of data reception. Finally the driver can't receive any packets collectly.+0x00 00 skb->data == buffer start address +0x01 00 +0x02 XX start address of data reception +0x03 XX ...
Quoted 7 lines
Quoted 4 lines
Although I might be able to replace
NET_IP_ALIGNwith an own macro without callingnetdev_alloc_skb_ip_align(), it seems clumsy. Is that any ideas?If your hardware assumes
NET_IP_ALIGN== 2, then yes, I suggest you introduce your own macros here instead.
Okay, I understand that NET_IP_ALIGN isn't a hardware specific value. I'll try to introduce my own macro as a hardware specific value.
Thank you,
---
Best Regards,
Kunihiko Hayashi
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
In reply to Kunihiko HayashiHi Kunihiko,
Quoted 8 lines · Kunihiko Hayashi
I found that arm64 board that had AVE controller couldn't receive any packets after this change.
AVE controller (hardware) forces to ignore lower 2bits of buffer start address, that is, the address is always aligned with 4bytes.
And more, the controller puts received datas with "2byte headroom" [1].
Is this "2-byte headroom" a fixed property of the hardware? If so, it seems like you should just be using 2 instead of NET_IP_ALIGN when calculating the DMA address of the skb buffer.
Will
PATCH 1/1 arm64: mm: define NET_IP_ALIGN to 0
Hi Will,
Quoted 10 lines · On Wed, 28 Nov 2018 12:04:56 +0000 <[email protected]> wrote:
Hi Kunihiko,
Quoted 7 lines · Kunihiko Hayashi
I found that arm64 board that had AVE controller couldn't receive any packets after this change.
AVE controller (hardware) forces to ignore lower 2bits of buffer start address, that is, the address is always aligned with 4bytes.
And more, the controller puts received datas with "2byte headroom" [1].
Quoted 5 lines
Quoted 1 line
Is this "2-byte headroom" a fixed property of the hardware? If so, it seems like you should just be using 2 instead of
NET_IP_ALIGNwhen calculating the DMA address of the skb buffer.
Yes, 2-byte headroom is a hardware specific property.
I've already calculated the DMA address with NET_IP_ALIGN, so I'll replace it with my own macro as a hardware specific value.
Thank you,
---
Best Regards,
Kunihiko Hayashi