On Wed, Apr 03, 2024 at 09:32:21PM +0200, Morten Brørup wrote: > > 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.
ooh, i'll fix it i'm not well versed in doxygen documentation. > > 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. it's easy enough for me to fix the comments in place and bring in a new version of the series, assuming other reviewers don't object i'll do that. the diff is already kind of annoying to review in mail without -b anyway. > > [...] > > > /* 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. ack, next rev.
