[v6,01/23] mbuf: add accessors for rearm and Rx descriptor fields
Checks
Commit Message
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(-)
Comments
> 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.
> +
> +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.
>
> 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.
> + * 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.
> + * referencing through the the anonymous union fields.
> */
> union {
> uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */
> --
> 1.8.3.1
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
+To: Thomas, previously joined the discussion
> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Tuesday, 27 February 2024 18.17
>
> 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.
No, I meant the return type.
The type is opaque; only its size is fixed at 8 byte.
Maybe uint64_t * is the best type, after all.
> arguably it could go
> void * first but we're already aliasing through a different type. this
> is one argument in favor of the union.
The zero-size markers (rearm_data and rx_descriptor_fields1) were intended to be used like unions, which the drivers effectively do.
Previous C standards didn't allow anonymous structures, so using union+struct in pre-C11 DPDK would clutter the code with subfield names. But now that we moved on to C11, we don't have this problem anymore.
IMHO, replacing the zero-size markers - which are directly visible in the structure's source code - with access functions located in some other header file degrades source code readability.
I am in favor of union+struct in the mbuf structure over access functions. I think the explicit grouping of the fields improves the readability of the mbuf structure, also compared to the zero-size markers.
>
> >
> > > +
> > > +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.
As you mention below, keeping the current names is an advantage for familiarity.
Changing the name brings no real benefit, and although the current name is somewhat silly (perhaps anticipating a second group of rx descriptor fields), we should probably just keep it.
>
> >
> > >
> > > 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).
OK, you already thought about the use of the names in the documentation. I don't object to your conclusion.
>
> >
> > > + * 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
On Wed, Feb 28, 2024 at 09:28:22AM +0100, Morten Brørup wrote:
> +To: Thomas, previously joined the discussion
>
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Tuesday, 27 February 2024 18.17
> >
> > 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.
>
> No, I meant the return type.
> The type is opaque; only its size is fixed at 8 byte.
> Maybe uint64_t * is the best type, after all.
ah, in many places the drivers want the uint64_t * keeping it here allows
for some of the casts (and potential for mistakes) to be discarded. if i
recall in some places they do math on the returned pointer.
>
> > arguably it could go
> > void * first but we're already aliasing through a different type. this
> > is one argument in favor of the union.
>
> The zero-size markers (rearm_data and rx_descriptor_fields1) were intended to be used like unions, which the drivers effectively do.
> Previous C standards didn't allow anonymous structures, so using union+struct in pre-C11 DPDK would clutter the code with subfield names. But now that we moved on to C11, we don't have this problem anymore.
>
> IMHO, replacing the zero-size markers - which are directly visible in the structure's source code - with access functions located in some other header file degrades source code readability.
>
> I am in favor of union+struct in the mbuf structure over access functions. I think the explicit grouping of the fields improves the readability of the mbuf structure, also compared to the zero-size markers.
it's unfortunate the zero-size markers could be accessed slightly
differently than the anonymous unions necessitating code referencing
them to be changed.
Thomas and i discussed rearm_data and rx_descriptor_fields1 at length
and it was understood why i had to add new field names if using the union
approach there was an expressed preference for accessors.
one possibly important detail that came out of our conversation was that
rearm_data rx_descriptor_fields1 are (I think) supposed to be internal
and that being the case means updading the dpdk drivers means i updated
*all* consumers i.e. i can break the api and adapt in the same commit if
they are truly internal because no application should be accessing the
fields.
one approach i've enjoyed some success with is to present semi-opaque
structures where the internal fields are by convention discouraged in
their use but doing this requires duplicating parts of the whole to e.g.
give the drivers the view of the struct they need and is an increased
burden for maintainers. again this relies on the fields actually being
internal only.
both approaches have been put up in this series now, just looking for
folks to look at the two and push for one or the other.
thanks!
@@ -132,6 +132,19 @@
#endif
}
+static inline
+uint64_t *
+rte_mbuf_rearm_data(struct rte_mbuf *m)
+{
+ return (uint64_t *)&m->data_off;
+}
+
+static inline
+void *
+rte_mbuf_rx_descriptor_fields1(struct rte_mbuf *m)
+{
+ return &m->packet_type;
+}
static inline uint16_t rte_pktmbuf_priv_size(struct rte_mempool *mp);
@@ -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()
+ * 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
+ * referencing through the the anonymous union fields.
*/
union {
uint32_t packet_type; /**< L2/L3/L4 and tunnel information. */