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

[Tyler Retzlaff]

On Wed, Apr 03, 2024 at 09:32:21PM +0200, Morten Brørup wrote:
> > 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.

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 <mb at smartsharesystems.com> to the next version.

ack, next rev.