diff mbox series

net: improve vlan header type alignment

Message ID 20241013083554.97489-1-mb@smartsharesystems.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series net: improve vlan header type alignment |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-unit-amd64-testing pending Testing pending
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-unit-arm64-testing success Testing PASS

Commit Message

Morten Brørup Oct. 13, 2024, 8:35 a.m. UTC 
Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
VLAN tag (a.k.a. VLAN header) embedded.
Since the Ethernet header is 2 byte aligned, and the VLAN tag is directly
related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
packing the VLAN tag structure is not necessary.

Furthermore, the Ethernet header type is implictly 2 byte aligned, so
removed the superfluous explicit 2 byte alignment.

Added static_asserts to verify the size and alignment of the various
Ethernet types.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/net/rte_ether.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Morten Brørup Oct. 13, 2024, 8:51 a.m. UTC | #1 
> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Sunday, 13 October 2024 10.36
> 
> Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
> VLAN tag (a.k.a. VLAN header) embedded.
> Since the Ethernet header is 2 byte aligned, and the VLAN tag is
> directly
> related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
> packing the VLAN tag structure is not necessary.

Stephen, Bruce, you missed the VLAN header type in this patch:
https://git.dpdk.org/dpdk/commit/lib/librte_net/rte_ether.h?id=da5350ef29afd35c1adabe76f60832f3092269ad

This patch completes your work.

> 
> Furthermore, the Ethernet header type is implictly 2 byte aligned, so
> removed the superfluous explicit 2 byte alignment.
> 
> Added static_asserts to verify the size and alignment of the various
> Ethernet types.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/net/rte_ether.h | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
> index 403e84f50b..c9a0b536c3 100644
> --- a/lib/net/rte_ether.h
> +++ b/lib/net/rte_ether.h
> @@ -11,6 +11,8 @@
>   * Ethernet Helpers in RTE
>   */
> 
> +#include <assert.h>
> +#include <stdalign.h>
>  #include <stdint.h>
>  #include <stdio.h>
> 
> @@ -75,6 +77,11 @@ struct __rte_aligned(2) rte_ether_addr {
>  	uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx
> order */
>  };
> 
> +static_assert(sizeof(struct rte_ether_addr) == 6,
> +		"sizeof(struct rte_ether_addr) == 6");
> +static_assert(alignof(struct rte_ether_addr) == 2,
> +		"alignof(struct rte_ether_addr) == 2");
> +
>  #define RTE_ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth.
> address. */
>  #define RTE_ETHER_GROUP_ADDR  0x01 /**< Multicast or broadcast Eth.
> address. */
> 
> @@ -290,12 +297,17 @@ rte_ether_unformat_addr(const char *str, struct
> rte_ether_addr *eth_addr);
>   * Ethernet header: Contains the destination address, source address
>   * and frame type.
>   */
> -struct __rte_aligned(2) rte_ether_hdr {
> +struct rte_ether_hdr {
>  	struct rte_ether_addr dst_addr; /**< Destination address. */
>  	struct rte_ether_addr src_addr; /**< Source address. */
>  	rte_be16_t ether_type; /**< Frame type. */
>  };
> 
> +static_assert(sizeof(struct rte_ether_hdr) == 14,
> +		"sizeof(struct rte_ether_hdr) == 14");
> +static_assert(alignof(struct rte_ether_hdr) == 2,
> +		"alignof(struct rte_ether_hdr) == 2");
> +
>  /**
>   * Ethernet VLAN Header.
>   * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet
> type
> @@ -304,7 +316,12 @@ struct __rte_aligned(2) rte_ether_hdr {
>  struct rte_vlan_hdr {
>  	rte_be16_t vlan_tci;  /**< Priority (3) + CFI (1) + Identifier
> Code (12) */
>  	rte_be16_t eth_proto; /**< Ethernet type of encapsulated frame.
> */
> -} __rte_packed;
> +};
> +
> +static_assert(sizeof(struct rte_vlan_hdr) == 4,
> +		"sizeof(struct rte_vlan_hdr) == 4");
> +static_assert(alignof(struct rte_vlan_hdr) == 2,
> +		"alignof(struct rte_vlan_hdr) == 2");
> 
> 
> 
> --
> 2.43.0
fengchengwen Oct. 14, 2024, 5:42 a.m. UTC | #2 
Acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2024/10/13 16:35, Morten Brørup wrote:
> Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
> VLAN tag (a.k.a. VLAN header) embedded.
> Since the Ethernet header is 2 byte aligned, and the VLAN tag is directly
> related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
> packing the VLAN tag structure is not necessary.
> 
> Furthermore, the Ethernet header type is implictly 2 byte aligned, so
> removed the superfluous explicit 2 byte alignment.
> 
> Added static_asserts to verify the size and alignment of the various
> Ethernet types.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Bruce Richardson Oct. 14, 2024, 8:04 a.m. UTC | #3 
On Sun, Oct 13, 2024 at 08:35:54AM +0000, Morten Brørup wrote:
> Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
> VLAN tag (a.k.a. VLAN header) embedded.
> Since the Ethernet header is 2 byte aligned, and the VLAN tag is directly
> related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
> packing the VLAN tag structure is not necessary.
> 
> Furthermore, the Ethernet header type is implictly 2 byte aligned, so
> removed the superfluous explicit 2 byte alignment.
> 
> Added static_asserts to verify the size and alignment of the various
> Ethernet types.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/net/rte_ether.h | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Thomas Monjalon Oct. 17, 2024, 8:44 p.m. UTC | #4 
13/10/2024 10:35, Morten Brørup:
> +static_assert(sizeof(struct rte_ether_addr) == 6,
> +		"sizeof(struct rte_ether_addr) == 6");
> +static_assert(alignof(struct rte_ether_addr) == 2,
> +		"alignof(struct rte_ether_addr) == 2");

