From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 58318A0487 for ; Fri, 5 Jul 2019 16:34:17 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D9B3F1B957; Fri, 5 Jul 2019 16:34:15 +0200 (CEST) Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 607BC5689 for ; Fri, 5 Jul 2019 16:34:14 +0200 (CEST) Received: from lfbn-lil-1-176-160.w90-45.abo.wanadoo.fr ([90.45.26.160] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hjPKu-00051M-6N; Fri, 05 Jul 2019 16:37:17 +0200 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 05 Jul 2019 16:34:08 +0200 Date: Fri, 5 Jul 2019 16:34:08 +0200 From: Olivier Matz To: Stephen Hemminger Cc: dev@dpdk.org, Bruce Richardson , Andrew Rybchenko Message-ID: <20190705143408.hm4jdvtxkdny27u3@platinum> References: <20190516180427.17270-1-stephen@networkplumber.org> <20190702221247.28391-1-stephen@networkplumber.org> <20190702221247.28391-6-stephen@networkplumber.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190702221247.28391-6-stephen@networkplumber.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v7 05/12] net/ether: mark ethernet addresses as being 2-byte aligned X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jul 02, 2019 at 03:12:40PM -0700, Stephen Hemminger wrote: > From: 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 > Signed-off-by: Stephen Hemminger > Reviewed-by: Andrew Rybchenko > --- > 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__)); > > #define RTE_ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */ > @@ -81,8 +82,8 @@ struct rte_ether_addr { > static inline int rte_is_same_ether_addr(const struct rte_ether_addr *ea1, > const struct rte_ether_addr *ea2) > { > - const unaligned_uint16_t *w1 = (const uint16_t *)ea1; > - const unaligned_uint16_t *w2 = (const uint16_t *)ea2; > + const uint16_t *w1 = (const uint16_t *)ea1; > + const uint16_t *w2 = (const uint16_t *)ea2; > > return ((w1[0] ^ w2[0]) | (w1[1] ^ w2[1]) | (w1[2] ^ w2[2])) == 0; > } > @@ -99,7 +100,7 @@ static inline int rte_is_same_ether_addr(const struct rte_ether_addr *ea1, > */ > static inline int rte_is_zero_ether_addr(const struct rte_ether_addr *ea) > { > - const unaligned_uint16_t *w = (const uint16_t *)ea; > + const uint16_t *w = (const uint16_t *)ea; > > return (w[0] | w[1] | w[2]) == 0; > } > @@ -146,7 +147,7 @@ static inline int rte_is_multicast_ether_addr(const struct rte_ether_addr *ea) > */ > static inline int rte_is_broadcast_ether_addr(const struct rte_ether_addr *ea) > { > - const unaligned_uint16_t *ea_words = (const unaligned_uint16_t *)ea; > + const uint16_t *ea_words = (const uint16_t *)ea; > > return (ea_words[0] == 0xFFFF && ea_words[1] == 0xFFFF && > ea_words[2] == 0xFFFF); > -- > 2.20.1 > Following this discussion: https://mails.dpdk.org/archives/dev/2019-July/136590.html I still think that changing the ABI without deprecation notice is not a good idea. The warning issued by the compiler makes me think that the definition of unaligned_uint16_t is wrong on intel arch. I made a quick test, and it seems that in this particular case, the generated code is the same with or without __attribute__((aligned(1))). See: https://godbolt.org/z/NjBNQk But changing the definition of unaligned_uint16_t without a deprecation notice is not an option either. What do you think about using a specific typedef similar to unaligned_uint16_t in rte_ether, that has the __attribute__((aligned(1))) ? It would avoid to change the alignment of struct rte_ether_addr. In parallel, we can talk about changing unaligned_uint16_t for intel in another patchset. Olivier