Re: [PATCH v6 01/23] mbuf: add accessors for rearm and Rx descriptor fields

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Tue, Feb 27, 2024 at 10:10:03AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Tuesday, 27 February 2024 06.41
> > 
> > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. Provide
> > inline functions to access compatible type pointer to rearm_data
> > and rx_descriptor_fields1 which will allow direct references on the
> > rte marker fields to be removed.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  lib/mbuf/rte_mbuf.h      | 13 +++++++++++++
> >  lib/mbuf/rte_mbuf_core.h | 11 ++++++++++-
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> > index 286b32b..aa7495b 100644
> > --- a/lib/mbuf/rte_mbuf.h
> > +++ b/lib/mbuf/rte_mbuf.h
> > @@ -132,6 +132,19 @@
> >  #endif
> >  }
> > 
> > +static inline
> > +uint64_t *
> > +rte_mbuf_rearm_data(struct rte_mbuf *m)
> > +{
> > +	return (uint64_t *)&m->data_off;
> > +}
> 
> Consider returning (void *) instead of (uint64_t *).
> Just a suggestion; I don't know which is better.

if you mean just the cast i don't think it matters. arguably it could go
void * first but we're already aliasing through a different type. this
is one argument in favor of the union.

> 
> > +
> > +static inline
> > +void *
> > +rte_mbuf_rx_descriptor_fields1(struct rte_mbuf *m)
> > +{
> > +	return &m->packet_type;
> > +}
> 
> The mbuf_rx_descriptor_fields1 field is disappearing in a later patch;
> consider taking the opportunity to use a better name here.

oh boy. you're asking for a new name but not suggesting a new name.
naming is hard (no one is ever happy). for this and rearm_data if you
or anyone suggests a name i'll make the change ~everywhere including
comments across all drivers.

> 
> > 
> >  static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool *mp);
> > 
> > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > index 5688683..7000c04 100644
> > --- a/lib/mbuf/rte_mbuf_core.h
> > +++ b/lib/mbuf/rte_mbuf_core.h
> > @@ -486,7 +486,12 @@ struct rte_mbuf {
> >  	struct rte_mbuf *next;
> >  #endif
> > 
> > -	/* next 8 bytes are initialised on RX descriptor rearm */
> > +	/**
> > +	 * next 8 bytes are initialised on RX descriptor rearm
> > +	 *
> > +	 * To obtain a pointer to rearm_data use the rte_mbuf_rearm_data()
> 
> The "rearm_data" field is disappearing in a later patch;
> don't refer to it by that name in the comments here.

without having renamed the block of fields cheerfully known as
"rearm_data" i kept all documentation/comments here and in drivers
using rearm_data for familiarity. (unless someone suggests a new name for the group
of fields).

> 
> > +	 * accessor instead of directly referencing through the data_off field.
> > +	 */
> >  	RTE_MARKER64 rearm_data;
> >  	uint16_t data_off;
> > 
> > @@ -522,6 +527,10 @@ struct rte_mbuf {
> >  	 * 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.
> > +	 *
> > +	 * To obtain a pointer to rx_descriptor_fields1 use the
> > +	 * rte_mbuf_rx_descriptor_fields1() accessor instead of directly
> 
> The "rx_descriptor_fields1" field is disappearing in a later patch;
> don't refer to it by that name in the comments here.
> And if you update the access function's name, remember to update it in the comment here too.

same as above.

> 
> > +	 * referencing through the the anonymous union fields.
> >  	 */
> >  	union {
> >  		uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */
> > --
> > 1.8.3.1