[dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Bruce Richardson]

When including the rte_ether.h header in applications with warnings
enabled, a warning was given because of the assumption of 2-byte alignment
of ethernet addresses when processing them.

.../include/rte_ether.h:149:2: warning: converting a packed ‘const
  struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
  {aka ‘const short unsigned int’} pointer (alignment 2) may result in
  an unaligned pointer value [-Waddress-of-packed-member]
149 |  const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
    |  ^~~~~

Since ethernet addresses should always be aligned on a two-byte boundary,
we can just inform the compiler of this assumption to remove the warnings
and allow us to always access the addresses using 16-bit operations.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---

Although this is an ABI break, the network structures are all being renamed
in this release, and a deprecation notice was previously posted for it.
---
 lib/librte_net/rte_ether.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 3a87ff184..8090b7c01 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -55,7 +55,8 @@ extern "C" {
  * See http://standards.ieee.org/regauth/groupmac/tutorial.html
  */
 struct ether_addr {
-	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
+	/** Addr bytes in tx order */
+	uint8_t addr_bytes[ETHER_ADDR_LEN] __rte_aligned(2);
 } __attribute__((__packed__));
 
 #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
-- 
2.21.0

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Kevin Traynor]

On 16/05/2019 16:54, Bruce Richardson wrote:
> When including the rte_ether.h header in applications with warnings
> enabled, a warning was given because of the assumption of 2-byte alignment
> of ethernet addresses when processing them.
> 
> .../include/rte_ether.h:149:2: warning: converting a packed ‘const
>   struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
>   {aka ‘const short unsigned int’} pointer (alignment 2) may result in
>   an unaligned pointer value [-Waddress-of-packed-member]
> 149 |  const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
>     |  ^~~~~
> 

Hi - There was a couple of these warnings in telemetry that were not
squashed with the patch to disable the warning in 19.05. I was just
about send a patch to address that when I saw this one.

As your patch is aiming for a better solution, I won't send my patch for
master but I'll still send to stable.

> Since ethernet addresses should always be aligned on a two-byte boundary,
> we can just inform the compiler of this assumption to remove the warnings
> and allow us to always access the addresses using 16-bit operations.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
> 
> Although this is an ABI break, the network structures are all being renamed
> in this release, and a deprecation notice was previously posted for it.
> ---
>  lib/librte_net/rte_ether.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 3a87ff184..8090b7c01 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -55,7 +55,8 @@ extern "C" {
>   * See http://standards.ieee.org/regauth/groupmac/tutorial.html
>   */
>  struct ether_addr {
> -	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
> +	/** Addr bytes in tx order */
> +	uint8_t addr_bytes[ETHER_ADDR_LEN] __rte_aligned(2);
>  } __attribute__((__packed__));
>  
>  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
> 

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Bruce Richardson]

On Thu, May 16, 2019 at 07:04:03PM +0100, Kevin Traynor wrote:
> On 16/05/2019 16:54, Bruce Richardson wrote:
> > When including the rte_ether.h header in applications with warnings
> > enabled, a warning was given because of the assumption of 2-byte alignment
> > of ethernet addresses when processing them.
> > 
> > .../include/rte_ether.h:149:2: warning: converting a packed ‘const
> >   struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
> >   {aka ‘const short unsigned int’} pointer (alignment 2) may result in
> >   an unaligned pointer value [-Waddress-of-packed-member]
> > 149 |  const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
> >     |  ^~~~~
> > 
> 
> Hi - There was a couple of these warnings in telemetry that were not
> squashed with the patch to disable the warning in 19.05. I was just
> about send a patch to address that when I saw this one.
> 
> As your patch is aiming for a better solution, I won't send my patch for
> master but I'll still send to stable.
> 
Good idea, thanks.

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Olivier Matz]

Hi Bruce,

On Thu, May 16, 2019 at 04:54:57PM +0100, Bruce Richardson wrote:
> When including the rte_ether.h header in applications with warnings
> enabled, a warning was given because of the assumption of 2-byte alignment
> of ethernet addresses when processing them.
> 
> .../include/rte_ether.h:149:2: warning: converting a packed ‘const
>   struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
>   {aka ‘const short unsigned int’} pointer (alignment 2) may result in
>   an unaligned pointer value [-Waddress-of-packed-member]
> 149 |  const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
>     |  ^~~~~
> 
> Since ethernet addresses should always be aligned on a two-byte boundary,

