Hi Joe,

On Tue, Jul 18, 2017 at 01:10:59PM -0500, Joe Hershberger wrote:

On Wed, Jul 12, 2017 at 9:34 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:

...

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

It seems like gcc 7.1 is a bit stricter than previous gcc versions on this, and using it lead to data abort for unaligned accesses when generating network traffic.

Fix this by adding the packed attribute to the ip_udp_hdr structure in order to let GCC do its job.

Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

include/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h index 997db9210a8f..7b815afffafa 100644 --- a/include/net.h +++ b/include/net.h @@ -390,7 +390,7 @@ struct ip_udp_hdr { u16 udp_dst; /* UDP destination port */ u16 udp_len; /* Length of UDP packet */ u16 udp_xsum; /* Checksum */ -}; +} __attribute__ ((packed));

Do you have an example of why this is unaligned?

You can have the discussion that led to this patch in "Data abort with gcc 7.1", started a week ago.

It seems that the structure itself is naturally packed (each element is aligned to its access size).

That's true.

It seems the only time this would hit a dabort is if the head of the buffer is not 32-bit aligned. Maybe we should address the place where that is the case instead of forcing byte-wise accesses in general for this structure?

That's exactly what happens, the pointer to the ip_up_hdr is not aligned on 32 bits, and triggers an alignment error.

However, I'm not sure how feasible it would be to align the IP packets on 32-bits, since the Ethernet header is only 14 bytes, right? We could use a bounce buffer for each packet, but that's not really optimized either.

Maxime

Hi,

On 07/18/2017 08:10 PM, Joe Hershberger wrote:

Hi Maxime,

On Wed, Jul 12, 2017 at 9:34 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:

...

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

It seems like gcc 7.1 is a bit stricter than previous gcc versions on this, and using it lead to data abort for unaligned accesses when generating network traffic.

Fix this by adding the packed attribute to the ip_udp_hdr structure in order to let GCC do its job.

Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

include/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h index 997db9210a8f..7b815afffafa 100644 --- a/include/net.h +++ b/include/net.h @@ -390,7 +390,7 @@ struct ip_udp_hdr { u16 udp_dst; /* UDP destination port */ u16 udp_len; /* Length of UDP packet */ u16 udp_xsum; /* Checksum */ -}; +} __attribute__ ((packed));

Do you have an example of why this is unaligned? It seems that the structure itself is naturally packed (each element is aligned to its access size). It seems the only time this would hit a dabort is if the head of the buffer is not 32-bit aligned. Maybe we should address the place where that is the case instead of forcing byte-wise accesses in general for this structure?

|Perhaps __attribute__((aligned(2))) can prevent byte wise accesses? Regards, Jeroen |

Hello Siarhei,

On 07/21/2017 08:46 PM, Siarhei Siamashka wrote:

On Wed, 19 Jul 2017 20:26:54 +0200 Jeroen Hofstee jeroen@myspectrum.nl wrote:

...

Hi,

On 07/18/2017 08:10 PM, Joe Hershberger wrote:

...

Hi Maxime,

On Wed, Jul 12, 2017 at 9:34 AM, Maxime Ripard maxime.ripard@free-electrons.com wrote:

...

The -mno-unaligned-access flag used on ARM to prevent GCC from generating unaligned accesses (obviously) will only do so on packed structures.

It seems like gcc 7.1 is a bit stricter than previous gcc versions on this, and using it lead to data abort for unaligned accesses when generating network traffic.

Fix this by adding the packed attribute to the ip_udp_hdr structure in order to let GCC do its job.

Signed-off-by: Maxime Ripard maxime.ripard@free-electrons.com

include/net.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net.h b/include/net.h index 997db9210a8f..7b815afffafa 100644 --- a/include/net.h +++ b/include/net.h @@ -390,7 +390,7 @@ struct ip_udp_hdr { u16 udp_dst; /* UDP destination port */ u16 udp_len; /* Length of UDP packet */ u16 udp_xsum; /* Checksum */ -}; +} __attribute__ ((packed));

Do you have an example of why this is unaligned? It seems that the structure itself is naturally packed (each element is aligned to its access size). It seems the only time this would hit a dabort is if the head of the buffer is not 32-bit aligned. Maybe we should address the place where that is the case instead of forcing byte-wise accesses in general for this structure?

|Perhaps __attribute__((aligned(2))) can prevent byte wise accesses? Regards, Jeroen |

https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html

It says that "The aligned attribute can only increase alignment".

yes, __attribute__((packed, aligned(2))); would be needed it seems. To first force alignment of 1 (which packed does) and thereafter increase it to 2 again.

That _might_ prevent the 32 bit stores, and use 16 bit ones instead where possible / not forcing byte accesses everywhere. Completely untested though ;)

Regards, Jeroen