[PATCH 0/6] Reduce scope address-of-packed-member warning

[PATCH 0/6] Reduce scope address-of-packed-member warning

[Bruce Richardson]

The warning for address-of-packed-member was being disabled globally in
DPDK.  While for drivers which need to access hardware-defined
data-structures the use of packed may make sense, for normal libs and
applications the use of packed data should be generally avoided.

This patchset initially applies some fixes for cases where we are
unnecessarily causing the warning to trigger. Thereafter the 6th patch
removes the global enabling of the warning and replaces it with more
selective disabling for drivers and for a couple of other components
which have not yet been fixed.

Bruce Richardson (6):
  ip_frag: remove use of unaligned variable
  efd: remove unnecessary packed attributes
  bus/ifpga: remove packed attribute
  pipeline: remove packed attribute
  net: add smaller IPv4 cksum function for simple cases
  build: limit scope of packed member warning disabling

 app/test-eventdev/test_pipeline_common.c | 25 +-----------
 app/test-pmd/icmpecho.c                  | 23 +----------
 app/test-pmd/txonly.c                    | 22 +----------
 app/test/packet_burst_generator.c        | 49 +-----------------------
 app/test/test_reassembly_perf.c          | 29 +-------------
 config/meson.build                       |  1 -
 drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
 drivers/meson.build                      |  9 +++--
 examples/ipsec-secgw/meson.build         |  6 +++
 lib/efd/rte_efd.c                        |  4 +-
 lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
 lib/net/rte_ip.h                         | 33 ++++++++++++++++
 lib/pipeline/rte_table_action.c          |  2 +-
 lib/vhost/meson.build                    |  5 ++-
 14 files changed, 60 insertions(+), 154 deletions(-)

--
2.43.0

[PATCH 1/6] ip_frag: remove use of unaligned variable

[Bruce Richardson]

If compiling with -Waddress-of-packed-member, we get a warning about the
use of the unaligned uint64_t value which is used to copy 8 bytes from
ip_hdr to the key. Replace this unaligned assignment with an equivalent
8-byte constant-sized memcpy, allowing the compiler to choose optimal
instructions to do the assignment.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/ip_frag/rte_ipv4_reassembly.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/ip_frag/rte_ipv4_reassembly.c b/lib/ip_frag/rte_ipv4_reassembly.c
index 4a89a5f536..5818f50f40 100644
--- a/lib/ip_frag/rte_ipv4_reassembly.c
+++ b/lib/ip_frag/rte_ipv4_reassembly.c
@@ -101,7 +101,6 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 {
 	struct ip_frag_pkt *fp;
 	struct ip_frag_key key;
-	const unaligned_uint64_t *psd;
 	uint16_t flag_offset, ip_ofs, ip_flag;
 	int32_t ip_len;
 	int32_t trim;
@@ -110,9 +109,8 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 	ip_ofs = (uint16_t)(flag_offset & RTE_IPV4_HDR_OFFSET_MASK);
 	ip_flag = (uint16_t)(flag_offset & RTE_IPV4_HDR_MF_FLAG);
 
-	psd = (unaligned_uint64_t *)&ip_hdr->src_addr;
 	/* use first 8 bytes only */
-	key.src_dst[0] = psd[0];
+	memcpy(&key.src_dst[0], &ip_hdr->src_addr, 8);
 	key.id = ip_hdr->packet_id;
 	key.key_len = IPV4_KEYLEN;
 
-- 
2.43.0

Re: [PATCH 1/6] ip_frag: remove use of unaligned variable

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:08 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> If compiling with -Waddress-of-packed-member, we get a warning about the
> use of the unaligned uint64_t value which is used to copy 8 bytes from
> ip_hdr to the key. Replace this unaligned assignment with an equivalent
> 8-byte constant-sized memcpy, allowing the compiler to choose optimal
> instructions to do the assignment.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

RE: [PATCH 1/6] ip_frag: remove use of unaligned variable

[Konstantin Ananyev]

> If compiling with -Waddress-of-packed-member, we get a warning about the
> use of the unaligned uint64_t value which is used to copy 8 bytes from
> ip_hdr to the key. Replace this unaligned assignment with an equivalent
> 8-byte constant-sized memcpy, allowing the compiler to choose optimal
> instructions to do the assignment.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/ip_frag/rte_ipv4_reassembly.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/lib/ip_frag/rte_ipv4_reassembly.c b/lib/ip_frag/rte_ipv4_reassembly.c
> index 4a89a5f536..5818f50f40 100644
> --- a/lib/ip_frag/rte_ipv4_reassembly.c
> +++ b/lib/ip_frag/rte_ipv4_reassembly.c
> @@ -101,7 +101,6 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
>  {
>  	struct ip_frag_pkt *fp;
>  	struct ip_frag_key key;
> -	const unaligned_uint64_t *psd;
>  	uint16_t flag_offset, ip_ofs, ip_flag;
>  	int32_t ip_len;
>  	int32_t trim;
> @@ -110,9 +109,8 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
>  	ip_ofs = (uint16_t)(flag_offset & RTE_IPV4_HDR_OFFSET_MASK);
>  	ip_flag = (uint16_t)(flag_offset & RTE_IPV4_HDR_MF_FLAG);
> 
> -	psd = (unaligned_uint64_t *)&ip_hdr->src_addr;
>  	/* use first 8 bytes only */
> -	key.src_dst[0] = psd[0];
> +	memcpy(&key.src_dst[0], &ip_hdr->src_addr, 8);
>  	key.id = ip_hdr->packet_id;
>  	key.key_len = IPV4_KEYLEN;
> 
> --

As a small nit, might be better:
memcpy(&key.src_dst[0], &ip_hdr->src_addr, sizeof(key.src_dst[0]));
 
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

> 2.43.0

[PATCH 2/6] efd: remove unnecessary packed attributes

[Bruce Richardson]

The structure "efd_online_group_entry" only consists of values which are
typedefs of "uint16_t", so packing the structure has no effect. The
"efd_online_chunk" structure has a mix of "uint8_t" and the
"efd_online_group_entry" struct, i.e. uint16_t values, but since the
first, uint8_t, member array is of even size, the packed attribute does
not affect the structure layout.

Removing these packed attributes allows the library to compile cleanly
without "-Wno-address-of-packed-member" compiler flag.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/efd/rte_efd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
index d3b732f2e8..3cbb3c2719 100644
--- a/lib/efd/rte_efd.c
+++ b/lib/efd/rte_efd.c
@@ -212,7 +212,7 @@ struct efd_offline_chunk_rules {
 struct efd_online_group_entry {
 	efd_hashfunc_t hash_idx[RTE_EFD_VALUE_NUM_BITS];
 	efd_lookuptbl_t lookup_table[RTE_EFD_VALUE_NUM_BITS];
-} __rte_packed;
+};
 
 /**
  * A single chunk record, containing EFD_TARGET_CHUNK_NUM_RULES rules.
@@ -228,7 +228,7 @@ struct efd_online_chunk {
 
 	struct efd_online_group_entry groups[EFD_CHUNK_NUM_GROUPS];
 	/**< Array of all the groups in the chunk. */
-} __rte_packed;
+};
 
 /**
  * EFD table structure
-- 
2.43.0

Re: [PATCH 2/6] efd: remove unnecessary packed attributes

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:09 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The structure "efd_online_group_entry" only consists of values which are
> typedefs of "uint16_t", so packing the structure has no effect. The
> "efd_online_chunk" structure has a mix of "uint8_t" and the
> "efd_online_group_entry" struct, i.e. uint16_t values, but since the
> first, uint8_t, member array is of even size, the packed attribute does
> not affect the structure layout.
> 
> Removing these packed attributes allows the library to compile cleanly
> without "-Wno-address-of-packed-member" compiler flag.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 3/6] bus/ifpga: remove packed attribute

[Bruce Richardson]

The struct rte_afu_device does not need to be packed, so remove the
packed attribute from it.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/bus/ifpga/bus_ifpga_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/ifpga/bus_ifpga_driver.h b/drivers/bus/ifpga/bus_ifpga_driver.h
index 5bbe36d6e0..557e8fd08d 100644
--- a/drivers/bus/ifpga/bus_ifpga_driver.h
+++ b/drivers/bus/ifpga/bus_ifpga_driver.h
@@ -77,7 +77,7 @@ struct rte_afu_device {
 	struct rte_intr_handle *intr_handle;     /**< Interrupt handle */
 	struct rte_afu_driver *driver;          /**< Associated driver */
 	char path[IFPGA_BUS_BITSTREAM_PATH_MAX_LEN];
