[PATCH v10 2/4] mbuf: remove rte marker fields

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla at linux.microsoft.com]
> Sent: Wednesday, 3 April 2024 19.54
> 
> RTE_MARKER typedefs are a GCC extension unsupported by MSVC. Remove
> RTE_MARKER fields from rte_mbuf struct.
> 
> Maintain alignment of fields after removed cacheline1 marker by placing
> C11 alignas(RTE_CACHE_LINE_MIN_SIZE).
> 
> Provide new rearm_data and rx_descriptor_fields1 fields in anonymous
> unions as single element arrays of with types matching the original
> markers to maintain API compatibility.
> 
> This change breaks the API for cacheline{0,1} fields that have been
> removed from rte_mbuf but it does not break the ABI, to address the
> false positives of the removed (but 0 size fields) provide the minimum
> libabigail.abignore for type = rte_mbuf.
> 
> Signed-off-by: Tyler Retzlaff <roretzla at linux.microsoft.com>
> ---

[...]

> +	/* remaining 24 bytes are set on RX when pulling packet from
> descriptor */

Good.

>  	union {
> +		/* void * type of the array elements is retained for driver
> compatibility. */
> +		void *rx_descriptor_fields1[24 / sizeof(void *)];

Good, also the description.

>  		__extension__
>  		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. */
> +			/*
> +			 * 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 {
> -				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.
> -				 */

[...]

> +						/**< ESP next protocol type, valid
> if
> +						 * RTE_PTYPE_TUNNEL_ESP tunnel type
> is set
> +						 * on both Tx and Rx.
> +						 */
> +						uint8_t inner_esp_next_proto;

Thank you for moving the comments up before the fields.

Please note that "/**<" means that the description is related to the field preceding the comment, so it should be replaced by "/**" when moving the description up above a field.

Maybe moving the descriptions as part of this patch was not a good idea after all; it doesn't improve the readability of the patch itself. I regret suggesting it.
If you leave the descriptions at their originals positions (relative to the fields), we can clean up the formatting of the descriptions in a later patch.

[...]

>  	/* second cache line - fields only used in slow path or on TX */
> -	alignas(RTE_CACHE_LINE_MIN_SIZE) RTE_MARKER cacheline1;
> -
>  #if RTE_IOVA_IN_MBUF
>  	/**
>  	 * Next segment of scattered packet. Must be NULL in the last
>  	 * segment or in case of non-segmented packet.
>  	 */
> +	alignas(RTE_CACHE_LINE_MIN_SIZE)
>  	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).
>  	 */
> +	alignas(RTE_CACHE_LINE_MIN_SIZE)

Good positioning of the alignas().

I like everything in this patch.

Please fix the descriptions preceding the fields "/**<" -> "/**" or move them back to their location after the fields; then you may add Reviewed-by: Morten Brørup <mb at smartsharesystems.com> to the next version.