From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on
	aws-us-west-2-korg-lkml-1.web.codeaurora.org
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by smtp.lore.kernel.org (Postfix) with ESMTP id 107D3C47258
	for <dpdk-dev@archiver.kernel.org>; Wed, 31 Jan 2024 09:18:45 +0000 (UTC)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id DC880402C5;
	Wed, 31 Jan 2024 10:18:43 +0100 (CET)
Received: from dkmailrelay1.smartsharesystems.com
 (smartserver.smartsharesystems.com [77.243.40.215])
 by mails.dpdk.org (Postfix) with ESMTP id 39863402A1
 for <dev@dpdk.org>; Wed, 31 Jan 2024 10:18:43 +0100 (CET)
Received: from smartserver.smartsharesystems.com
 (smartserver.smartsharesys.local [192.168.4.10])
 by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 02C3F2079B;
 Wed, 31 Jan 2024 10:18:41 +0100 (CET)
Content-class: urn:content-classes:message
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Subject: RE: [PATCH] mbuf: replace GCC marker extension with C11 anonymous
 unions
X-MimeOLE: Produced By Microsoft Exchange V6.5
Date: Wed, 31 Jan 2024 10:18:37 +0100
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F1D0@smartserver.smartshare.dk>
In-Reply-To: <1706657173-26166-2-git-send-email-roretzla@linux.microsoft.com>
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
Thread-Topic: [PATCH] mbuf: replace GCC marker extension with C11 anonymous
 unions
Thread-Index: AdpT073EgGhvNJioTLeaqOhbE7oizwATZq9Q
References: <1706657173-26166-1-git-send-email-roretzla@linux.microsoft.com>
 <1706657173-26166-2-git-send-email-roretzla@linux.microsoft.com>
From: =?iso-8859-1?Q?Morten_Br=F8rup?= <mb@smartsharesystems.com>
To: "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
	<dev@dpdk.org>
Cc: "Andrew Boyer" <andrew.boyer@amd.com>,
 "Andrew Rybchenko" <andrew.rybchenko@oktetlabs.ru>,
 "Bruce Richardson" <bruce.richardson@intel.com>,
 "Chenbo Xia" <chenbox@nvidia.com>,
 "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>,
 "Maxime Coquelin" <maxime.coquelin@redhat.com>
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Wednesday, 31 January 2024 00.26
>=20
> Replace the use of RTE_MARKER<x> with C11 anonymous unions to improve
> code portability between toolchains.
>=20
> Update use of rte_mbuf rearm_data field in net/ionic, net/sfc and
> net/virtio which were accessing field as a zero-length array.
>=20
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---

I have some comments, putting weight on code readability rather than =
avoiding API breakage.

We can consider my suggested API breaking changes for the next API =
breaking release, and keep your goal of minimal API breakage with the =
current changes.

> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index 5688683..d731ea0 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -464,9 +464,10 @@ enum {
>   * The generic rte_mbuf, containing a packet mbuf.
>   */
>  struct rte_mbuf {
> -	RTE_MARKER cacheline0;
> -
> -	void *buf_addr;           /**< Virtual address of segment buffer.
> */
> +	union {
> +	    void *cacheline0;
> +	    void *buf_addr;           /**< Virtual address of segment
> buffer. */
> +	};

I suppose this is the least ugly workaround for not being able to use =
the RTE_MARKER hack here.

>  #if RTE_IOVA_IN_MBUF
>  	/**
>  	 * Physical address of segment buffer.
> @@ -487,69 +488,77 @@ struct rte_mbuf {
>  #endif
>=20
>  	/* next 8 bytes are initialised on RX descriptor rearm */
> -	RTE_MARKER64 rearm_data;
> -	uint16_t data_off;
> -
> -	/**
> -	 * Reference counter. Its size should at least equal to the size
> -	 * of port field (16 bits), to support zero-copy broadcast.
> -	 * It should only be accessed using the following functions:
> -	 * rte_mbuf_refcnt_update(), rte_mbuf_refcnt_read(), and
> -	 * rte_mbuf_refcnt_set(). The functionality of these functions
> (atomic,
> -	 * or non-atomic) is controlled by the RTE_MBUF_REFCNT_ATOMIC
> flag.
> -	 */
> -	RTE_ATOMIC(uint16_t) refcnt;
> +	union {
> +		uint64_t rearm_data;

I consider this union with uint64_t rearm_data an improvement for code =
readability. Using a marker here was weird.

> +		struct {
> +			uint16_t data_off;
> +
> +			/**
> +			 * Reference counter. Its size should at least equal
> to the size
> +			 * of port field (16 bits), to support zero-copy
> broadcast.
> +			 * It should only be accessed using the following
> functions:
> +			 * rte_mbuf_refcnt_update(), rte_mbuf_refcnt_read(),
> and
> +			 * rte_mbuf_refcnt_set(). The functionality of these
> functions (atomic,
> +			 * or non-atomic) is controlled by the
> RTE_MBUF_REFCNT_ATOMIC flag.
> +			 */
> +			RTE_ATOMIC(uint16_t) refcnt;
>=20
> -	/**
> -	 * Number of segments. Only valid for the first segment of an
> mbuf
> -	 * chain.
> -	 */
> -	uint16_t nb_segs;
> +			/**
> +			 * Number of segments. Only valid for the first
> segment of an mbuf
> +			 * chain.
> +			 */
> +			uint16_t nb_segs;
>=20
> -	/** Input port (16 bits to support more than 256 virtual ports).
> -	 * The event eth Tx adapter uses this field to specify the output
> port.
> -	 */
> -	uint16_t port;
> +			/** Input port (16 bits to support more than 256
> virtual ports).
> +			 * The event eth Tx adapter uses this field to
> specify the output port.
> +			 */
> +			uint16_t port;
>=20
> -	uint64_t ol_flags;        /**< Offload features. */
> +			uint64_t ol_flags;        /**< Offload features. */

Either:
1. If the comment about 8 bytes init on rearm is correct: ol_flags =
should remain outside the struct and union, i.e. at top level, else
2. It would be nice to increase the size of the rearm_data variable to =
16 byte, so it covers the entire struct being rearmed. (And the =
incorrect comment about how many bytes are being rearmed should be =
fixed.)

> +		};
> +	};
>=20
>  	/* remaining bytes are set on RX when pulling packet from
> descriptor */
> -	RTE_MARKER rx_descriptor_fields1;
> -
> -	/*
> -	 * The packet type, which is the combination of outer/inner L2,
> L3, L4
> -	 * and tunnel types. The packet_type is about data really present
> in the
> -	 * mbuf. Example: if vlan stripping is enabled, a received vlan
> packet
> -	 * would have RTE_PTYPE_L2_ETHER and not RTE_PTYPE_L2_VLAN
> because the
> -	 * vlan is stripped from the data.
> -	 */
>  	union {
> -		uint32_t packet_type; /**< L2/L3/L4 and tunnel information.
> */
> -		__extension__
> +		void *rx_descriptor_fields1;

Instead of using void* for rx_descriptor_fields1, it would be nice to =
make rx_descriptor_fields1 a type of the correct size. It might need to =
be an uint32_t array to avoid imposing additional alignment =
requirements.

> +
> +		/*
> +		 * The packet type, which is the combination of outer/inner
> L2, L3, L4
> +		 * and tunnel types. The packet_type is about data really
> present in the
> +		 * mbuf. Example: if vlan stripping is enabled, a received
> vlan packet
> +		 * would have RTE_PTYPE_L2_ETHER and not RTE_PTYPE_L2_VLAN
> because the
> +		 * vlan is stripped from the data.
> +		 */
>  		struct {
> -			uint8_t l2_type:4;   /**< (Outer) L2 type. */
> -			uint8_t l3_type:4;   /**< (Outer) L3 type. */
> -			uint8_t l4_type:4;   /**< (Outer) L4 type. */
> -			uint8_t tun_type:4;  /**< Tunnel type. */
>  			union {
> -				uint8_t inner_esp_next_proto;
> -				/**< ESP next protocol type, valid if
> -				 * RTE_PTYPE_TUNNEL_ESP tunnel type is set
> -				 * on both Tx and Rx.
> -				 */
> +				uint32_t packet_type; /**< L2/L3/L4 and tunnel
> information. */
>  				__extension__
>  				struct {
> -					uint8_t inner_l2_type:4;
> -					/**< Inner L2 type. */
> -					uint8_t inner_l3_type:4;
> -					/**< Inner L3 type. */
> +					uint8_t l2_type:4;   /**< (Outer) L2
> type. */
> +					uint8_t l3_type:4;   /**< (Outer) L3
> type. */
> +					uint8_t l4_type:4;   /**< (Outer) L4
> type. */
> +					uint8_t tun_type:4;  /**< Tunnel type. */
> +					union {
> +						uint8_t inner_esp_next_proto;
> +						/**< ESP next protocol type, valid
> if
> +						 * RTE_PTYPE_TUNNEL_ESP tunnel type
> is set
> +						 * on both Tx and Rx.
> +						 */
> +						__extension__
> +						struct {
> +							uint8_t inner_l2_type:4;
> +							/**< Inner L2 type. */
> +							uint8_t inner_l3_type:4;
> +							/**< Inner L3 type. */
> +						};
> +					};
> +					uint8_t inner_l4_type:4; /**< Inner L4
> type. */
>  				};
>  			};
> -			uint8_t inner_l4_type:4; /**< Inner L4 type. */
> +			uint32_t pkt_len;         /**< Total pkt len: sum of
> all segments. */
>  		};
>  	};
>=20
> -	uint32_t pkt_len;         /**< Total pkt len: sum of all
> segments. */
>  	uint16_t data_len;        /**< Amount of data in segment buffer.
> */
>  	/** VLAN TCI (CPU order), valid if RTE_MBUF_F_RX_VLAN is set. */
>  	uint16_t vlan_tci;
> @@ -595,21 +604,23 @@ struct rte_mbuf {
>  	struct rte_mempool *pool; /**< Pool from which mbuf was
> allocated. */
>=20
>  	/* second cache line - fields only used in slow path or on TX */
> -	RTE_MARKER cacheline1 __rte_cache_min_aligned;
> +	union {
> +		void *cacheline1;

The __rte_cache_min_aligned cannot be removed. It provides cache line =
alignment for 32 bit platforms, where pointers in the first cache line =
only use 4 byte.

NB: The rte_mbuf structure could be optimized for 32 bit platforms by =
moving fields from the second cache line to the holes in the first, but =
that's another discussion.

>=20
>  #if RTE_IOVA_IN_MBUF
> -	/**
> -	 * Next segment of scattered packet. Must be NULL in the last
> -	 * segment or in case of non-segmented packet.
> -	 */
> -	struct rte_mbuf *next;
> +		/**
> +		 * Next segment of scattered packet. Must be NULL in the
> last
> +		 * segment or in case of non-segmented packet.
> +		 */
> +		struct rte_mbuf *next;
>  #else
> -	/**
> -	 * Reserved for dynamic fields
> -	 * when the next pointer is in first cache line (i.e.
> RTE_IOVA_IN_MBUF is 0).
> -	 */
> -	uint64_t dynfield2;
> +		/**
> +		 * Reserved for dynamic fields
> +		 * when the next pointer is in first cache line (i.e.
> RTE_IOVA_IN_MBUF is 0).
> +		 */
> +		uint64_t dynfield2;
>  #endif
> +	};
>=20
>  	/* fields to support TX offloads */
>  	union {
> --
> 1.8.3.1