-} __rte_packed;
+};
 
 /**
  * @internal
-- 
2.43.0

RE: [PATCH 3/6] bus/ifpga: remove packed attribute

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, October 17, 2024 10:22 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Xu, Rosen
> <rosen.xu@intel.com>
> Subject: [PATCH 3/6] bus/ifpga: remove packed attribute
> 
> The struct rte_afu_device does not need to be packed, so remove the
> packed attribute from it.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/bus/ifpga/bus_ifpga_driver.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/ifpga/bus_ifpga_driver.h
> b/drivers/bus/ifpga/bus_ifpga_driver.h
> index 5bbe36d6e0..557e8fd08d 100644
> --- a/drivers/bus/ifpga/bus_ifpga_driver.h
> +++ b/drivers/bus/ifpga/bus_ifpga_driver.h
> @@ -77,7 +77,7 @@ struct rte_afu_device {
>  	struct rte_intr_handle *intr_handle;     /**< Interrupt handle */
>  	struct rte_afu_driver *driver;          /**< Associated driver */
>  	char path[IFPGA_BUS_BITSTREAM_PATH_MAX_LEN];
> -} __rte_packed;
> +};
> 
>  /**
>   * @internal
> --
> 2.43.0

Acked-by: Rosen Xu <rosen.xu@intel.com>

Re: [PATCH 3/6] bus/ifpga: remove packed attribute

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:10 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The struct rte_afu_device does not need to be packed, so remove the
> packed attribute from it.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 4/6] pipeline: remove packed attribute

[Bruce Richardson]

The rte_trtcm_data structure, and its substructures, only consist of
uint64_t types. This makes packing unnecessary to remove the packed
attribute from the structure.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/pipeline/rte_table_action.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pipeline/rte_table_action.c b/lib/pipeline/rte_table_action.c
index 87c3e0e2c9..b3c77dbf66 100644
--- a/lib/pipeline/rte_table_action.c
+++ b/lib/pipeline/rte_table_action.c
@@ -109,7 +109,7 @@ mtr_cfg_check(struct rte_table_action_mtr_config *mtr)
 struct mtr_trtcm_data {
 	struct rte_meter_trtcm trtcm;
 	uint64_t stats[RTE_COLORS];
-} __rte_packed;
+};
 
 #define MTR_TRTCM_DATA_METER_PROFILE_ID_GET(data)          \
 	(((data)->stats[RTE_COLOR_GREEN] & 0xF8LLU) >> 3)
-- 
2.43.0

Re: [PATCH 4/6] pipeline: remove packed attribute

[Bruce Richardson]

On Thu, Oct 17, 2024 at 03:22:11PM +0100, Bruce Richardson wrote:
> The rte_trtcm_data structure, and its substructures, only consist of
> uint64_t types. This makes packing unnecessary to remove the packed

s/to/, so/

> attribute from the structure.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/pipeline/rte_table_action.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/pipeline/rte_table_action.c b/lib/pipeline/rte_table_action.c
> index 87c3e0e2c9..b3c77dbf66 100644
> --- a/lib/pipeline/rte_table_action.c
> +++ b/lib/pipeline/rte_table_action.c
> @@ -109,7 +109,7 @@ mtr_cfg_check(struct rte_table_action_mtr_config *mtr)
>  struct mtr_trtcm_data {
>  	struct rte_meter_trtcm trtcm;
>  	uint64_t stats[RTE_COLORS];
> -} __rte_packed;
> +};
>  
>  #define MTR_TRTCM_DATA_METER_PROFILE_ID_GET(data)          \
>  	(((data)->stats[RTE_COLOR_GREEN] & 0xF8LLU) >> 3)
> -- 
> 2.43.0
> 

Re: [PATCH 4/6] pipeline: remove packed attribute

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:11 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The rte_trtcm_data structure, and its substructures, only consist of
> uint64_t types. This makes packing unnecessary to remove the packed
> attribute from the structure.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Bruce Richardson]

There are multiple instances in the DPDK app folder where we set up an
IP header and then compute the checksum field by direct addition of
nine uint16_t values in the header (20 bytes less the cksum field).
The existing rte_ip.h checksum function is more general than necessary
here and requires that the checksum field is already set to zero -
rather than having it skipped.

Fix the code duplication present in the apps by creating a new
rte_ipv4_cksum_simple function - taking the code from the existing
testpmd icmpecho.c file - and using that in app/test, testpmd and
testeventdev.

Within that new function, we can adjust slightly how the typecasting to
uint16_t is done, and thereby ensure that the app can all be compiled
without -Wno-address-of-packed-member compiler flag.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test-eventdev/test_pipeline_common.c | 25 +-----------
 app/test-pmd/icmpecho.c                  | 23 +----------
 app/test-pmd/txonly.c                    | 22 +----------
 app/test/packet_burst_generator.c        | 49 +-----------------------
 app/test/test_reassembly_perf.c          | 29 +-------------
 lib/net/rte_ip.h                         | 33 ++++++++++++++++
 6 files changed, 39 insertions(+), 142 deletions(-)

diff --git a/app/test-eventdev/test_pipeline_common.c b/app/test-eventdev/test_pipeline_common.c
index b111690b7c..204117ef7f 100644
--- a/app/test-eventdev/test_pipeline_common.c
+++ b/app/test-eventdev/test_pipeline_common.c
@@ -74,8 +74,6 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 			 struct rte_udp_hdr *udp_hdr, uint16_t pkt_data_len,
 			 uint8_t port, uint8_t flow)
 {
-	uint16_t *ptr16;
-	uint32_t ip_cksum;
 	uint16_t pkt_len;
 
 	/*
@@ -104,28 +102,7 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	/*
 	 * Compute IP header checksum.
 	 */
-	ptr16 = (unaligned_uint16_t *)ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0];
-	ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2];
-	ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6];
-	ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8];
-	ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) + (ip_cksum & 0x0000FFFF);
-	if (ip_cksum > 65535)
-		ip_cksum -= 65535;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t)ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 }
 
 static void
diff --git a/app/test-pmd/icmpecho.c b/app/test-pmd/icmpecho.c
index 68524484e3..754a3bc758 100644
--- a/app/test-pmd/icmpecho.c
+++ b/app/test-pmd/icmpecho.c
@@ -241,27 +241,6 @@ ipv4_addr_dump(const char *what, uint32_t be_ipv4_addr)
 	printf("%s", buf);
 }
 
-static uint16_t
-ipv4_hdr_cksum(struct rte_ipv4_hdr *ip_h)
-{
-	uint16_t *v16_h;
-	uint32_t ip_cksum;
-
-	/*
-	 * Compute the sum of successive 16-bit words of the IPv4 header,
-	 * skipping the checksum field of the header.
-	 */
-	v16_h = (unaligned_uint16_t *) ip_h;
-	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
-		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
-
-	/* reduce 32 bit checksum to 16 bits and complement it */
-	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
-	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;
-}
-
 #define is_multicast_ipv4_addr(ipv4_addr) \
 	(((rte_be_to_cpu_32((ipv4_addr)) >> 24) & 0x000000FF) == 0xE0)
 
@@ -458,7 +437,7 @@ reply_to_icmp_echo_rqsts(struct fwd_stream *fs)
 				ip_src = (ip_src & 0xFFFFFFFC) | 0x00000001;
 			ip_h->src_addr = rte_cpu_to_be_32(ip_src);
 			ip_h->dst_addr = ip_addr;
-			ip_h->hdr_checksum = ipv4_hdr_cksum(ip_h);
+			ip_h->hdr_checksum = rte_ipv4_cksum_simple(ip_h);
 		} else {
 			ip_h->src_addr = ip_h->dst_addr;
 			ip_h->dst_addr = ip_addr;
diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index c2b88764be..59d821a22d 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -106,8 +106,6 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 			 struct rte_udp_hdr *udp_hdr,
 			 uint16_t pkt_data_len)
 {
-	uint16_t *ptr16;
-	uint32_t ip_cksum;
 	uint16_t pkt_len;
 
 	/*
@@ -136,25 +134,7 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	/*
 	 * Compute IP header checksum.
 	 */
-	ptr16 = (unaligned_uint16_t*) ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6]; ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8]; ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-		(ip_cksum & 0x0000FFFF);
-	if (ip_cksum > 65535)
-		ip_cksum -= 65535;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 }
 
 static inline void
diff --git a/app/test/packet_burst_generator.c b/app/test/packet_burst_generator.c
index 867a88da00..6547d2a07f 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -159,8 +159,6 @@ initialize_ipv4_header(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 		uint32_t dst_addr, uint16_t pkt_data_len)
 {
 	uint16_t pkt_len;
-	unaligned_uint16_t *ptr16;
-	uint32_t ip_cksum;
 
 	/*
 	 * Initialize IP header.
@@ -177,27 +175,7 @@ initialize_ipv4_header(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 	ip_hdr->src_addr = rte_cpu_to_be_32(src_addr);
 	ip_hdr->dst_addr = rte_cpu_to_be_32(dst_addr);
 
-	/*
-	 * Compute IP header checksum.
-	 */
-	ptr16 = (unaligned_uint16_t *)ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6]; ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8]; ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-		(ip_cksum & 0x0000FFFF);
-	ip_cksum %= 65536;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 
 	return pkt_len;
 }
@@ -207,8 +185,6 @@ initialize_ipv4_header_proto(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 		uint32_t dst_addr, uint16_t pkt_data_len, uint8_t proto)
 {
 	uint16_t pkt_len;
-	unaligned_uint16_t *ptr16;
-	uint32_t ip_cksum;
 
 	/*
 	 * Initialize IP header.
@@ -224,28 +200,7 @@ initialize_ipv4_header_proto(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 	ip_hdr->total_length   = rte_cpu_to_be_16(pkt_len);
 	ip_hdr->src_addr = rte_cpu_to_be_32(src_addr);
 	ip_hdr->dst_addr = rte_cpu_to_be_32(dst_addr);
-
-	/*
-	 * Compute IP header checksum.
-	 */
-	ptr16 = (unaligned_uint16_t *)ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6]; ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8]; ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-		(ip_cksum & 0x0000FFFF);
-	ip_cksum %= 65536;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 
 	return pkt_len;
 }
