On Tue, Feb 27, 2024 at 10:10:03AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roret...@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 <roret...@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