I'm a bit reserved about this last assumption. The ethernet address
structure may be used in a private structure, whose alignment is 1. Are
we sure that there is no (funny) protocol that carries unaligned
ethernet addresses?

Shouldn't we change the definition of unaligned_uint16_t instead?
Or change the rte_is_broadcast_ether_addr() function?


> we can just inform the compiler of this assumption to remove the warnings
> and allow us to always access the addresses using 16-bit operations.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
> 
> Although this is an ABI break, the network structures are all being renamed
> in this release, and a deprecation notice was previously posted for it.

Yes, but the network renaming is identified in the release note as an
API break, not an ABI break.


> ---
>  lib/librte_net/rte_ether.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 3a87ff184..8090b7c01 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -55,7 +55,8 @@ extern "C" {
>   * See http://standards.ieee.org/regauth/groupmac/tutorial.html
>   */
>  struct ether_addr {
> -	uint8_t addr_bytes[ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
> +	/** Addr bytes in tx order */
> +	uint8_t addr_bytes[ETHER_ADDR_LEN] __rte_aligned(2);
>  } __attribute__((__packed__));
>  
>  #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
> -- 
> 2.21.0
> 

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Bruce Richardson]

On Mon, Jul 01, 2019 at 03:11:12PM +0200, Olivier Matz wrote:
> Hi Bruce,
> 
> On Thu, May 16, 2019 at 04:54:57PM +0100, Bruce Richardson wrote:
> > When including the rte_ether.h header in applications with warnings
> > enabled, a warning was given because of the assumption of 2-byte alignment
> > of ethernet addresses when processing them.
> > 
> > .../include/rte_ether.h:149:2: warning: converting a packed ‘const
> >   struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
> >   {aka ‘const short unsigned int’} pointer (alignment 2) may result in
> >   an unaligned pointer value [-Waddress-of-packed-member]
> > 149 |  const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
> >     |  ^~~~~
> > 
> > Since ethernet addresses should always be aligned on a two-byte boundary,
> 
> I'm a bit reserved about this last assumption. The ethernet address
> structure may be used in a private structure, whose alignment is 1. Are
> we sure that there is no (funny) protocol that carries unaligned
> ethernet addresses?
> 
> Shouldn't we change the definition of unaligned_uint16_t instead?
> Or change the rte_is_broadcast_ether_addr() function?
> 

We could, but I believe the correct behaviour is to make the addresses
always 2-byte aligned, unless someone actually has a real-world case where
there is a protocol that doesn't have the data 2-byte aligned.

/Bruce

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Olivier Matz]

Hi Bruce,

On Mon, Jul 01, 2019 at 02:38:43PM +0100, Bruce Richardson wrote:
> On Mon, Jul 01, 2019 at 03:11:12PM +0200, Olivier Matz wrote:
> > Hi Bruce,
> > 
> > On Thu, May 16, 2019 at 04:54:57PM +0100, Bruce Richardson wrote:
> > > When including the rte_ether.h header in applications with warnings
> > > enabled, a warning was given because of the assumption of 2-byte alignment
> > > of ethernet addresses when processing them.
> > > 
> > > .../include/rte_ether.h:149:2: warning: converting a packed ‘const
> > >   struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
> > >   {aka ‘const short unsigned int’} pointer (alignment 2) may result in
> > >   an unaligned pointer value [-Waddress-of-packed-member]
> > > 149 |  const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea;
> > >     |  ^~~~~
> > > 
> > > Since ethernet addresses should always be aligned on a two-byte boundary,
> > 
> > I'm a bit reserved about this last assumption. The ethernet address
> > structure may be used in a private structure, whose alignment is 1. Are
> > we sure that there is no (funny) protocol that carries unaligned
> > ethernet addresses?
> > 
> > Shouldn't we change the definition of unaligned_uint16_t instead?
> > Or change the rte_is_broadcast_ether_addr() function?
> > 
> 
> We could, but I believe the correct behaviour is to make the addresses
> always 2-byte aligned, unless someone actually has a real-world case where
> there is a protocol that doesn't have the data 2-byte aligned.