diff --git a/app/test/test_reassembly_perf.c b/app/test/test_reassembly_perf.c
index 3912179022..32fee984a0 100644
--- a/app/test/test_reassembly_perf.c
+++ b/app/test/test_reassembly_perf.c
@@ -136,9 +136,7 @@ ipv4_frag_fill_data(struct rte_mbuf **mbuf, uint8_t nb_frags, uint32_t flow_id,
 	for (i = 0; i < nb_frags; i++) {
 		struct rte_mbuf *frag = mbuf[i];
 		uint16_t frag_offset = 0;
-		uint32_t ip_cksum;
 		uint16_t pkt_len;
-		uint16_t *ptr16;
 
 		frag_offset = i * (frag_len / 8);
 
@@ -189,32 +187,7 @@ ipv4_frag_fill_data(struct rte_mbuf **mbuf, uint8_t nb_frags, uint32_t flow_id,
 		ip_hdr->src_addr = rte_cpu_to_be_32(IP_SRC_ADDR(flow_id));
 		ip_hdr->dst_addr = rte_cpu_to_be_32(IP_DST_ADDR(flow_id));
 
-		/*
-		 * Compute IP header checksum.
-		 */
-		ptr16 = (unaligned_uint16_t *)ip_hdr;
-		ip_cksum = 0;
-		ip_cksum += ptr16[0];
-		ip_cksum += ptr16[1];
-		ip_cksum += ptr16[2];
-		ip_cksum += ptr16[3];
-		ip_cksum += ptr16[4];
-		ip_cksum += ptr16[6];
-		ip_cksum += ptr16[7];
-		ip_cksum += ptr16[8];
-		ip_cksum += ptr16[9];
-
-		/*
-		 * Reduce 32 bit checksum to 16 bits and complement it.
-		 */
-		ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-			   (ip_cksum & 0x0000FFFF);
-		if (ip_cksum > 65535)
-			ip_cksum -= 65535;
-		ip_cksum = (~ip_cksum) & 0x0000FFFF;
-		if (ip_cksum == 0)
-			ip_cksum = 0xFFFF;
-		ip_hdr->hdr_checksum = (uint16_t)ip_cksum;
+		ip_hdr->hdr_checksum = (uint16_t)rte_ipv4_cksum_simple(ip_hdr);
 
 		frag->data_len = sizeof(struct rte_ether_hdr) + pkt_len;
 		frag->pkt_len = frag->data_len;
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 0d103d4127..9980be8a2d 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -309,6 +309,39 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
 	return (uint16_t)~cksum;
 }
 
+/**
+ * Process the IPv4 checksum of an IPv4 header without any extensions.
+ *
+ * The checksum field does NOT have to be set by the caller, the field
+ * is skipped by the calculation.
+ *
+ * @param ipv4_hdr
+ *   The pointer to the contiguous IPv4 header.
+ * @return
+ *   The complemented checksum to set in the IP packet.
+ */
+__rte_experimental
+static inline uint16_t
+rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
+{
+	const uint16_t *v16_h;
+	uint32_t ip_cksum;
+
+	/*
+	 * Compute the sum of successive 16-bit words of the IPv4 header,
+	 * skipping the checksum field of the header.
+	 */
+	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
+	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
+		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
+
+	/* reduce 32 bit checksum to 16 bits and complement it */
+	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
+	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
+	ip_cksum = (~ip_cksum) & 0x0000FFFF;
+	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;
+}
+
 /**
  * Process the pseudo-header checksum of an IPv4 header.
  *
-- 
2.43.0

Re: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:12 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> There are multiple instances in the DPDK app folder where we set up an
> IP header and then compute the checksum field by direct addition of
> nine uint16_t values in the header (20 bytes less the cksum field).
> The existing rte_ip.h checksum function is more general than necessary
> here and requires that the checksum field is already set to zero -
> rather than having it skipped.
> 
> Fix the code duplication present in the apps by creating a new
> rte_ipv4_cksum_simple function - taking the code from the existing
> testpmd icmpecho.c file - and using that in app/test, testpmd and
> testeventdev.
> 
> Within that new function, we can adjust slightly how the typecasting to
> uint16_t is done, and thereby ensure that the app can all be compiled
> without -Wno-address-of-packed-member compiler flag.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

DPDK should use the same optimization as FreeBSD and Linux
and use 32 bit add's here.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Bruce Richardson]

On Thu, Oct 17, 2024 at 09:24:37AM -0700, Stephen Hemminger wrote:
> On Thu, 17 Oct 2024 15:22:12 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > There are multiple instances in the DPDK app folder where we set up an
> > IP header and then compute the checksum field by direct addition of
> > nine uint16_t values in the header (20 bytes less the cksum field).
> > The existing rte_ip.h checksum function is more general than necessary
> > here and requires that the checksum field is already set to zero -
> > rather than having it skipped.
> > 
> > Fix the code duplication present in the apps by creating a new
> > rte_ipv4_cksum_simple function - taking the code from the existing
> > testpmd icmpecho.c file - and using that in app/test, testpmd and
> > testeventdev.
> > 
> > Within that new function, we can adjust slightly how the typecasting to
> > uint16_t is done, and thereby ensure that the app can all be compiled
> > without -Wno-address-of-packed-member compiler flag.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> DPDK should use the same optimization as FreeBSD and Linux
> and use 32 bit add's here.
> 

Yep. And if we get this patch applied, there is only one place to optimize, not
a dozen! :-)

RE: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Morten Brørup]

> +/**
> + * Process the IPv4 checksum of an IPv4 header without any extensions.
> + *
> + * The checksum field does NOT have to be set by the caller, the field
> + * is skipped by the calculation.
> + *
> + * @param ipv4_hdr
> + *   The pointer to the contiguous IPv4 header.
> + * @return
> + *   The complemented checksum to set in the IP packet.
> + */
> +__rte_experimental
> +static inline uint16_t
> +rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
> +{
> +	const uint16_t *v16_h;
> +	uint32_t ip_cksum;
> +
> +	/*
> +	 * Compute the sum of successive 16-bit words of the IPv4 header,
> +	 * skipping the checksum field of the header.
> +	 */
> +	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> +	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
> +		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> +
> +	/* reduce 32 bit checksum to 16 bits and complement it */
> +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> +	ip_cksum = (~ip_cksum) & 0x0000FFFF;
> +	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;

The zero exception does not apply to the checksum stored in the IP header, only to the checksum in the UDP header.

> +}

Besides that, for the series,
Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Bruce Richardson]

On Thu, Oct 17, 2024 at 07:15:10PM +0200, Morten Brørup wrote:
> > +/**
> > + * Process the IPv4 checksum of an IPv4 header without any extensions.
> > + *
> > + * The checksum field does NOT have to be set by the caller, the field
> > + * is skipped by the calculation.
> > + *
> > + * @param ipv4_hdr
> > + *   The pointer to the contiguous IPv4 header.
> > + * @return
> > + *   The complemented checksum to set in the IP packet.
> > + */
> > +__rte_experimental
> > +static inline uint16_t
> > +rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
> > +{
> > +	const uint16_t *v16_h;
> > +	uint32_t ip_cksum;
> > +
> > +	/*
> > +	 * Compute the sum of successive 16-bit words of the IPv4 header,
> > +	 * skipping the checksum field of the header.
> > +	 */
> > +	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > +	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
> > +		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> > +
> > +	/* reduce 32 bit checksum to 16 bits and complement it */
> > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > +	ip_cksum = (~ip_cksum) & 0x0000FFFF;
> > +	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;
> 
> The zero exception does not apply to the checksum stored in the IP header, only to the checksum in the UDP header.
> 

I was wondering about that, because I didn't see it mentioned anywhere in
the RFCs I consulted, but on the other hand all the implementations in the
code seemed to have the check for zero.

> > +}
> 
> Besides that, for the series,

So, just to confirm, the zero check at the end of the new ip_cksum_simple
function should be removed and we always return the computed value
directly?

> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

Re: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Stephen Hemminger]

On Thu, 17 Oct 2024 20:03:13 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Oct 17, 2024 at 07:15:10PM +0200, Morten Brørup wrote:
> > > +/**
> > > + * Process the IPv4 checksum of an IPv4 header without any extensions.
> > > + *
> > > + * The checksum field does NOT have to be set by the caller, the field
> > > + * is skipped by the calculation.
> > > + *
> > > + * @param ipv4_hdr
> > > + *   The pointer to the contiguous IPv4 header.
> > > + * @return
> > > + *   The complemented checksum to set in the IP packet.
> > > + */
> > > +__rte_experimental
> > > +static inline uint16_t
> > > +rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
> > > +{
> > > +	const uint16_t *v16_h;
> > > +	uint32_t ip_cksum;
> > > +
> > > +	/*
> > > +	 * Compute the sum of successive 16-bit words of the IPv4 header,
> > > +	 * skipping the checksum field of the header.
> > > +	 */
> > > +	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > > +	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
> > > +		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> > > +
> > > +	/* reduce 32 bit checksum to 16 bits and complement it */
> > > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > > +	ip_cksum = (~ip_cksum) & 0x0000FFFF;
> > > +	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;  
> > 
> > The zero exception does not apply to the checksum stored in the IP header, only to the checksum in the UDP header.
> >   
> 
> I was wondering about that, because I didn't see it mentioned anywhere in
> the RFCs I consulted, but on the other hand all the implementations in the
> code seemed to have the check for zero.
> 
> > > +}  
> > 
> > Besides that, for the series,  
> 
> So, just to confirm, the zero check at the end of the new ip_cksum_simple
> function should be removed and we always return the computed value
> directly?

Depends on usage.
  - if the computed value is zero, then 0xffff should be placed in
    the IP header.
  - often code use ip checksum code to see if incoming checksum is good.
    in that case zero means the checksum is valid.

RE: [PATCH 5/6] net: add smaller IPv4 cksum function for simple cases

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 17 October 2024 21.35
> 
> On Thu, 17 Oct 2024 20:03:13 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > On Thu, Oct 17, 2024 at 07:15:10PM +0200, Morten Brørup wrote:
> > > > +/**
> > > > + * Process the IPv4 checksum of an IPv4 header without any
> extensions.
> > > > + *
> > > > + * The checksum field does NOT have to be set by the caller, the
> field
> > > > + * is skipped by the calculation.
> > > > + *
> > > > + * @param ipv4_hdr
> > > > + *   The pointer to the contiguous IPv4 header.
> > > > + * @return
> > > > + *   The complemented checksum to set in the IP packet.
> > > > + */
> > > > +__rte_experimental
> > > > +static inline uint16_t
> > > > +rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
> > > > +{
> > > > +	const uint16_t *v16_h;
> > > > +	uint32_t ip_cksum;
> > > > +
> > > > +	/*
> > > > +	 * Compute the sum of successive 16-bit words of the IPv4
> header,
> > > > +	 * skipping the checksum field of the header.
> > > > +	 */
> > > > +	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > > > +	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
> > > > +		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
> > > > +
> > > > +	/* reduce 32 bit checksum to 16 bits and complement it */
> > > > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > > > +	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
> > > > +	ip_cksum = (~ip_cksum) & 0x0000FFFF;
> > > > +	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;
> > >
> > > The zero exception does not apply to the checksum stored in the IP
> header, only to the checksum in the UDP header.
> > >
> >
> > I was wondering about that, because I didn't see it mentioned
> anywhere in
> > the RFCs I consulted, but on the other hand all the implementations
> in the
> > code seemed to have the check for zero.

Copy-pasted including the bug from the originating code.

> >
> > > > +}
> > >
> > > Besides that, for the series,
> >
> > So, just to confirm, the zero check at the end of the new
> ip_cksum_simple
> > function should be removed and we always return the computed value
> > directly?

Agree.

> 
> Depends on usage.
>   - if the computed value is zero, then 0xffff should be placed in
>     the IP header.

Not for the IP header, no.
For the UDP header, yes.

The Linux kernel doesn't do it to the IP header:
https://elixir.bootlin.com/linux/v6.12-rc3/source/net/ipv4/ip_output.c#L96
https://elixir.bootlin.com/linux/v6.12-rc3/source/lib/checksum.c#L108

>   - often code use ip checksum code to see if incoming checksum is
> good.
>     in that case zero means the checksum is valid.

I don't remember all the details, but when checking the checksum, there's something about adding 0xffff or 0x0000 yields the same result. E.g.

1 + 0xffff = 0x10000, and then 0x1 + 0x0000 = 0x0001.
1 + 0x0000 = 0x0001.

So, when checking, it doesn't matter if the header checksum field is 0xffff or 0x0000.

There are other rules for the UDP header's checksum field, because 0x0000 there has the special meaning that the UDP checksum is not present/valid. (And AFAIR, not for IPv6, because the checksum is not optional for IPv6 UDP packets.)

The above calculation is also the reason why it is safe to substitute 0x0000 by 0xffff in the UDP header; it will still yield the same result when checking the checksum.

[PATCH 6/6] build: limit scope of packed member warning disabling

[Bruce Richardson]

The flag '-Wno-address-of-packed-member' is a global warning flag in
DPDK. Rather than disabling this warning globally, it is better to just
have it enabled for components that may need it. This patch limits the
scope of it in the following ways:

* limit the use of the flag to the drivers subfolder only - all libs and
  apps should be buildable without the warning.
* exception is made for the vhost library and the ipsec-secgw example
  for now, as making them buildable with the warning enabled is more
  complicated. This exception can hopefully be removed in future.

Limiting the scope further within the drivers directory is also left for
future consideration. However, since HW drivers often have to use packed
structures to align with hardware-implemented data layouts, it may be
more practical to keep the warning disabled in the longer term.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 config/meson.build               | 1 -
 drivers/meson.build              | 9 ++++++---
 examples/ipsec-secgw/meson.build | 6 ++++++
 lib/vhost/meson.build            | 5 ++++-
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/config/meson.build b/config/meson.build
index 6a6b56a27e..8b1a0fe779 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -325,7 +325,6 @@ warning_flags = [
         '-Wwrite-strings',
 
         # globally disabled warnings
-        '-Wno-address-of-packed-member',
         '-Wno-packed-not-aligned',
         '-Wno-missing-field-initializers',
 ]
