[dpdk-dev] [PATCH v4 5/8] net/ether: mark ethernet addresses as being 2-byte aligned

[Richardson, Bruce]

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Friday, June 7, 2019 6:00 PM
> To: Stephen Hemminger <stephen at networkplumber.org>; dev at dpdk.org
> Cc: Richardson, Bruce <bruce.richardson at intel.com>; Andrew Rybchenko
> <arybchenko at solarflare.com>
> Subject: RE: [dpdk-dev] [PATCH v4 5/8] net/ether: mark ethernet addresses
> as being 2-byte aligned
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Wednesday, June 5, 2019 7:10 PM
> > To: dev at dpdk.org
> > Cc: Richardson, Bruce <bruce.richardson at intel.com>; Stephen Hemminger
> > <stephen at networkplumber.org>; Andrew Rybchenko
> > <arybchenko at solarflare.com>
> > Subject: [dpdk-dev] [PATCH v4 5/8] net/ether: mark ethernet addresses
> > as being 2-byte aligned
> >
> > From: Bruce Richardson <bruce.richardson at intel.com>
> >
> > 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 at intel.com>
> > Signed-off-by: Stephen Hemminger <stephen at networkplumber.org>
> > Reviewed-by: Andrew Rybchenko <arybchenko at solarflare.com>
> > ---
> >  lib/librte_net/rte_ether.h | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index feb35a33c94b..d7b76ddf63eb 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -58,7 +58,8 @@ extern "C" {
> >   * See http://standards.ieee.org/regauth/groupmac/tutorial.html
> >   */
> >  struct rte_ether_addr {
> > -	uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx order
> */
> > +	uint8_t addr_bytes[RTE_ETHER_ADDR_LEN] __rte_aligned(2);
> > +	/**< Addr bytes in tx order */
> >  } __attribute__((__packed__));
> 
> Hmm, that would change layout of any struct/union that has struct
> rte_ether_addr inside.
> So seems like implicit ABI breakage to me.
> Konstantin
> 

I suppose it could, though only if you had the structure starting at an odd byte
inside the larger structure. Personally, I'd think it should be ok in just about
all cases, as I'd find it hard to imagine when you wouldn't have this, but 
technically I suppose it's a break. For real packet heads the fields will always be
two-byte aligned so there is no issue.

/Bruce