[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[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_ALIGN to 0 to get rid of this
padding.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/processor.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 3e2091708b8e..6b0d4dff5012 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -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__
 
-- 
2.19.1

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[Mark Rutland]

On Wed, Nov 07, 2018 at 06:10:38PM +0100, Ard Biesheuvel wrote:
> 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/processor.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 3e2091708b8e..6b0d4dff5012 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -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] arm64: mm: define NET_IP_ALIGN to 0

[Ard Biesheuvel]

(+ Arnd)

On 7 November 2018 at 19:10, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Nov 07, 2018 at 06:10:38PM +0100, Ard Biesheuvel wrote:
>> 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.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/include/asm/processor.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> index 3e2091708b8e..6b0d4dff5012 100644
>> --- a/arch/arm64/include/asm/processor.h
>> +++ b/arch/arm64/include/asm/processor.h
>> @@ -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?
>

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

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[Mark Rutland]

On Thu, Nov 08, 2018 at 12:18:33AM +0100, Ard Biesheuvel wrote:
> (+ Arnd)
> 
> On 7 November 2018 at 19:10, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Nov 07, 2018 at 06:10:38PM +0100, Ard Biesheuvel wrote:
> >> 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.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  arch/arm64/include/asm/processor.h | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> >> index 3e2091708b8e..6b0d4dff5012 100644
> >> --- a/arch/arm64/include/asm/processor.h
> >> +++ b/arch/arm64/include/asm/processor.h
> >> @@ -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?
> 
> In general, yes. But that will cause performance issues on ARMv6+,
> since we will end up using ldrd instructions that require alignment
> fixups.

Ok.

> In my opinion, CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS should 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:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[Ard Biesheuvel]

On 8 November 2018 at 11:06, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Nov 08, 2018 at 12:18:33AM +0100, Ard Biesheuvel wrote:
>> (+ Arnd)
>>
>> On 7 November 2018 at 19:10, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Wed, Nov 07, 2018 at 06:10:38PM +0100, Ard Biesheuvel wrote:
>> >> 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.
>> >>
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >> ---
>> >>  arch/arm64/include/asm/processor.h | 8 ++++++++
>> >>  1 file changed, 8 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> >> index 3e2091708b8e..6b0d4dff5012 100644
>> >> --- a/arch/arm64/include/asm/processor.h
>> >> +++ b/arch/arm64/include/asm/processor.h
>> >> @@ -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?
>>
>> In general, yes. But that will cause performance issues on ARMv6+,
>> since we will end up using ldrd instructions that require alignment
>> fixups.
>
> Ok.
>
>> In my opinion, CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS should 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.


> 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 <mark.rutland@arm.com>
>
> Thanks,
> Mark.

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[Will Deacon]

On Wed, Nov 07, 2018 at 06:10:38PM +0100, Ard Biesheuvel wrote:
> 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/processor.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 3e2091708b8e..6b0d4dff5012 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -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

Acked-by: Will Deacon <will.deacon@arm.com>

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] arm64: mm: define NET_IP_ALIGN to 0

[Ilias Apalodimas]

Hi, 
> 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/processor.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 3e2091708b8e..6b0d4dff5012 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -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__
>  
> -- 
> 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

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[Catalin Marinas]

On Wed, Nov 07, 2018 at 06:10:38PM +0100, Ard Biesheuvel wrote:
> 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Queued for 4.20. Thanks

-- 
Catalin

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[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].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/socionext/sni_ave.c#n591

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


On Wed, 7 Nov 2018 18:10:38 +0100
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/processor.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 3e2091708b8e..6b0d4dff5012 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -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__
>  
> -- 
> 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] arm64: mm: define NET_IP_ALIGN to 0

[Ard Biesheuvel]

Hello Kunihiko,

On Wed, 28 Nov 2018 at 07:02, Kunihiko Hayashi
<hayashi.kunihiko@socionext.com> wrote:
>
> 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].
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/socionext/sni_ave.c#n591
>
> 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?
>

If your hardware assumes NET_IP_ALIGN == 2, then yes, I suggest you
introduce your own macros here instead.

[PATCH] arm64: mm: define NET_IP_ALIGN to 0

[Kunihiko Hayashi]

Hi Ard,

On Wed, 28 Nov 2018 12:58:54 +0100 <ard.biesheuvel@linaro.org> wrote:

> Hello Kunihiko,
> 
> On Wed, 28 Nov 2018 at 07:02, Kunihiko Hayashi
> <hayashi.kunihiko@socionext.com> wrote:
> >
> > 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].
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/socionext/sni_ave.c#n591
> >
> > 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?
> >
> 
> 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] arm64: mm: define NET_IP_ALIGN to 0

[Will Deacon]

Hi Kunihiko,

On Wed, Nov 28, 2018 at 03:02:25PM +0900, Kunihiko Hayashi wrote:
> 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].
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/socionext/sni_ave.c#n591

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] arm64: mm: define NET_IP_ALIGN to 0

[Kunihiko Hayashi]

Hi Will,

On Wed, 28 Nov 2018 12:04:56 +0000 <will.deacon@arm.com> wrote:

> Hi Kunihiko,
> 
> On Wed, Nov 28, 2018 at 03:02:25PM +0900, Kunihiko Hayashi wrote:
> > 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].
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/socionext/sni_ave.c#n591
> 
> 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.

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