diff --git a/drivers/meson.build b/drivers/meson.build
index 2733306698..1653990f3b 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -59,9 +59,12 @@ default_cflags = machine_args
 default_cflags += ['-DALLOW_EXPERIMENTAL_API']
 default_cflags += ['-DALLOW_INTERNAL_API']
 
-if cc.has_argument('-Wno-format-truncation')
-    default_cflags += '-Wno-format-truncation'
-endif
+warning_disable_cflags = ['-Wno-format-truncation', '-Wno-address-of-packed-member']
+foreach cflag:warning_disable_cflags
+    if cc.has_argument(cflag)
+        default_cflags += cflag
+    endif
+endforeach
 
 dpdk_drivers_build_dir = meson.current_build_dir()
 
diff --git a/examples/ipsec-secgw/meson.build b/examples/ipsec-secgw/meson.build
index ccdaef1c4d..023d9cf039 100644
--- a/examples/ipsec-secgw/meson.build
+++ b/examples/ipsec-secgw/meson.build
@@ -23,3 +23,9 @@ sources = files(
         'sp4.c',
         'sp6.c',
 )
+app_cflags = ['-Wno-address-of-packed-member']
+foreach flag:app_cflags
+    if cc.has_argument(flag)
+        cflags += flag
+    endif
+endforeach
diff --git a/lib/vhost/meson.build b/lib/vhost/meson.build
index 41b622a9be..51bcf17244 100644
--- a/lib/vhost/meson.build
+++ b/lib/vhost/meson.build
@@ -16,7 +16,10 @@ elif (toolchain == 'icc' and cc.version().version_compare('>=16.0.0'))
     cflags += '-DVHOST_ICC_UNROLL_PRAGMA'
 endif
 dpdk_conf.set('RTE_LIBRTE_VHOST_POSTCOPY', cc.has_header('linux/userfaultfd.h'))
-cflags += '-fno-strict-aliasing'
+cflags += [
+        '-fno-strict-aliasing',
+        '-Wno-address-of-packed-member',
+]
 
 sources = files(
         'fd_man.c',
-- 
2.43.0

Re: [PATCH 6/6] build: limit scope of packed member warning disabling

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:13 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The flag '-Wno-address-of-packed-member' is a global warning flag in
> DPDK. Rather than disabling this warning globally, it is better to just
> have it enabled for components that may need it. This patch limits the
> scope of it in the following ways:
> 
> * limit the use of the flag to the drivers subfolder only - all libs and
>   apps should be buildable without the warning.
> * exception is made for the vhost library and the ipsec-secgw example
>   for now, as making them buildable with the warning enabled is more
>   complicated. This exception can hopefully be removed in future.
> 
> Limiting the scope further within the drivers directory is also left for
> future consideration. However, since HW drivers often have to use packed
> structures to align with hardware-implemented data layouts, it may be
> more practical to keep the warning disabled in the longer term.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  config/meson.build               | 1 -
>  drivers/meson.build              | 9 ++++++---
>  examples/ipsec-secgw/meson.build | 6 ++++++
>  lib/vhost/meson.build            | 5 ++++-
>  4 files changed, 16 insertions(+), 5 deletions(-)

Good first step, followups can fix ipsec and vhost later.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 0/6] Reduce scope address-of-packed-member warning

[Stephen Hemminger]

On Thu, 17 Oct 2024 15:22:07 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The warning for address-of-packed-member was being disabled globally in
> DPDK.  While for drivers which need to access hardware-defined
> data-structures the use of packed may make sense, for normal libs and
> applications the use of packed data should be generally avoided.
> 
> This patchset initially applies some fixes for cases where we are
> unnecessarily causing the warning to trigger. Thereafter the 6th patch
> removes the global enabling of the warning and replaces it with more
> selective disabling for drivers and for a couple of other components
> which have not yet been fixed.
> 
> Bruce Richardson (6):
>   ip_frag: remove use of unaligned variable
>   efd: remove unnecessary packed attributes
>   bus/ifpga: remove packed attribute
>   pipeline: remove packed attribute
>   net: add smaller IPv4 cksum function for simple cases
>   build: limit scope of packed member warning disabling
> 
>  app/test-eventdev/test_pipeline_common.c | 25 +-----------
>  app/test-pmd/icmpecho.c                  | 23 +----------
>  app/test-pmd/txonly.c                    | 22 +----------
>  app/test/packet_burst_generator.c        | 49 +-----------------------
>  app/test/test_reassembly_perf.c          | 29 +-------------
>  config/meson.build                       |  1 -
>  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
>  drivers/meson.build                      |  9 +++--
>  examples/ipsec-secgw/meson.build         |  6 +++
>  lib/efd/rte_efd.c                        |  4 +-
>  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
>  lib/net/rte_ip.h                         | 33 ++++++++++++++++
>  lib/pipeline/rte_table_action.c          |  2 +-
>  lib/vhost/meson.build                    |  5 ++-
>  14 files changed, 60 insertions(+), 154 deletions(-)


Any chance to just fix all the places and have it enabled globally?

Re: [PATCH 0/6] Reduce scope address-of-packed-member warning

[Bruce Richardson]

On Thu, Oct 17, 2024 at 09:21:48AM -0700, Stephen Hemminger wrote:
> On Thu, 17 Oct 2024 15:22:07 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > The warning for address-of-packed-member was being disabled globally in
> > DPDK.  While for drivers which need to access hardware-defined
> > data-structures the use of packed may make sense, for normal libs and
> > applications the use of packed data should be generally avoided.
> > 
> > This patchset initially applies some fixes for cases where we are
> > unnecessarily causing the warning to trigger. Thereafter the 6th patch
> > removes the global enabling of the warning and replaces it with more
> > selective disabling for drivers and for a couple of other components
> > which have not yet been fixed.
> > 
> > Bruce Richardson (6):
> >   ip_frag: remove use of unaligned variable
> >   efd: remove unnecessary packed attributes
> >   bus/ifpga: remove packed attribute
> >   pipeline: remove packed attribute
> >   net: add smaller IPv4 cksum function for simple cases
> >   build: limit scope of packed member warning disabling
> > 
> >  app/test-eventdev/test_pipeline_common.c | 25 +-----------
> >  app/test-pmd/icmpecho.c                  | 23 +----------
> >  app/test-pmd/txonly.c                    | 22 +----------
> >  app/test/packet_burst_generator.c        | 49 +-----------------------
> >  app/test/test_reassembly_perf.c          | 29 +-------------
> >  config/meson.build                       |  1 -
> >  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
> >  drivers/meson.build                      |  9 +++--
> >  examples/ipsec-secgw/meson.build         |  6 +++
> >  lib/efd/rte_efd.c                        |  4 +-
> >  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
> >  lib/net/rte_ip.h                         | 33 ++++++++++++++++
> >  lib/pipeline/rte_table_action.c          |  2 +-
> >  lib/vhost/meson.build                    |  5 ++-
> >  14 files changed, 60 insertions(+), 154 deletions(-)
> 
> 
> Any chance to just fix all the places and have it enabled globally?

Not any time soon (at least for me). There are a huge number of drivers
with lots of errors if we turn back on the warning. We should look at all
our common structures first to ensure none of them are unnecessarily
packed.

/Bruce

Re: [PATCH 0/6] Reduce scope address-of-packed-member warning

[David Marchand]

On Thu, Oct 17, 2024 at 4:22 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> The warning for address-of-packed-member was being disabled globally in
> DPDK.  While for drivers which need to access hardware-defined
> data-structures the use of packed may make sense, for normal libs and
> applications the use of packed data should be generally avoided.
>
> This patchset initially applies some fixes for cases where we are
> unnecessarily causing the warning to trigger. Thereafter the 6th patch
> removes the global enabling of the warning and replaces it with more
> selective disabling for drivers and for a couple of other components
> which have not yet been fixed.
>
> Bruce Richardson (6):
>   ip_frag: remove use of unaligned variable
>   efd: remove unnecessary packed attributes
>   bus/ifpga: remove packed attribute
>   pipeline: remove packed attribute
>   net: add smaller IPv4 cksum function for simple cases
>   build: limit scope of packed member warning disabling
>
>  app/test-eventdev/test_pipeline_common.c | 25 +-----------
>  app/test-pmd/icmpecho.c                  | 23 +----------
>  app/test-pmd/txonly.c                    | 22 +----------
>  app/test/packet_burst_generator.c        | 49 +-----------------------
>  app/test/test_reassembly_perf.c          | 29 +-------------
>  config/meson.build                       |  1 -
>  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
>  drivers/meson.build                      |  9 +++--
>  examples/ipsec-secgw/meson.build         |  6 +++
>  lib/efd/rte_efd.c                        |  4 +-
>  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
>  lib/net/rte_ip.h                         | 33 ++++++++++++++++
>  lib/pipeline/rte_table_action.c          |  2 +-
>  lib/vhost/meson.build                    |  5 ++-
>  14 files changed, 60 insertions(+), 154 deletions(-)

Recheck-request: rebase=main,iol-unit-amd64-testing,iol-unit-arm64-testing


-- 
David Marchand

Re: [PATCH 0/6] Reduce scope address-of-packed-member warning

[David Marchand]

On Fri, Oct 25, 2024 at 3:24 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Thu, Oct 17, 2024 at 4:22 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > The warning for address-of-packed-member was being disabled globally in
> > DPDK.  While for drivers which need to access hardware-defined
> > data-structures the use of packed may make sense, for normal libs and
> > applications the use of packed data should be generally avoided.
> >
> > This patchset initially applies some fixes for cases where we are
> > unnecessarily causing the warning to trigger. Thereafter the 6th patch
> > removes the global enabling of the warning and replaces it with more
> > selective disabling for drivers and for a couple of other components
> > which have not yet been fixed.
> >
> > Bruce Richardson (6):
> >   ip_frag: remove use of unaligned variable
> >   efd: remove unnecessary packed attributes
> >   bus/ifpga: remove packed attribute
> >   pipeline: remove packed attribute
> >   net: add smaller IPv4 cksum function for simple cases
> >   build: limit scope of packed member warning disabling
> >
> >  app/test-eventdev/test_pipeline_common.c | 25 +-----------
> >  app/test-pmd/icmpecho.c                  | 23 +----------
> >  app/test-pmd/txonly.c                    | 22 +----------
> >  app/test/packet_burst_generator.c        | 49 +-----------------------
> >  app/test/test_reassembly_perf.c          | 29 +-------------
> >  config/meson.build                       |  1 -
> >  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
> >  drivers/meson.build                      |  9 +++--
> >  examples/ipsec-secgw/meson.build         |  6 +++
> >  lib/efd/rte_efd.c                        |  4 +-
> >  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
> >  lib/net/rte_ip.h                         | 33 ++++++++++++++++
> >  lib/pipeline/rte_table_action.c          |  2 +-
> >  lib/vhost/meson.build                    |  5 ++-
> >  14 files changed, 60 insertions(+), 154 deletions(-)
>
> Recheck-request: rebase=main,iol-unit-amd64-testing,iol-unit-arm64-testing

Unfortunately, there is a small conflict and the CI can't retest this series.
Bruce, can you send a v2 please?


