ether: remove packing and set two-byte alignment

Message ID 20191010104623.10090-1-bruce.richardson@intel.com (mailing list archive)
State Accepted, archived
Headers
Series ether: remove packing and set two-byte alignment |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Bruce Richardson Oct. 10, 2019, 10:46 a.m. UTC
  The ether header does not need to be packed since that makes no sense for
structures with only bytes in them, but it should be aligned to a two-byte
boundary to simplify access to it from code. Other packed structures that
use this also need to be updated to take account of the change, either by
removing packing - where it is clearly unneeded - or by explicitly giving
those structures 2-byte alignment also.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/rel_notes/deprecation.rst      | 10 ----------
 doc/guides/rel_notes/release_19_11.rst    |  6 ++++++
 drivers/net/bonding/rte_eth_bond_8023ad.h | 14 +++++++-------
 lib/librte_net/rte_arp.h                  |  4 ++--
 lib/librte_net/rte_ether.h                | 12 ++++++------
 lib/librte_pipeline/rte_table_action.c    | 22 +++++++++++-----------
 6 files changed, 32 insertions(+), 36 deletions(-)
  

Comments

Thomas Monjalon Oct. 27, 2019, 5:13 p.m. UTC | #1
10/10/2019 12:46, Bruce Richardson:
> The ether header does not need to be packed since that makes no sense for
> structures with only bytes in them, but it should be aligned to a two-byte
> boundary to simplify access to it from code. Other packed structures that
> use this also need to be updated to take account of the change, either by
> removing packing - where it is clearly unneeded - or by explicitly giving
> those structures 2-byte alignment also.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Applied, thanks
  

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 0ee8533b1..86ec962d0 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -61,16 +61,6 @@  Deprecation Notices
   structure would be made internal (or removed if all dependencies are cleared)
   in future releases.
 
-* net: The Ethernet address and header definitions will change
-  attributes. The Ethernet address struct will no longer be marked as
-  packed since the packed attribute is meaningless on a byte
-  array. The Ethernet header will be marked as aligned on a 2-byte
-  boundary and will no longer have the packed attribute. This allows
-  for efficient access on CPU architectures where unaligned access is
-  expensive. These changes should not impact normal usage because drivers
-  naturally align the Ethernet header on receive and all known
-  encapsulations preserve the alignment of the header.
-
 * ethdev: The function ``rte_eth_dev_count`` will be removed in DPDK 20.02.
   It is replaced by the function ``rte_eth_dev_count_avail``.
   If the intent is to iterate over ports, ``RTE_ETH_FOREACH_*`` macros
diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
index 27cfbd9e3..efaba7872 100644
--- a/doc/guides/rel_notes/release_19_11.rst
+++ b/doc/guides/rel_notes/release_19_11.rst
@@ -110,6 +110,12 @@  ABI Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* net: The Ethernet address and other header definitions have changed
+  attributes. They have been modified to be aligned on 2-byte boundaries.
+  These changes should not impact normal usage because drivers naturally
+  align the Ethernet header on receive and all known encapsulations
+  preserve the alignment of the header.
+
 
 Shared Library Versions
 -----------------------