Instead of repeating the condition twice,
it would be simpler to use RTE_BUILD_BUG_ON
Morten Brørup Oct. 17, 2024, 11:41 p.m. UTC | #5 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 17 October 2024 22.44
> 
> 13/10/2024 10:35, Morten Brørup:
> > +static_assert(sizeof(struct rte_ether_addr) == 6,
> > +		"sizeof(struct rte_ether_addr) == 6");
> > +static_assert(alignof(struct rte_ether_addr) == 2,
> > +		"alignof(struct rte_ether_addr) == 2");
> 
> Instead of repeating the condition twice,
> it would be simpler to use RTE_BUILD_BUG_ON

RTE_BUILD_BUG_ON can only be used in code blocks, so it would need to be wrapped in some dummy function.
Thomas Monjalon Oct. 18, 2024, 9:11 a.m. UTC | #6 
13/10/2024 10:35, Morten Brørup:
> Ethernet packets can be VLAN tagged, i.e. an Ethernet header can have a
> VLAN tag (a.k.a. VLAN header) embedded.
> Since the Ethernet header is 2 byte aligned, and the VLAN tag is directly
> related to the Ethernet header, the VLAN tag is also 2 byte aligned, so
> packing the VLAN tag structure is not necessary.
> 
> Furthermore, the Ethernet header type is implictly 2 byte aligned, so
> removed the superfluous explicit 2 byte alignment.
> 
> Added static_asserts to verify the size and alignment of the various
> Ethernet types.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>

Applied, thanks.
diff mbox series

Patch

diff --git a/lib/net/rte_ether.h b/lib/net/rte_ether.h
index 403e84f50b..c9a0b536c3 100644
--- a/lib/net/rte_ether.h
+++ b/lib/net/rte_ether.h
@@ -11,6 +11,8 @@ 
  * Ethernet Helpers in RTE
  */
 
+#include <assert.h>
+#include <stdalign.h>
 #include <stdint.h>
 #include <stdio.h>
 
@@ -75,6 +77,11 @@  struct __rte_aligned(2) rte_ether_addr {
 	uint8_t addr_bytes[RTE_ETHER_ADDR_LEN]; /**< Addr bytes in tx order */
 };
 
+static_assert(sizeof(struct rte_ether_addr) == 6,
+		"sizeof(struct rte_ether_addr) == 6");
+static_assert(alignof(struct rte_ether_addr) == 2,
+		"alignof(struct rte_ether_addr) == 2");
+
 #define RTE_ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
 #define RTE_ETHER_GROUP_ADDR  0x01 /**< Multicast or broadcast Eth. address. */
 
@@ -290,12 +297,17 @@  rte_ether_unformat_addr(const char *str, struct rte_ether_addr *eth_addr);
  * Ethernet header: Contains the destination address, source address
  * and frame type.
  */
-struct __rte_aligned(2) rte_ether_hdr {
+struct rte_ether_hdr {
 	struct rte_ether_addr dst_addr; /**< Destination address. */
 	struct rte_ether_addr src_addr; /**< Source address. */
 	rte_be16_t ether_type; /**< Frame type. */
 };
 
+static_assert(sizeof(struct rte_ether_hdr) == 14,
+		"sizeof(struct rte_ether_hdr) == 14");
+static_assert(alignof(struct rte_ether_hdr) == 2,
+		"alignof(struct rte_ether_hdr) == 2");
+
 /**
  * Ethernet VLAN Header.
  * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type
@@ -304,7 +316,12 @@  struct __rte_aligned(2) rte_ether_hdr {
 struct rte_vlan_hdr {
 	rte_be16_t vlan_tci;  /**< Priority (3) + CFI (1) + Identifier Code (12) */
 	rte_be16_t eth_proto; /**< Ethernet type of encapsulated frame. */
-} __rte_packed;
+};
+
+static_assert(sizeof(struct rte_vlan_hdr) == 4,
+		"sizeof(struct rte_vlan_hdr) == 4");
+static_assert(alignof(struct rte_vlan_hdr) == 2,
+		"alignof(struct rte_vlan_hdr) == 2");