-- 
David Marchand

Re: [PATCH 0/6] Reduce scope address-of-packed-member warning

[Bruce Richardson]

On Fri, Oct 25, 2024 at 04:55:08PM +0200, David Marchand wrote:
> On Fri, Oct 25, 2024 at 3:24 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Thu, Oct 17, 2024 at 4:22 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > The warning for address-of-packed-member was being disabled globally in
> > > DPDK.  While for drivers which need to access hardware-defined
> > > data-structures the use of packed may make sense, for normal libs and
> > > applications the use of packed data should be generally avoided.
> > >
> > > This patchset initially applies some fixes for cases where we are
> > > unnecessarily causing the warning to trigger. Thereafter the 6th patch
> > > removes the global enabling of the warning and replaces it with more
> > > selective disabling for drivers and for a couple of other components
> > > which have not yet been fixed.
> > >
> > > Bruce Richardson (6):
> > >   ip_frag: remove use of unaligned variable
> > >   efd: remove unnecessary packed attributes
> > >   bus/ifpga: remove packed attribute
> > >   pipeline: remove packed attribute
> > >   net: add smaller IPv4 cksum function for simple cases
> > >   build: limit scope of packed member warning disabling
> > >
> > >  app/test-eventdev/test_pipeline_common.c | 25 +-----------
> > >  app/test-pmd/icmpecho.c                  | 23 +----------
> > >  app/test-pmd/txonly.c                    | 22 +----------
> > >  app/test/packet_burst_generator.c        | 49 +-----------------------
> > >  app/test/test_reassembly_perf.c          | 29 +-------------
> > >  config/meson.build                       |  1 -
> > >  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
> > >  drivers/meson.build                      |  9 +++--
> > >  examples/ipsec-secgw/meson.build         |  6 +++
> > >  lib/efd/rte_efd.c                        |  4 +-
> > >  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
> > >  lib/net/rte_ip.h                         | 33 ++++++++++++++++
> > >  lib/pipeline/rte_table_action.c          |  2 +-
> > >  lib/vhost/meson.build                    |  5 ++-
> > >  14 files changed, 60 insertions(+), 154 deletions(-)
> >
> > Recheck-request: rebase=main,iol-unit-amd64-testing,iol-unit-arm64-testing
> 
> Unfortunately, there is a small conflict and the CI can't retest this series.
> Bruce, can you send a v2 please?
> 
Done. V2 is rebased and also includes the checksum calc adjustment reported
by Morten.

/Bruce

[PATCH v2 0/6] Reduce scope address-of-packed-member warning

[Bruce Richardson]

The warning for address-of-packed-member was being disabled globally in
DPDK.  While for drivers which need to access hardware-defined
data-structures the use of packed may make sense, for normal libs and
applications the use of packed data should be generally avoided.

This patchset initially applies some fixes for cases where we are
unnecessarily causing the warning to trigger. Thereafter the 6th patch
removes the global enabling of the warning and replaces it with more
selective disabling for drivers and for a couple of other components
which have not yet been fixed.

v2:
* rebase on latest main (rte_ip.h -> rte_ip4.h)
* remove unnecessary 0-check in IP checksum calc

Bruce Richardson (6):
  ip_frag: remove use of unaligned variable
  efd: remove unnecessary packed attributes
  bus/ifpga: remove packed attribute
  pipeline: remove packed attribute
  net: add smaller IPv4 cksum function for simple cases
  build: limit scope of packed member warning disabling

 app/test-eventdev/test_pipeline_common.c | 25 +-----------
 app/test-pmd/icmpecho.c                  | 23 +----------
 app/test-pmd/txonly.c                    | 22 +----------
 app/test/packet_burst_generator.c        | 49 +-----------------------
 app/test/test_reassembly_perf.c          | 29 +-------------
 config/meson.build                       |  1 -
 drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
 drivers/meson.build                      |  9 +++--
 examples/ipsec-secgw/meson.build         |  6 +++
 lib/efd/rte_efd.c                        |  4 +-
 lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
 lib/net/rte_ip4.h                        | 32 ++++++++++++++++
 lib/pipeline/rte_table_action.c          |  2 +-
 lib/vhost/meson.build                    |  5 ++-
 14 files changed, 59 insertions(+), 154 deletions(-)

--
2.43.0

[PATCH v2 1/6] ip_frag: remove use of unaligned variable

[Bruce Richardson]

If compiling with -Waddress-of-packed-member, we get a warning about the
use of the unaligned uint64_t value which is used to copy 8 bytes from
ip_hdr to the key. Replace this unaligned assignment with an equivalent
8-byte constant-sized memcpy, allowing the compiler to choose optimal
instructions to do the assignment.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ip_frag/rte_ipv4_reassembly.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/ip_frag/rte_ipv4_reassembly.c b/lib/ip_frag/rte_ipv4_reassembly.c
index 4a89a5f536..5818f50f40 100644
--- a/lib/ip_frag/rte_ipv4_reassembly.c
+++ b/lib/ip_frag/rte_ipv4_reassembly.c
@@ -101,7 +101,6 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 {
 	struct ip_frag_pkt *fp;
 	struct ip_frag_key key;
-	const unaligned_uint64_t *psd;
 	uint16_t flag_offset, ip_ofs, ip_flag;
 	int32_t ip_len;
 	int32_t trim;
@@ -110,9 +109,8 @@ rte_ipv4_frag_reassemble_packet(struct rte_ip_frag_tbl *tbl,
 	ip_ofs = (uint16_t)(flag_offset & RTE_IPV4_HDR_OFFSET_MASK);
 	ip_flag = (uint16_t)(flag_offset & RTE_IPV4_HDR_MF_FLAG);
 
-	psd = (unaligned_uint64_t *)&ip_hdr->src_addr;
 	/* use first 8 bytes only */
-	key.src_dst[0] = psd[0];
+	memcpy(&key.src_dst[0], &ip_hdr->src_addr, 8);
 	key.id = ip_hdr->packet_id;
 	key.key_len = IPV4_KEYLEN;
 
-- 
2.43.0

[PATCH v2 2/6] efd: remove unnecessary packed attributes

[Bruce Richardson]

The structure "efd_online_group_entry" only consists of values which are
typedefs of "uint16_t", so packing the structure has no effect. The
"efd_online_chunk" structure has a mix of "uint8_t" and the
"efd_online_group_entry" struct, i.e. uint16_t values, but since the
first, uint8_t, member array is of even size, the packed attribute does
not affect the structure layout.

Removing these packed attributes allows the library to compile cleanly
without "-Wno-address-of-packed-member" compiler flag.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/efd/rte_efd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efd/rte_efd.c b/lib/efd/rte_efd.c
index d3b732f2e8..3cbb3c2719 100644
--- a/lib/efd/rte_efd.c
+++ b/lib/efd/rte_efd.c
@@ -212,7 +212,7 @@ struct efd_offline_chunk_rules {
 struct efd_online_group_entry {
 	efd_hashfunc_t hash_idx[RTE_EFD_VALUE_NUM_BITS];
 	efd_lookuptbl_t lookup_table[RTE_EFD_VALUE_NUM_BITS];
-} __rte_packed;
+};
 
 /**
  * A single chunk record, containing EFD_TARGET_CHUNK_NUM_RULES rules.
@@ -228,7 +228,7 @@ struct efd_online_chunk {
 
 	struct efd_online_group_entry groups[EFD_CHUNK_NUM_GROUPS];
 	/**< Array of all the groups in the chunk. */
-} __rte_packed;
+};
 
 /**
  * EFD table structure
-- 
2.43.0

[PATCH v2 3/6] bus/ifpga: remove packed attribute

[Bruce Richardson]

The struct rte_afu_device does not need to be packed, so remove the
packed attribute from it.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Rosen Xu <rosen.xu@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 drivers/bus/ifpga/bus_ifpga_driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/ifpga/bus_ifpga_driver.h b/drivers/bus/ifpga/bus_ifpga_driver.h
index a42afc7d75..af151ffd4b 100644
--- a/drivers/bus/ifpga/bus_ifpga_driver.h
+++ b/drivers/bus/ifpga/bus_ifpga_driver.h
@@ -77,7 +77,7 @@ struct rte_afu_device {
 	struct rte_intr_handle *intr_handle;     /**< Interrupt handle */
 	struct rte_afu_driver *driver;          /**< Associated driver */
 	char path[IFPGA_BUS_BITSTREAM_PATH_MAX_LEN];
-} __rte_packed;
+};
 
 /**
  * @internal
-- 
2.43.0

[PATCH v2 4/6] pipeline: remove packed attribute

[Bruce Richardson]

The rte_trtcm_data structure, and its substructures, only consist of
uint64_t types. This makes packing unnecessary to remove the packed
attribute from the structure.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/pipeline/rte_table_action.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/pipeline/rte_table_action.c b/lib/pipeline/rte_table_action.c
index a7e63b9846..a431f8f128 100644
--- a/lib/pipeline/rte_table_action.c
+++ b/lib/pipeline/rte_table_action.c
@@ -109,7 +109,7 @@ mtr_cfg_check(struct rte_table_action_mtr_config *mtr)
 struct mtr_trtcm_data {
 	struct rte_meter_trtcm trtcm;
 	uint64_t stats[RTE_COLORS];
-} __rte_packed;
+};
 
 #define MTR_TRTCM_DATA_METER_PROFILE_ID_GET(data)          \
 	(((data)->stats[RTE_COLOR_GREEN] & 0xF8LLU) >> 3)
-- 
2.43.0

[PATCH v2 5/6] net: add smaller IPv4 cksum function for simple cases

[Bruce Richardson]

There are multiple instances in the DPDK app folder where we set up an
IP header and then compute the checksum field by direct addition of
nine uint16_t values in the header (20 bytes less the cksum field).
The existing rte_ip.h checksum function is more general than necessary
here and requires that the checksum field is already set to zero -
rather than having it skipped.

Fix the code duplication present in the apps by creating a new
rte_ipv4_cksum_simple function - taking the code from the existing
testpmd icmpecho.c file - and using that in app/test, testpmd and
testeventdev.

Within that new function, we can adjust slightly how the typecasting to
uint16_t is done, and thereby ensure that the app can all be compiled
without -Wno-address-of-packed-member compiler flag.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test-eventdev/test_pipeline_common.c | 25 +-----------
 app/test-pmd/icmpecho.c                  | 23 +----------
 app/test-pmd/txonly.c                    | 22 +----------
 app/test/packet_burst_generator.c        | 49 +-----------------------
 app/test/test_reassembly_perf.c          | 29 +-------------
 lib/net/rte_ip4.h                        | 32 ++++++++++++++++
 6 files changed, 38 insertions(+), 142 deletions(-)

diff --git a/app/test-eventdev/test_pipeline_common.c b/app/test-eventdev/test_pipeline_common.c
index b111690b7c..204117ef7f 100644
--- a/app/test-eventdev/test_pipeline_common.c
+++ b/app/test-eventdev/test_pipeline_common.c
@@ -74,8 +74,6 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 			 struct rte_udp_hdr *udp_hdr, uint16_t pkt_data_len,
 			 uint8_t port, uint8_t flow)
 {
-	uint16_t *ptr16;
-	uint32_t ip_cksum;
 	uint16_t pkt_len;
 
 	/*
@@ -104,28 +102,7 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	/*
 	 * Compute IP header checksum.
 	 */