diff --git a/drivers/net/bonding/rte_eth_bond_8023ad.h b/drivers/net/bonding/rte_eth_bond_8023ad.h
index cbad59aa7..62265f449 100644
--- a/drivers/net/bonding/rte_eth_bond_8023ad.h
+++ b/drivers/net/bonding/rte_eth_bond_8023ad.h
@@ -60,7 +60,7 @@  struct slow_protocol {
 struct slow_protocol_frame {
 	struct rte_ether_hdr eth_hdr;
 	struct slow_protocol slow_protocol;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct port_params {
 	uint16_t system_priority;
@@ -73,7 +73,7 @@  struct port_params {
 	/**< Priority of this (unused in current implementation) */
 	uint16_t port_number;
 	/**< Port number. It corresponds to slave port id. */
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct lacpdu_actor_partner_params {
 	uint8_t tlv_type_info;
@@ -81,7 +81,7 @@  struct lacpdu_actor_partner_params {
 	struct port_params port_params;
 	uint8_t state;
 	uint8_t reserved_3[3];
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 /** LACPDU structure (5.4.2 in 802.1AX documentation). */
 struct lacpdu {
@@ -99,13 +99,13 @@  struct lacpdu {
 	uint8_t tlv_type_terminator;
 	uint8_t terminator_length;
 	uint8_t reserved_50[50];
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 /** LACPDU frame: Contains ethernet header and LACPDU. */
 struct lacpdu_header {
 	struct rte_ether_hdr eth_hdr;
 	struct lacpdu lacpdu;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct marker {
 	uint8_t subtype;
@@ -121,12 +121,12 @@  struct marker {
 	uint8_t tlv_type_terminator;
 	uint8_t terminator_length;
 	uint8_t reserved_90[90];
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct marker_header {
 	struct rte_ether_hdr eth_hdr;
 	struct marker marker;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct rte_eth_bond_8023ad_conf {
 	uint32_t fast_periodic_ms;
diff --git a/lib/librte_net/rte_arp.h b/lib/librte_net/rte_arp.h
index ccb6875cf..fabd68624 100644
--- a/lib/librte_net/rte_arp.h
+++ b/lib/librte_net/rte_arp.h
@@ -26,7 +26,7 @@  struct rte_arp_ipv4 {
 	uint32_t          arp_sip;  /**< sender IP address */
 	struct rte_ether_addr arp_tha;  /**< target hardware address */
 	uint32_t          arp_tip;  /**< target IP address */
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 /**
  * ARP header.
@@ -47,7 +47,7 @@  struct rte_arp_hdr {
 #define	RTE_ARP_OP_INVREPLY   9 /* response identifying peer */
 
 	struct rte_arp_ipv4 arp_data;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 /**
  * @warning
diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index aa6eff037..5b81a7a96 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -59,7 +59,7 @@  extern "C" {
  */
 struct rte_ether_addr {
 	uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
-} __attribute__((__packed__));
+} __attribute__((aligned(2)));
 
 #define RTE_ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
 #define RTE_ETHER_GROUP_ADDR  0x01 /**< Multicast or broadcast Eth. address. */
@@ -81,8 +81,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 +99,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 +146,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);
@@ -273,7 +273,7 @@  struct rte_ether_hdr {
 	struct rte_ether_addr d_addr; /**< Destination address. */
 	struct rte_ether_addr s_addr; /**< Source address. */
 	uint16_t ether_type;      /**< Frame type. */
-} __attribute__((__packed__));
+} __attribute__((aligned(2)));
 
 /**
  * Ethernet VLAN Header.
diff --git a/lib/librte_pipeline/rte_table_action.c b/lib/librte_pipeline/rte_table_action.c
index 47d7efbc1..962404b7a 100644
--- a/lib/librte_pipeline/rte_table_action.c
+++ b/lib/librte_pipeline/rte_table_action.c
@@ -438,7 +438,7 @@  encap_cfg_check(struct rte_table_action_encap_config *encap)
 
 struct encap_ether_data {
 	struct rte_ether_hdr ether;
-} __attribute__((__packed__));
+};
 
 #define VLAN(pcp, dei, vid)                                \
 	((uint16_t)((((uint64_t)(pcp)) & 0x7LLU) << 13) |  \
@@ -448,13 +448,13 @@  struct encap_ether_data {
 struct encap_vlan_data {
 	struct rte_ether_hdr ether;
 	struct rte_vlan_hdr vlan;
-} __attribute__((__packed__));
+};
 
 struct encap_qinq_data {
 	struct rte_ether_hdr ether;
 	struct rte_vlan_hdr svlan;
 	struct rte_vlan_hdr cvlan;
-} __attribute__((__packed__));
+};
 
 #define ETHER_TYPE_MPLS_UNICAST                            0x8847
 
@@ -470,7 +470,7 @@  struct encap_mpls_data {
 	struct rte_ether_hdr ether;
 	uint32_t mpls[RTE_TABLE_ACTION_MPLS_LABELS_MAX];
 	uint32_t mpls_count;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 #define PPP_PROTOCOL_IP                                    0x0021
 
@@ -479,12 +479,12 @@  struct pppoe_ppp_hdr {
 	uint16_t session_id;
 	uint16_t length;
 	uint16_t protocol;
-} __attribute__((__packed__));
+};
 
 struct encap_pppoe_data {
 	struct rte_ether_hdr ether;
 	struct pppoe_ppp_hdr pppoe_ppp;
-} __attribute__((__packed__));
+};
 
 #define IP_PROTO_UDP                                       17
 
@@ -493,7 +493,7 @@  struct encap_vxlan_ipv4_data {
 	struct rte_ipv4_hdr ipv4;
 	struct rte_udp_hdr udp;
 	struct rte_vxlan_hdr vxlan;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct encap_vxlan_ipv4_vlan_data {
 	struct rte_ether_hdr ether;
@@ -501,14 +501,14 @@  struct encap_vxlan_ipv4_vlan_data {
 	struct rte_ipv4_hdr ipv4;
 	struct rte_udp_hdr udp;
 	struct rte_vxlan_hdr vxlan;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct encap_vxlan_ipv6_data {
 	struct rte_ether_hdr ether;
 	struct rte_ipv6_hdr ipv6;
 	struct rte_udp_hdr udp;
 	struct rte_vxlan_hdr vxlan;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct encap_vxlan_ipv6_vlan_data {
 	struct rte_ether_hdr ether;
@@ -516,14 +516,14 @@  struct encap_vxlan_ipv6_vlan_data {
 	struct rte_ipv6_hdr ipv6;
 	struct rte_udp_hdr udp;
 	struct rte_vxlan_hdr vxlan;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 struct encap_qinq_pppoe_data {
 	struct rte_ether_hdr ether;
 	struct rte_vlan_hdr svlan;
 	struct rte_vlan_hdr cvlan;
 	struct pppoe_ppp_hdr pppoe_ppp;
-} __attribute__((__packed__));
+} __attribute__((__packed__)) __attribute__((aligned(2)));
 
 static size_t
 encap_data_size(struct rte_table_action_encap_config *encap)