Maybe you missed that part of my previous answer, I'm copy it again here:

  > Although this is an ABI break, the network structures are all being renamed
  > in this release, and a deprecation notice was previously posted for it.

  Yes, but the network renaming is identified in the release note as an
  API break, not an ABI break.

I thought we agreed to limit ABI breakages to cases where there is no
other solution. Here, this is surely a "small" ABI breakage, but I
suppose there is a way to do differently.

If we really want to do that way, it's better to announce it as an ABI
break.

Olivier

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Bruce Richardson]

On Mon, Jul 01, 2019 at 04:14:30PM +0200, Olivier Matz wrote:
> Hi Bruce,
> 
> On Mon, Jul 01, 2019 at 02:38:43PM +0100, Bruce Richardson wrote:
> > On Mon, Jul 01, 2019 at 03:11:12PM +0200, Olivier Matz wrote:
> > > Hi Bruce,
> > > 
> > > On Thu, May 16, 2019 at 04:54:57PM +0100, Bruce Richardson wrote:
> > > > When including the rte_ether.h header in applications with warnings
> > > > enabled, a warning was given because of the assumption of 2-byte
> > > > alignment of ethernet addresses when processing them.
> > > > 
> > > > .../include/rte_ether.h:149:2: warning: converting a packed ‘const
> > > > struct ether_addr’ pointer (alignment 1) to a ‘unaligned_uint16_t’
> > > > {aka ‘const short unsigned int’} pointer (alignment 2) may result
> > > > in an unaligned pointer value [-Waddress-of-packed-member] 149 |
> > > > const unaligned_uint16_t *ea_words = (const unaligned_uint16_t
> > > > *)ea; |  ^~~~~
> > > > 
> > > > Since ethernet addresses should always be aligned on a two-byte
> > > > boundary,
> > > 
> > > I'm a bit reserved about this last assumption. The ethernet address
> > > structure may be used in a private structure, whose alignment is 1.
> > > Are we sure that there is no (funny) protocol that carries unaligned
> > > ethernet addresses?
> > > 
> > > Shouldn't we change the definition of unaligned_uint16_t instead?  Or
> > > change the rte_is_broadcast_ether_addr() function?
> > > 
> > 
> > We could, but I believe the correct behaviour is to make the addresses
> > always 2-byte aligned, unless someone actually has a real-world case
> > where there is a protocol that doesn't have the data 2-byte aligned.
> 
> Maybe you missed that part of my previous answer, I'm copy it again here:
> 
>   > Although this is an ABI break, the network structures are all being
>   > renamed in this release, and a deprecation notice was previously
>   > posted for it.
> 
>   Yes, but the network renaming is identified in the release note as an
>   API break, not an ABI break.
> 
> I thought we agreed to limit ABI breakages to cases where there is no
> other solution. Here, this is surely a "small" ABI breakage, but I
> suppose there is a way to do differently.
> 
> If we really want to do that way, it's better to announce it as an ABI
> break.
>
At this stage, I'm ok with pushing this to 19.11 to follow the official
deprecation process. 

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Bruce Richardson]

On Wed, Feb 05, 2020 at 01:21:54AM +0100, Martins Eglitis wrote:
> Dear Bruce and Kevin,
> 
> I tried building an application (NFF-GO) which has DPDK as a dependency.
> I am still getting the same warnings you and Kevin were discussing. My
> current DPDK version is 19.11-1.
> 
> Do you know if this issue has been resolved?
> 
> This is the output:
> 
> # github.com/intel-go/nff-go/internal/low
> In file included from
> /home/zebra/Projects/nff-go/dpdk/dpdk/x86_64-native-linuxapp-gcc-install/usr/local/share/dpdk/x86_64-native-linuxapp-gcc/include/rte_ethdev.h:160,
>                  from
> ../../go/pkg/mod/github.com/intel-go/nff-go@v0.9.1/internal/low/low.h:11,
>                  from
> ../../go/pkg/mod/github.com/intel-go/nff-go@v0.9.1/internal/low/low.go:16:
> /home/zebra/Projects/nff-go/dpdk/dpdk/x86_64-native-linuxapp-gcc-install/usr/local/share/dpdk/x86_64-native-linuxapp-gcc/include/rte_ether.h:
> In function ‘rte_is_same_ether_addr’:
> /home/zebra/Projects/nff-go/dpdk/dpdk/x86_64-native-linuxapp-gcc-install/usr/local/share/dpdk/x86_64-native-linuxapp-gcc/include/rte_ether.h:84:2:
> warning: converting a packed ‘const struct rte_ether_addr’ pointer
> (alignment 1) to a ‘unaligned_uint16_t’ {aka ‘const short unsigned int’}
> pointer (alignment 2) may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>    84 |  const unaligned_uint16_t *w1 = (const uint16_t *)ea1;
>       |  ^~~~~
<snip>
> 