-	ptr16 = (unaligned_uint16_t *)ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0];
-	ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2];
-	ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6];
-	ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8];
-	ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) + (ip_cksum & 0x0000FFFF);
-	if (ip_cksum > 65535)
-		ip_cksum -= 65535;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t)ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 }
 
 static void
diff --git a/app/test-pmd/icmpecho.c b/app/test-pmd/icmpecho.c
index 4ef23ae67a..c87b7a80df 100644
--- a/app/test-pmd/icmpecho.c
+++ b/app/test-pmd/icmpecho.c
@@ -241,27 +241,6 @@ ipv4_addr_dump(const char *what, uint32_t be_ipv4_addr)
 	printf("%s", buf);
 }
 
-static uint16_t
-ipv4_hdr_cksum(struct rte_ipv4_hdr *ip_h)
-{
-	uint16_t *v16_h;
-	uint32_t ip_cksum;
-
-	/*
-	 * Compute the sum of successive 16-bit words of the IPv4 header,
-	 * skipping the checksum field of the header.
-	 */
-	v16_h = (unaligned_uint16_t *) ip_h;
-	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
-		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
-
-	/* reduce 32 bit checksum to 16 bits and complement it */
-	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
-	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	return (ip_cksum == 0) ? 0xFFFF : (uint16_t) ip_cksum;
-}
-
 #define is_multicast_ipv4_addr(ipv4_addr) \
 	(((rte_be_to_cpu_32((ipv4_addr)) >> 24) & 0x000000FF) == 0xE0)
 
@@ -458,7 +437,7 @@ reply_to_icmp_echo_rqsts(struct fwd_stream *fs)
 				ip_src = (ip_src & 0xFFFFFFFC) | 0x00000001;
 			ip_h->src_addr = rte_cpu_to_be_32(ip_src);
 			ip_h->dst_addr = ip_addr;
-			ip_h->hdr_checksum = ipv4_hdr_cksum(ip_h);
+			ip_h->hdr_checksum = rte_ipv4_cksum_simple(ip_h);
 		} else {
 			ip_h->src_addr = ip_h->dst_addr;
 			ip_h->dst_addr = ip_addr;
diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index c2b88764be..59d821a22d 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -106,8 +106,6 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 			 struct rte_udp_hdr *udp_hdr,
 			 uint16_t pkt_data_len)
 {
-	uint16_t *ptr16;
-	uint32_t ip_cksum;
 	uint16_t pkt_len;
 
 	/*
@@ -136,25 +134,7 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	/*
 	 * Compute IP header checksum.
 	 */
-	ptr16 = (unaligned_uint16_t*) ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6]; ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8]; ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-		(ip_cksum & 0x0000FFFF);
-	if (ip_cksum > 65535)
-		ip_cksum -= 65535;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 }
 
 static inline void
diff --git a/app/test/packet_burst_generator.c b/app/test/packet_burst_generator.c
index c9ff5257f0..4c17737739 100644
--- a/app/test/packet_burst_generator.c
+++ b/app/test/packet_burst_generator.c
@@ -159,8 +159,6 @@ initialize_ipv4_header(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 		uint32_t dst_addr, uint16_t pkt_data_len)
 {
 	uint16_t pkt_len;
-	unaligned_uint16_t *ptr16;
-	uint32_t ip_cksum;
 
 	/*
 	 * Initialize IP header.
@@ -177,27 +175,7 @@ initialize_ipv4_header(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 	ip_hdr->src_addr = rte_cpu_to_be_32(src_addr);
 	ip_hdr->dst_addr = rte_cpu_to_be_32(dst_addr);
 
-	/*
-	 * Compute IP header checksum.
-	 */
-	ptr16 = (unaligned_uint16_t *)ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6]; ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8]; ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-		(ip_cksum & 0x0000FFFF);
-	ip_cksum %= 65536;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 
 	return pkt_len;
 }
@@ -207,8 +185,6 @@ initialize_ipv4_header_proto(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 		uint32_t dst_addr, uint16_t pkt_data_len, uint8_t proto)
 {
 	uint16_t pkt_len;
-	unaligned_uint16_t *ptr16;
-	uint32_t ip_cksum;
 
 	/*
 	 * Initialize IP header.
@@ -224,28 +200,7 @@ initialize_ipv4_header_proto(struct rte_ipv4_hdr *ip_hdr, uint32_t src_addr,
 	ip_hdr->total_length   = rte_cpu_to_be_16(pkt_len);
 	ip_hdr->src_addr = rte_cpu_to_be_32(src_addr);
 	ip_hdr->dst_addr = rte_cpu_to_be_32(dst_addr);
-
-	/*
-	 * Compute IP header checksum.
-	 */
-	ptr16 = (unaligned_uint16_t *)ip_hdr;
-	ip_cksum = 0;
-	ip_cksum += ptr16[0]; ip_cksum += ptr16[1];
-	ip_cksum += ptr16[2]; ip_cksum += ptr16[3];
-	ip_cksum += ptr16[4];
-	ip_cksum += ptr16[6]; ip_cksum += ptr16[7];
-	ip_cksum += ptr16[8]; ip_cksum += ptr16[9];
-
-	/*
-	 * Reduce 32 bit checksum to 16 bits and complement it.
-	 */
-	ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-		(ip_cksum & 0x0000FFFF);
-	ip_cksum %= 65536;
-	ip_cksum = (~ip_cksum) & 0x0000FFFF;
-	if (ip_cksum == 0)
-		ip_cksum = 0xFFFF;
-	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
+	ip_hdr->hdr_checksum = rte_ipv4_cksum_simple(ip_hdr);
 
 	return pkt_len;
 }
