> From: Tyler Retzlaff [mailto:roret...@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 <roret...@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 <m...@smartsharesystems.com> to the next version.