Hi,

looking at the code in DPDK for 19.11, rte_ether.h no longer has the
unaligned_uint16_t type in rte_ether.h. For example, line 84 of rte_ether.h
should read as below:

 81 static inline int rte_is_same_ether_addr(const struct rte_ether_addr *ea1,
 82                                      const struct rte_ether_addr *ea2)
 83 {
 84         const uint16_t *w1 = (const uint16_t *)ea1;
 85         const uint16_t *w2 = (const uint16_t *)ea2;
 86

Have you got mixed header files from two different DPDK releases?

/Bruce

Re: [dpdk-dev] [PATCH] ether: mark ethernet addresses as being 2-byte aligned

[Martins Eglitis]

Hi,

Thank you for the help. I think it has something to do how the NFF-Go is 
being built. Will address the question to NFF-Go devs.

Thank you,
Martins

On 2020-02-05 14:45, Bruce Richardson wrote:
> On Wed, Feb 05, 2020 at 01:21:54AM +0100, Martins Eglitis wrote:
>> Dear Bruce and Kevin,
>>
>> I tried building an application (NFF-GO) which has DPDK as a dependency.
>> I am still getting the same warnings you and Kevin were discussing. My
>> current DPDK version is 19.11-1.
>>
>> Do you know if this issue has been resolved?
>>
>> This is the output:
>>
>> # github.com/intel-go/nff-go/internal/low
>> In file included from
>> /home/zebra/Projects/nff-go/dpdk/dpdk/x86_64-native-linuxapp-gcc-install/usr/local/share/dpdk/x86_64-native-linuxapp-gcc/include/rte_ethdev.h:160,
>>                   from
>> ../../go/pkg/mod/github.com/intel-go/nff-go@v0.9.1/internal/low/low.h:11,
>>                   from
>> ../../go/pkg/mod/github.com/intel-go/nff-go@v0.9.1/internal/low/low.go:16:
>> /home/zebra/Projects/nff-go/dpdk/dpdk/x86_64-native-linuxapp-gcc-install/usr/local/share/dpdk/x86_64-native-linuxapp-gcc/include/rte_ether.h:
>> In function ‘rte_is_same_ether_addr’:
>> /home/zebra/Projects/nff-go/dpdk/dpdk/x86_64-native-linuxapp-gcc-install/usr/local/share/dpdk/x86_64-native-linuxapp-gcc/include/rte_ether.h:84:2:
>> warning: converting a packed ‘const struct rte_ether_addr’ pointer
>> (alignment 1) to a ‘unaligned_uint16_t’ {aka ‘const short unsigned int’}
>> pointer (alignment 2) may result in an unaligned pointer value
>> [-Waddress-of-packed-member]
>>     84 |  const unaligned_uint16_t *w1 = (const uint16_t *)ea1;
>>        |  ^~~~~
> <snip>
> Hi,
>
> looking at the code in DPDK for 19.11, rte_ether.h no longer has the
> unaligned_uint16_t type in rte_ether.h. For example, line 84 of rte_ether.h
> should read as below:
>
>   81 static inline int rte_is_same_ether_addr(const struct rte_ether_addr *ea1,
>   82                                      const struct rte_ether_addr *ea2)
>   83 {
>   84         const uint16_t *w1 = (const uint16_t *)ea1;
>   85         const uint16_t *w2 = (const uint16_t *)ea2;
>   86
>
> Have you got mixed header files from two different DPDK releases?
>
> /Bruce

-- 
Best regards,
Martins Eglitis