diff --git a/app/test/test_reassembly_perf.c b/app/test/test_reassembly_perf.c
index 245ab3fa7d..69cf029468 100644
--- a/app/test/test_reassembly_perf.c
+++ b/app/test/test_reassembly_perf.c
@@ -136,9 +136,7 @@ ipv4_frag_fill_data(struct rte_mbuf **mbuf, uint8_t nb_frags, uint32_t flow_id,
 	for (i = 0; i < nb_frags; i++) {
 		struct rte_mbuf *frag = mbuf[i];
 		uint16_t frag_offset = 0;
-		uint32_t ip_cksum;
 		uint16_t pkt_len;
-		uint16_t *ptr16;
 
 		frag_offset = i * (frag_len / 8);
 
@@ -189,32 +187,7 @@ ipv4_frag_fill_data(struct rte_mbuf **mbuf, uint8_t nb_frags, uint32_t flow_id,
 		ip_hdr->src_addr = rte_cpu_to_be_32(IP_SRC_ADDR(flow_id));
 		ip_hdr->dst_addr = rte_cpu_to_be_32(IP_DST_ADDR(flow_id));
 
-		/*
-		 * Compute IP header checksum.
-		 */
-		ptr16 = (unaligned_uint16_t *)ip_hdr;
-		ip_cksum = 0;
-		ip_cksum += ptr16[0];
-		ip_cksum += ptr16[1];
-		ip_cksum += ptr16[2];
-		ip_cksum += ptr16[3];
-		ip_cksum += ptr16[4];
-		ip_cksum += ptr16[6];
-		ip_cksum += ptr16[7];
-		ip_cksum += ptr16[8];
-		ip_cksum += ptr16[9];
-
-		/*
-		 * Reduce 32 bit checksum to 16 bits and complement it.
-		 */
-		ip_cksum = ((ip_cksum & 0xFFFF0000) >> 16) +
-			   (ip_cksum & 0x0000FFFF);
-		if (ip_cksum > 65535)
-			ip_cksum -= 65535;
-		ip_cksum = (~ip_cksum) & 0x0000FFFF;
-		if (ip_cksum == 0)
-			ip_cksum = 0xFFFF;
-		ip_hdr->hdr_checksum = (uint16_t)ip_cksum;
+		ip_hdr->hdr_checksum = (uint16_t)rte_ipv4_cksum_simple(ip_hdr);
 
 		frag->data_len = sizeof(struct rte_ether_hdr) + pkt_len;
 		frag->pkt_len = frag->data_len;
diff --git a/lib/net/rte_ip4.h b/lib/net/rte_ip4.h
index d9840b3cff..3dc0c5a017 100644
--- a/lib/net/rte_ip4.h
+++ b/lib/net/rte_ip4.h
@@ -163,6 +163,38 @@ rte_ipv4_cksum(const struct rte_ipv4_hdr *ipv4_hdr)
 	return (uint16_t)~cksum;
 }
 
+/**
+ * Process the IPv4 checksum of an IPv4 header without any extensions.
+ *
+ * The checksum field does NOT have to be set by the caller, the field
+ * is skipped by the calculation.
+ *
+ * @param ipv4_hdr
+ *   The pointer to the contiguous IPv4 header.
+ * @return
+ *   The complemented checksum to set in the IP packet.
+ */
+__rte_experimental
+static inline uint16_t
+rte_ipv4_cksum_simple(const struct rte_ipv4_hdr *ipv4_hdr)
+{
+	const uint16_t *v16_h;
+	uint32_t ip_cksum;
+
+	/*
+	 * Compute the sum of successive 16-bit words of the IPv4 header,
+	 * skipping the checksum field of the header.
+	 */
+	v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
+	ip_cksum = v16_h[0] + v16_h[1] + v16_h[2] + v16_h[3] +
+		v16_h[4] + v16_h[6] + v16_h[7] + v16_h[8] + v16_h[9];
+
+	/* reduce 32 bit checksum to 16 bits and complement it */
+	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
+	ip_cksum = (ip_cksum & 0xffff) + (ip_cksum >> 16);
+	return (uint16_t)(~ip_cksum);
+}
+
 /**
  * Process the pseudo-header checksum of an IPv4 header.
  *
-- 
2.43.0

Re: [PATCH v2 5/6] net: add smaller IPv4 cksum function for simple cases

[David Marchand]

Hello Bruce,

On Fri, Oct 25, 2024 at 6:51 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> There are multiple instances in the DPDK app folder where we set up an
> IP header and then compute the checksum field by direct addition of
> nine uint16_t values in the header (20 bytes less the cksum field).
> The existing rte_ip.h checksum function is more general than necessary
> here and requires that the checksum field is already set to zero -
> rather than having it skipped.
>
> Fix the code duplication present in the apps by creating a new
> rte_ipv4_cksum_simple function - taking the code from the existing
> testpmd icmpecho.c file - and using that in app/test, testpmd and
> testeventdev.
>
> Within that new function, we can adjust slightly how the typecasting to
> uint16_t is done, and thereby ensure that the app can all be compiled
> without -Wno-address-of-packed-member compiler flag.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

This added function triggers a build error with OVS because of -Wcast-align:
https://github.com/david-marchand/ovs/actions/runs/11401635820/job/32273090691#step:12:514

libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I
./include/sparse -I ./include -m64 -I /usr/local/include -I
/usr/include/x86_64-linux-gnu " cgcc -target=x86_64
-target=host_os_specs -D__MMX__=1 -D__MMX_WITH_SSE__=1
-D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE__=1 -D__SSE2__=1
-DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
-Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
-Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
-Wbad-function-cast -Wcast-align -Wstrict-prototypes
-Wold-style-definition -Wmissing-prototypes
-Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
-Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
-Wshift-negative-value -Wduplicated-cond -Wshadow
-Wmultistatement-macros -Wcast-align=strict -mssse3
-I/home/runner/work/ovs/ovs/dpdk-dir/include -include rte_config.h
-mrtm -Werror -D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/ofp-protocol.lo
-MD -MP -MF lib/.deps/ofp-protocol.Tpo -c lib/ofp-protocol.c -o
lib/ofp-protocol.o
In file included from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip.h:9,
from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_flow.h:25,
from lib/netdev-dpdk.h:30,
from lib/dp-packet.h:30,
from lib/ofp-print.c:34:
/home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h: In function
‘rte_ipv4_cksum_simple’:
/home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:17: error:
cast increases required alignment of target type [-Werror=cast-align]
191 | v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
| ^
cc1: all warnings being treated as errors
make[1]: *** [Makefile:4736: lib/ofp-packet.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
cc1: all warnings being treated as errors
make[1]: *** [Makefile:4736: lib/ofp-print.lo] Error 1
make[1]: Leaving directory '/home/runner/work/ovs/ovs'
make: *** [Makefile:3110: all] Error 2



-- 
David Marchand

Re: [PATCH v2 5/6] net: add smaller IPv4 cksum function for simple cases

[Bruce Richardson]

On Wed, Oct 30, 2024 at 12:21:00PM +0100, David Marchand wrote:
> Hello Bruce,
> 
> On Fri, Oct 25, 2024 at 6:51 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > There are multiple instances in the DPDK app folder where we set up an
> > IP header and then compute the checksum field by direct addition of
> > nine uint16_t values in the header (20 bytes less the cksum field).
> > The existing rte_ip.h checksum function is more general than necessary
> > here and requires that the checksum field is already set to zero -
> > rather than having it skipped.
> >
> > Fix the code duplication present in the apps by creating a new
> > rte_ipv4_cksum_simple function - taking the code from the existing
> > testpmd icmpecho.c file - and using that in app/test, testpmd and
> > testeventdev.
> >
> > Within that new function, we can adjust slightly how the typecasting to
> > uint16_t is done, and thereby ensure that the app can all be compiled
> > without -Wno-address-of-packed-member compiler flag.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 
> This added function triggers a build error with OVS because of -Wcast-align:
> https://github.com/david-marchand/ovs/actions/runs/11401635820/job/32273090691#step:12:514
> 
> libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I
> ./include/sparse -I ./include -m64 -I /usr/local/include -I
> /usr/include/x86_64-linux-gnu " cgcc -target=x86_64
> -target=host_os_specs -D__MMX__=1 -D__MMX_WITH_SSE__=1
> -D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE__=1 -D__SSE2__=1
> -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
> -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
> -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
> -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> -Wold-style-definition -Wmissing-prototypes
> -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
> -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
> -Wshift-negative-value -Wduplicated-cond -Wshadow
> -Wmultistatement-macros -Wcast-align=strict -mssse3
> -I/home/runner/work/ovs/ovs/dpdk-dir/include -include rte_config.h
> -mrtm -Werror -D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/ofp-protocol.lo
> -MD -MP -MF lib/.deps/ofp-protocol.Tpo -c lib/ofp-protocol.c -o
> lib/ofp-protocol.o
> In file included from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip.h:9,
> from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_flow.h:25,
> from lib/netdev-dpdk.h:30,
> from lib/dp-packet.h:30,
> from lib/ofp-print.c:34:
> /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h: In function
> ‘rte_ipv4_cksum_simple’:
> /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:17: error:
> cast increases required alignment of target type [-Werror=cast-align]
> 191 | v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> | ^
Ok, I'll see if I can rework it to avoid issues.

/Bruce

RE: [PATCH v2 5/6] net: add smaller IPv4 cksum function for simple cases

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 30 October 2024 12.28
> 
> On Wed, Oct 30, 2024 at 12:21:00PM +0100, David Marchand wrote:
> > Hello Bruce,
> >
> > On Fri, Oct 25, 2024 at 6:51 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > There are multiple instances in the DPDK app folder where we set up
> an
> > > IP header and then compute the checksum field by direct addition of
> > > nine uint16_t values in the header (20 bytes less the cksum field).
> > > The existing rte_ip.h checksum function is more general than
> necessary
> > > here and requires that the checksum field is already set to zero -
> > > rather than having it skipped.
> > >
> > > Fix the code duplication present in the apps by creating a new
> > > rte_ipv4_cksum_simple function - taking the code from the existing
> > > testpmd icmpecho.c file - and using that in app/test, testpmd and
> > > testeventdev.
> > >
> > > Within that new function, we can adjust slightly how the
> typecasting to
> > > uint16_t is done, and thereby ensure that the app can all be
> compiled
> > > without -Wno-address-of-packed-member compiler flag.
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >
> > This added function triggers a build error with OVS because of -
> Wcast-align:
> > https://github.com/david-
> marchand/ovs/actions/runs/11401635820/job/32273090691#step:12:514
> >
> > libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I
> > ./include/sparse -I ./include -m64 -I /usr/local/include -I
> > /usr/include/x86_64-linux-gnu " cgcc -target=x86_64
> > -target=host_os_specs -D__MMX__=1 -D__MMX_WITH_SSE__=1
> > -D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE__=1 -D__SSE2__=1
> > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
> > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
> > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
> > -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> > -Wold-style-definition -Wmissing-prototypes
> > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
> > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
> > -Wshift-negative-value -Wduplicated-cond -Wshadow
> > -Wmultistatement-macros -Wcast-align=strict -mssse3
> > -I/home/runner/work/ovs/ovs/dpdk-dir/include -include rte_config.h
> > -mrtm -Werror -D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/ofp-protocol.lo
> > -MD -MP -MF lib/.deps/ofp-protocol.Tpo -c lib/ofp-protocol.c -o
> > lib/ofp-protocol.o
> > In file included from /home/runner/work/ovs/ovs/dpdk-
> dir/include/rte_ip.h:9,
> > from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_flow.h:25,
> > from lib/netdev-dpdk.h:30,
> > from lib/dp-packet.h:30,
> > from lib/ofp-print.c:34:
> > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h: In function
> > ‘rte_ipv4_cksum_simple’:
> > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:17: error:
> > cast increases required alignment of target type [-Werror=cast-align]
> > 191 | v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > | ^
> Ok, I'll see if I can rework it to avoid issues.

Would be easier if IP(v4/v6) headers were 2-byte aligned, like the Ethernet header.
Just saying. ;-)

Re: [PATCH v2 5/6] net: add smaller IPv4 cksum function for simple cases

[Bruce Richardson]

On Wed, Oct 30, 2024 at 12:32:30PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, 30 October 2024 12.28
> > 
> > On Wed, Oct 30, 2024 at 12:21:00PM +0100, David Marchand wrote:
> > > Hello Bruce,
> > >
> > > On Fri, Oct 25, 2024 at 6:51 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > There are multiple instances in the DPDK app folder where we set up
> > an
> > > > IP header and then compute the checksum field by direct addition of
> > > > nine uint16_t values in the header (20 bytes less the cksum field).
> > > > The existing rte_ip.h checksum function is more general than
> > necessary
> > > > here and requires that the checksum field is already set to zero -
> > > > rather than having it skipped.
> > > >
> > > > Fix the code duplication present in the apps by creating a new
> > > > rte_ipv4_cksum_simple function - taking the code from the existing
> > > > testpmd icmpecho.c file - and using that in app/test, testpmd and
> > > > testeventdev.
> > > >
> > > > Within that new function, we can adjust slightly how the
> > typecasting to
> > > > uint16_t is done, and thereby ensure that the app can all be
> > compiled
> > > > without -Wno-address-of-packed-member compiler flag.
> > > >
> > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >
> > > This added function triggers a build error with OVS because of -
> > Wcast-align:
> > > https://github.com/david-
> > marchand/ovs/actions/runs/11401635820/job/32273090691#step:12:514
> > >
> > > libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I
> > > ./include/sparse -I ./include -m64 -I /usr/local/include -I
> > > /usr/include/x86_64-linux-gnu " cgcc -target=x86_64
> > > -target=host_os_specs -D__MMX__=1 -D__MMX_WITH_SSE__=1
> > > -D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE__=1 -D__SSE2__=1
> > > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
> > > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith
> > > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
> > > -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> > > -Wold-style-definition -Wmissing-prototypes
> > > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
> > > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
> > > -Wshift-negative-value -Wduplicated-cond -Wshadow
> > > -Wmultistatement-macros -Wcast-align=strict -mssse3
> > > -I/home/runner/work/ovs/ovs/dpdk-dir/include -include rte_config.h
> > > -mrtm -Werror -D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/ofp-protocol.lo
> > > -MD -MP -MF lib/.deps/ofp-protocol.Tpo -c lib/ofp-protocol.c -o
> > > lib/ofp-protocol.o
> > > In file included from /home/runner/work/ovs/ovs/dpdk-
> > dir/include/rte_ip.h:9,
> > > from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_flow.h:25,
> > > from lib/netdev-dpdk.h:30,
> > > from lib/dp-packet.h:30,
> > > from lib/ofp-print.c:34:
> > > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h: In function
> > > ‘rte_ipv4_cksum_simple’:
> > > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:17: error:
> > > cast increases required alignment of target type [-Werror=cast-align]
> > > 191 | v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > > | ^
> > Ok, I'll see if I can rework it to avoid issues.
> 
> Would be easier if IP(v4/v6) headers were 2-byte aligned, like the Ethernet header.
> Just saying. ;-)
>
I agree. Would adding that alignment constraint cause us any issues, does
anyone know?

/Bruce 

RE: [PATCH v2 5/6] net: add smaller IPv4 cksum function for simple cases

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 30 October 2024 13.28
> 
> On Wed, Oct 30, 2024 at 12:32:30PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Wednesday, 30 October 2024 12.28
> > >
> > > On Wed, Oct 30, 2024 at 12:21:00PM +0100, David Marchand wrote:
> > > > Hello Bruce,
> > > >
> > > > On Fri, Oct 25, 2024 at 6:51 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > >
> > > > > There are multiple instances in the DPDK app folder where we
> set up
> > > an
> > > > > IP header and then compute the checksum field by direct
> addition of
> > > > > nine uint16_t values in the header (20 bytes less the cksum
> field).
> > > > > The existing rte_ip.h checksum function is more general than
> > > necessary
> > > > > here and requires that the checksum field is already set to
> zero -
> > > > > rather than having it skipped.
> > > > >
> > > > > Fix the code duplication present in the apps by creating a new
> > > > > rte_ipv4_cksum_simple function - taking the code from the
> existing
> > > > > testpmd icmpecho.c file - and using that in app/test, testpmd
> and
> > > > > testeventdev.
> > > > >
> > > > > Within that new function, we can adjust slightly how the
> > > typecasting to
> > > > > uint16_t is done, and thereby ensure that the app can all be
> > > compiled
> > > > > without -Wno-address-of-packed-member compiler flag.
> > > > >
> > > > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > > Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > >
> > > > This added function triggers a build error with OVS because of -
> > > Wcast-align:
> > > > https://github.com/david-
> > > marchand/ovs/actions/runs/11401635820/job/32273090691#step:12:514
> > > >
> > > > libtool: compile: env REAL_CC=gcc "CHECK=sparse -Wsparse-error -I
> > > > ./include/sparse -I ./include -m64 -I /usr/local/include -I
> > > > /usr/include/x86_64-linux-gnu " cgcc -target=x86_64
> > > > -target=host_os_specs -D__MMX__=1 -D__MMX_WITH_SSE__=1
> > > > -D__SSE2_MATH__=1 -D__SSE_MATH__=1 -D__SSE__=1 -D__SSE2__=1
> > > > -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib
> > > > -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-
> arith
> > > > -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter
> > > > -Wbad-function-cast -Wcast-align -Wstrict-prototypes
> > > > -Wold-style-definition -Wmissing-prototypes
> > > > -Wmissing-field-initializers -fno-strict-aliasing -Wswitch-bool
> > > > -Wlogical-not-parentheses -Wsizeof-array-argument -Wbool-compare
> > > > -Wshift-negative-value -Wduplicated-cond -Wshadow
> > > > -Wmultistatement-macros -Wcast-align=strict -mssse3
> > > > -I/home/runner/work/ovs/ovs/dpdk-dir/include -include
> rte_config.h
> > > > -mrtm -Werror -D_FILE_OFFSET_BITS=64 -g -O2 -MT lib/ofp-
> protocol.lo
> > > > -MD -MP -MF lib/.deps/ofp-protocol.Tpo -c lib/ofp-protocol.c -o
> > > > lib/ofp-protocol.o
> > > > In file included from /home/runner/work/ovs/ovs/dpdk-
> > > dir/include/rte_ip.h:9,
> > > > from /home/runner/work/ovs/ovs/dpdk-dir/include/rte_flow.h:25,
> > > > from lib/netdev-dpdk.h:30,
> > > > from lib/dp-packet.h:30,
> > > > from lib/ofp-print.c:34:
> > > > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h: In function
> > > > ‘rte_ipv4_cksum_simple’:
> > > > /home/runner/work/ovs/ovs/dpdk-dir/include/rte_ip4.h:191:17:
> error:
> > > > cast increases required alignment of target type [-Werror=cast-
> align]
> > > > 191 | v16_h = (const unaligned_uint16_t *)&ipv4_hdr->version_ihl;
> > > > | ^
> > > Ok, I'll see if I can rework it to avoid issues.
> >
> > Would be easier if IP(v4/v6) headers were 2-byte aligned, like the
> Ethernet header.
> > Just saying. ;-)
> >
> I agree. Would adding that alignment constraint cause us any issues,
> does
> anyone know?

No issues according to the CI:
https://patchwork.dpdk.org/project/dpdk/patch/20241011160653.88028-1-mb@smartsharesystems.com/

The single failure is not related to the patch.

[PATCH v2 6/6] build: limit scope of packed member warning disabling

[Bruce Richardson]

The flag '-Wno-address-of-packed-member' is a global warning flag in
DPDK. Rather than disabling this warning globally, it is better to just
have it enabled for components that may need it. This patch limits the
scope of it in the following ways:

* limit the use of the flag to the drivers subfolder only - all libs and
  apps should be buildable without the warning.
* exception is made for the vhost library and the ipsec-secgw example
  for now, as making them buildable with the warning enabled is more
  complicated. This exception can hopefully be removed in future.

Limiting the scope further within the drivers directory is also left for
future consideration. However, since HW drivers often have to use packed
structures to align with hardware-implemented data layouts, it may be
more practical to keep the warning disabled in the longer term.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 config/meson.build               | 1 -
 drivers/meson.build              | 9 ++++++---
 examples/ipsec-secgw/meson.build | 6 ++++++
 lib/vhost/meson.build            | 5 ++++-
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/config/meson.build b/config/meson.build
index 8dae811378..5095d2fbcb 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -325,7 +325,6 @@ warning_flags = [
         '-Wwrite-strings',
 
         # globally disabled warnings
-        '-Wno-address-of-packed-member',
         '-Wno-packed-not-aligned',
         '-Wno-missing-field-initializers',
 ]
diff --git a/drivers/meson.build b/drivers/meson.build
index 5270160c56..4a27f197a8 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -59,9 +59,12 @@ default_cflags = machine_args
 default_cflags += ['-DALLOW_EXPERIMENTAL_API']
 default_cflags += ['-DALLOW_INTERNAL_API']
 
-if cc.has_argument('-Wno-format-truncation')
-    default_cflags += '-Wno-format-truncation'
-endif
+warning_disable_cflags = ['-Wno-format-truncation', '-Wno-address-of-packed-member']
+foreach cflag:warning_disable_cflags
+    if cc.has_argument(cflag)
+        default_cflags += cflag
+    endif
+endforeach
 
 dpdk_drivers_build_dir = meson.current_build_dir()
 
diff --git a/examples/ipsec-secgw/meson.build b/examples/ipsec-secgw/meson.build
index ccdaef1c4d..023d9cf039 100644
--- a/examples/ipsec-secgw/meson.build
+++ b/examples/ipsec-secgw/meson.build
@@ -23,3 +23,9 @@ sources = files(
         'sp4.c',
         'sp6.c',
 )
+app_cflags = ['-Wno-address-of-packed-member']
+foreach flag:app_cflags
+    if cc.has_argument(flag)
+        cflags += flag
+    endif
+endforeach
diff --git a/lib/vhost/meson.build b/lib/vhost/meson.build
index 41b622a9be..51bcf17244 100644
--- a/lib/vhost/meson.build
+++ b/lib/vhost/meson.build
@@ -16,7 +16,10 @@ elif (toolchain == 'icc' and cc.version().version_compare('>=16.0.0'))
     cflags += '-DVHOST_ICC_UNROLL_PRAGMA'
 endif
 dpdk_conf.set('RTE_LIBRTE_VHOST_POSTCOPY', cc.has_header('linux/userfaultfd.h'))
-cflags += '-fno-strict-aliasing'
+cflags += [
+        '-fno-strict-aliasing',
+        '-Wno-address-of-packed-member',
+]
 
 sources = files(
         'fd_man.c',
-- 
2.43.0

Re: [PATCH v2 0/6] Reduce scope address-of-packed-member warning

[fengchengwen]

LGTM
Series-acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2024/10/26 0:50, Bruce Richardson wrote:
> The warning for address-of-packed-member was being disabled globally in
> DPDK.  While for drivers which need to access hardware-defined
> data-structures the use of packed may make sense, for normal libs and
> applications the use of packed data should be generally avoided.
> 
> This patchset initially applies some fixes for cases where we are
> unnecessarily causing the warning to trigger. Thereafter the 6th patch
> removes the global enabling of the warning and replaces it with more
> selective disabling for drivers and for a couple of other components
> which have not yet been fixed.
> 
> v2:
> * rebase on latest main (rte_ip.h -> rte_ip4.h)
> * remove unnecessary 0-check in IP checksum calc
> 
> Bruce Richardson (6):
>   ip_frag: remove use of unaligned variable
>   efd: remove unnecessary packed attributes
>   bus/ifpga: remove packed attribute
>   pipeline: remove packed attribute
>   net: add smaller IPv4 cksum function for simple cases
>   build: limit scope of packed member warning disabling
> 
>  app/test-eventdev/test_pipeline_common.c | 25 +-----------
>  app/test-pmd/icmpecho.c                  | 23 +----------
>  app/test-pmd/txonly.c                    | 22 +----------
>  app/test/packet_burst_generator.c        | 49 +-----------------------
>  app/test/test_reassembly_perf.c          | 29 +-------------
>  config/meson.build                       |  1 -
>  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
>  drivers/meson.build                      |  9 +++--
>  examples/ipsec-secgw/meson.build         |  6 +++
>  lib/efd/rte_efd.c                        |  4 +-
>  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
>  lib/net/rte_ip4.h                        | 32 ++++++++++++++++
>  lib/pipeline/rte_table_action.c          |  2 +-
>  lib/vhost/meson.build                    |  5 ++-
>  14 files changed, 59 insertions(+), 154 deletions(-)
> 
> --
> 2.43.0
> 

Re: [PATCH v2 0/6] Reduce scope address-of-packed-member warning

[David Marchand]

On Fri, Oct 25, 2024 at 6:51 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> The warning for address-of-packed-member was being disabled globally in
> DPDK.  While for drivers which need to access hardware-defined
> data-structures the use of packed may make sense, for normal libs and
> applications the use of packed data should be generally avoided.
>
> This patchset initially applies some fixes for cases where we are
> unnecessarily causing the warning to trigger. Thereafter the 6th patch
> removes the global enabling of the warning and replaces it with more
> selective disabling for drivers and for a couple of other components
> which have not yet been fixed.
>
> v2:
> * rebase on latest main (rte_ip.h -> rte_ip4.h)
> * remove unnecessary 0-check in IP checksum calc
>
> Bruce Richardson (6):
>   ip_frag: remove use of unaligned variable
>   efd: remove unnecessary packed attributes
>   bus/ifpga: remove packed attribute
>   pipeline: remove packed attribute
>   net: add smaller IPv4 cksum function for simple cases
>   build: limit scope of packed member warning disabling
>
>  app/test-eventdev/test_pipeline_common.c | 25 +-----------
>  app/test-pmd/icmpecho.c                  | 23 +----------
>  app/test-pmd/txonly.c                    | 22 +----------
>  app/test/packet_burst_generator.c        | 49 +-----------------------
>  app/test/test_reassembly_perf.c          | 29 +-------------
>  config/meson.build                       |  1 -
>  drivers/bus/ifpga/bus_ifpga_driver.h     |  2 +-
>  drivers/meson.build                      |  9 +++--
>  examples/ipsec-secgw/meson.build         |  6 +++
>  lib/efd/rte_efd.c                        |  4 +-
>  lib/ip_frag/rte_ipv4_reassembly.c        |  4 +-
>  lib/net/rte_ip4.h                        | 32 ++++++++++++++++
>  lib/pipeline/rte_table_action.c          |  2 +-
>  lib/vhost/meson.build                    |  5 ++-
>  14 files changed, 59 insertions(+), 154 deletions(-)

Series applied, thanks Bruce.


-- 
David Marchand