[v2,2/7] ether: do not mark ethernet address and header as packed

Message ID 20190516180427.17270-3-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Headers
Series ether: improvements and optimizations |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger May 16, 2019, 6:04 p.m. UTC
  Since the Ethernet address and header are naturally aligned
correctly on all architectures, there is no need to mark them
as packed. This results in faster code, and also gets rid of
warnings with gcc-9 about getting address of packed structure.

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_net/rte_ether.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
  

Comments

Bruce Richardson May 16, 2019, 8:40 p.m. UTC | #1
On Thu, May 16, 2019 at 11:04:22AM -0700, Stephen Hemminger wrote:
> Since the Ethernet address and header are naturally aligned
> correctly on all architectures, there is no need to mark them
> as packed. This results in faster code, and also gets rid of
> warnings with gcc-9 about getting address of packed structure.
> 
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_net/rte_ether.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index ac8897681aca..0954d183cca7 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -57,7 +57,7 @@ extern "C" {
>  struct ether_addr {
>  	/** 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. */
>  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
> @@ -273,7 +273,7 @@ struct ether_hdr {
>  	struct ether_addr d_addr; /**< Destination address. */
>  	struct ether_addr s_addr; /**< Source address. */
>  	uint16_t ether_type;      /**< Frame type. */
> -} __attribute__((__packed__));
> +};
>  
>  /**
>   * Ethernet VLAN Header.
> @@ -283,7 +283,7 @@ struct ether_hdr {
>  struct vlan_hdr {
>  	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
>  	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
> -} __attribute__((__packed__));
> +};
>  
>  /**
>   * VXLAN protocol header.
> @@ -293,7 +293,7 @@ struct vlan_hdr {
>  struct vxlan_hdr {
>  	uint32_t vx_flags; /**< flag (8) + Reserved (24). */
>  	uint32_t vx_vni;   /**< VNI (24) + Reserved (8). */
> -} __attribute__((__packed__));
> +};
>  
>  /* Ethernet frame types */
>  #define ETHER_TYPE_IPv4 0x0800 /**< IPv4 Protocol. */
> @@ -325,7 +325,7 @@ struct vxlan_gpe_hdr {
>  	uint8_t reserved[2]; /**< Reserved (16). */
>  	uint8_t proto;       /**< next-protocol (8). */
>  	uint32_t vx_vni;     /**< VNI (24) + Reserved (8). */
> -} __attribute__((__packed__));
> +};
>  

Are we sure that these last two structures are always 4-byte aligned in
packets?

>  /* VXLAN-GPE next protocol types */
>  #define VXLAN_GPE_TYPE_IPV4 1 /**< IPv4 Protocol. */
> -- 
> 2.20.1
>
  
Stephen Hemminger May 16, 2019, 11:17 p.m. UTC | #2
On Thu, 16 May 2019 21:40:05 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, May 16, 2019 at 11:04:22AM -0700, Stephen Hemminger wrote:
> > Since the Ethernet address and header are naturally aligned
> > correctly on all architectures, there is no need to mark them
> > as packed. This results in faster code, and also gets rid of
> > warnings with gcc-9 about getting address of packed structure.
> > 
> > Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_net/rte_ether.h | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index ac8897681aca..0954d183cca7 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -57,7 +57,7 @@ extern "C" {
> >  struct ether_addr {
> >  	/** 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. */
> >  #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
> > @@ -273,7 +273,7 @@ struct ether_hdr {
> >  	struct ether_addr d_addr; /**< Destination address. */
> >  	struct ether_addr s_addr; /**< Source address. */
> >  	uint16_t ether_type;      /**< Frame type. */
> > -} __attribute__((__packed__));
> > +};
> >  
> >  /**
> >   * Ethernet VLAN Header.
> > @@ -283,7 +283,7 @@ struct ether_hdr {
> >  struct vlan_hdr {
> >  	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
> >  	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
> > -} __attribute__((__packed__));
> > +};
> >  
> >  /**
> >   * VXLAN protocol header.
> > @@ -293,7 +293,7 @@ struct vlan_hdr {
> >  struct vxlan_hdr {
> >  	uint32_t vx_flags; /**< flag (8) + Reserved (24). */
> >  	uint32_t vx_vni;   /**< VNI (24) + Reserved (8). */
> > -} __attribute__((__packed__));
> > +};
> >  
> >  /* Ethernet frame types */
> >  #define ETHER_TYPE_IPv4 0x0800 /**< IPv4 Protocol. */
> > @@ -325,7 +325,7 @@ struct vxlan_gpe_hdr {
> >  	uint8_t reserved[2]; /**< Reserved (16). */
> >  	uint8_t proto;       /**< next-protocol (8). */
> >  	uint32_t vx_vni;     /**< VNI (24) + Reserved (8). */
> > -} __attribute__((__packed__));
> > +};
> >    
> 
> Are we sure that these last two structures are always 4-byte aligned in
> packets?

That is a general issue of unaligned IP header problem.
Really we should be doing a combination of packed and 2 byte alignment.

I will drop the Vxlan and NVGre ones
  

Patch

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index ac8897681aca..0954d183cca7 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -57,7 +57,7 @@  extern "C" {
 struct ether_addr {
 	/** 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. */
 #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. */
@@ -273,7 +273,7 @@  struct ether_hdr {
 	struct ether_addr d_addr; /**< Destination address. */
 	struct ether_addr s_addr; /**< Source address. */
 	uint16_t ether_type;      /**< Frame type. */
-} __attribute__((__packed__));
+};
 
 /**
  * Ethernet VLAN Header.
@@ -283,7 +283,7 @@  struct ether_hdr {
 struct vlan_hdr {
 	uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
 	uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
-} __attribute__((__packed__));
+};
 
 /**
  * VXLAN protocol header.
@@ -293,7 +293,7 @@  struct vlan_hdr {
 struct vxlan_hdr {
 	uint32_t vx_flags; /**< flag (8) + Reserved (24). */
 	uint32_t vx_vni;   /**< VNI (24) + Reserved (8). */
-} __attribute__((__packed__));
+};
 
 /* Ethernet frame types */
 #define ETHER_TYPE_IPv4 0x0800 /**< IPv4 Protocol. */
@@ -325,7 +325,7 @@  struct vxlan_gpe_hdr {
 	uint8_t reserved[2]; /**< Reserved (16). */
 	uint8_t proto;       /**< next-protocol (8). */
 	uint32_t vx_vni;     /**< VNI (24) + Reserved (8). */
-} __attribute__((__packed__));
+};
 
 /* VXLAN-GPE next protocol types */
 #define VXLAN_GPE_TYPE_IPV4 1 /**< IPv4 Protocol. */