DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v3] mbuf: minor cleanup
@ 2020-09-16 10:40 Morten Brørup
  2020-10-07  9:16 ` Olivier Matz
  0 siblings, 1 reply; 5+ messages in thread
From: Morten Brørup @ 2020-09-16 10:40 UTC (permalink / raw)
  To: olivier.matz, thomas; +Cc: dev, Morten Brørup

The mbuf header files had some commenting style errors that affected the
API documentation.
Also, the RTE_ prefix was missing on a macro and a definition.

Note: This patch does not touch the offload and attachment flags that are
also missing the RTE_ prefix.

Changes only affecting documentation:
* Removed the MBUF_INVALID_PORT definition from rte_mbuf.h; it is
  already defined in rte_mbuf_core.h.
  This removal also reestablished the description of the
  rte_pktmbuf_reset() function.
* Corrected the comment related to RTE_MBUF_MAX_NB_SEGS.
* Corrected the comment related to PKT_TX_QINQ_PKT.

Changes regarding missing RTE_ prefix:
* Converted the MBUF_RAW_ALLOC_CHECK() macro to an
  __rte_mbuf_raw_sanity_check() inline function.
  Added backwards compatible macro with the original name.
* Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID.
  Added backwards compatible definition with the original name.

v2:
* Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c.

v3:
* The functions/macros used in __rte_mbuf_raw_sanity_check() require
  RTE_ENABLE_ASSERT or RTE_LIBRTE_MBUF_DEBUG, or they don't use the mbuf
  parameter, which generates a compiler waning. So mark the mbuf parameter
  __rte_unused if none of them are defined.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 doc/guides/rel_notes/deprecation.rst |  7 ----
 lib/librte_mbuf/rte_mbuf.c           |  4 +-
 lib/librte_mbuf/rte_mbuf.h           | 55 +++++++++++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h      |  9 +++--
 4 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 279eccb04..88d7d0761 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -294,13 +294,6 @@ Deprecation Notices
   - https://patches.dpdk.org/patch/71457/
   - https://patches.dpdk.org/patch/71456/
 
-* rawdev: The rawdev APIs which take a device-specific structure as
-  parameter directly, or indirectly via a "private" pointer inside another
-  structure, will be modified to take an additional parameter of the
-  structure size. The affected APIs will include ``rte_rawdev_info_get``,
-  ``rte_rawdev_configure``, ``rte_rawdev_queue_conf_get`` and
-  ``rte_rawdev_queue_setup``.
-
 * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value will be removed.
   This enum value is not used inside DPDK, while it prevents to add new
   classify algorithms without causing an ABI breakage.
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 8a456e5e6..53a015311 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -104,7 +104,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
 	/* init some constant fields */
 	m->pool = mp;
 	m->nb_segs = 1;
-	m->port = MBUF_INVALID_PORT;
+	m->port = RTE_MBUF_PORT_INVALID;
 	rte_mbuf_refcnt_set(m, 1);
 	m->next = NULL;
 }
@@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp,
 	/* init some constant fields */
 	m->pool = mp;
 	m->nb_segs = 1;
-	m->port = MBUF_INVALID_PORT;
+	m->port = RTE_MBUF_PORT_INVALID;
 	m->ol_flags = EXT_ATTACHED_MBUF;
 	rte_mbuf_refcnt_set(m, 1);
 	m->next = NULL;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7259575a7..406d3abb2 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -554,12 +554,36 @@ __rte_experimental
 int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 		   const char **reason);
 
-#define MBUF_RAW_ALLOC_CHECK(m) do {				\
-	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
-	RTE_ASSERT((m)->next == NULL);				\
-	RTE_ASSERT((m)->nb_segs == 1);				\
-	__rte_mbuf_sanity_check(m, 0);				\
-} while (0)
+#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG)
+/**
+ * Sanity checks on a reinitialized mbuf.
+ *
+ * Check the consistency of the given reinitialized mbuf.
+ * The function will cause a panic if corruption is detected.
+ *
+ * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
+ * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
+ *
+ * @param m
+ *   The mbuf to be checked.
+ */
+static __rte_always_inline void
+__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m)
+{
+	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
+	RTE_ASSERT(m->next == NULL);
+	RTE_ASSERT(m->nb_segs == 1);
+	__rte_mbuf_sanity_check(m, 0);
+}
+#else
+static __rte_always_inline void
+__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m __rte_unused)
+{
+    /* Nothing here. */
+}
+#endif
+/** For backwards compatibility. */
+#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
 
 /**
  * Allocate an uninitialized mbuf from mempool *mp*.
@@ -586,7 +610,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
 
 	if (rte_mempool_get(mp, (void **)&m) < 0)
 		return NULL;
-	MBUF_RAW_ALLOC_CHECK(m);
+	__rte_mbuf_raw_sanity_check(m);
 	return m;
 }
 
@@ -609,10 +633,7 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
 {
 	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
 		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
-	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
-	RTE_ASSERT(m->next == NULL);
-	RTE_ASSERT(m->nb_segs == 1);
-	__rte_mbuf_sanity_check(m, 0);
+	__rte_mbuf_raw_sanity_check(m);
 	rte_mempool_put(m->pool, m);
 }
 
@@ -858,8 +879,6 @@ static inline void rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
  * @param m
  *   The packet mbuf to be reset.
  */
-#define MBUF_INVALID_PORT UINT16_MAX
-
 static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 {
 	m->next = NULL;
@@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 	m->vlan_tci = 0;
 	m->vlan_tci_outer = 0;
 	m->nb_segs = 1;
-	m->port = MBUF_INVALID_PORT;
+	m->port = RTE_MBUF_PORT_INVALID;
 
 	m->ol_flags &= EXT_ATTACHED_MBUF;
 	m->packet_type = 0;
@@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
 	switch (count % 4) {
 	case 0:
 		while (idx != count) {
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 3:
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 2:
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 1:
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 8cd7137ac..4ac5609e3 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -272,7 +272,7 @@ extern "C" {
  * mbuf 'vlan_tci' & 'vlan_tci_outer' must be valid when this flag is set.
  */
 #define PKT_TX_QINQ        (1ULL << 49)
-/* this old name is deprecated */
+/** This old name is deprecated. */
 #define PKT_TX_QINQ_PKT    PKT_TX_QINQ
 
 /**
@@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info {
 	};
 };
 
-/**< Maximum number of nb_segs allowed. */
+/** Maximum number of nb_segs allowed. */
 #define RTE_MBUF_MAX_NB_SEGS	UINT16_MAX
 
 /**
@@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info {
 #define RTE_MBUF_DIRECT(mb) \
 	(!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF)))
 
-#define MBUF_INVALID_PORT UINT16_MAX
+/** NULL value for the uint16_t port type. */
+#define RTE_MBUF_PORT_INVALID UINT16_MAX
+/** For backwards compatibility. */
+#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID
 
 /**
  * A macro that points to an offset into the data in the mbuf.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: minor cleanup
  2020-09-16 10:40 [dpdk-dev] [PATCH v3] mbuf: minor cleanup Morten Brørup
@ 2020-10-07  9:16 ` Olivier Matz
  2020-10-20 11:55   ` Thomas Monjalon
  0 siblings, 1 reply; 5+ messages in thread
From: Olivier Matz @ 2020-10-07  9:16 UTC (permalink / raw)
  To: Morten Brørup; +Cc: thomas, dev

Hi Morten,

Thanks for this cleanup. Please see some comments below.

On Wed, Sep 16, 2020 at 12:40:13PM +0200, Morten Brørup wrote:
> The mbuf header files had some commenting style errors that affected the
> API documentation.
> Also, the RTE_ prefix was missing on a macro and a definition.
> 
> Note: This patch does not touch the offload and attachment flags that are
> also missing the RTE_ prefix.
> 
> Changes only affecting documentation:
> * Removed the MBUF_INVALID_PORT definition from rte_mbuf.h; it is
>   already defined in rte_mbuf_core.h.
>   This removal also reestablished the description of the
>   rte_pktmbuf_reset() function.
> * Corrected the comment related to RTE_MBUF_MAX_NB_SEGS.
> * Corrected the comment related to PKT_TX_QINQ_PKT.
> 
> Changes regarding missing RTE_ prefix:
> * Converted the MBUF_RAW_ALLOC_CHECK() macro to an
>   __rte_mbuf_raw_sanity_check() inline function.
>   Added backwards compatible macro with the original name.
> * Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID.
>   Added backwards compatible definition with the original name.
> 
> v2:
> * Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c.
> 
> v3:
> * The functions/macros used in __rte_mbuf_raw_sanity_check() require
>   RTE_ENABLE_ASSERT or RTE_LIBRTE_MBUF_DEBUG, or they don't use the mbuf
>   parameter, which generates a compiler waning. So mark the mbuf parameter
>   __rte_unused if none of them are defined.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  doc/guides/rel_notes/deprecation.rst |  7 ----
>  lib/librte_mbuf/rte_mbuf.c           |  4 +-
>  lib/librte_mbuf/rte_mbuf.h           | 55 +++++++++++++++++++---------
>  lib/librte_mbuf/rte_mbuf_core.h      |  9 +++--
>  4 files changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 279eccb04..88d7d0761 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -294,13 +294,6 @@ Deprecation Notices
>    - https://patches.dpdk.org/patch/71457/
>    - https://patches.dpdk.org/patch/71456/
>  
> -* rawdev: The rawdev APIs which take a device-specific structure as
> -  parameter directly, or indirectly via a "private" pointer inside another
> -  structure, will be modified to take an additional parameter of the
> -  structure size. The affected APIs will include ``rte_rawdev_info_get``,
> -  ``rte_rawdev_configure``, ``rte_rawdev_queue_conf_get`` and
> -  ``rte_rawdev_queue_setup``.
> -
>  * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value will be removed.
>    This enum value is not used inside DPDK, while it prevents to add new
>    classify algorithms without causing an ABI breakage.

I think this change is not related.

This makes me think that a deprecation notice could be done for the
old names without the RTE_ prefix, to be removed in 21.11.


> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 8a456e5e6..53a015311 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -104,7 +104,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
>  	/* init some constant fields */
>  	m->pool = mp;
>  	m->nb_segs = 1;
> -	m->port = MBUF_INVALID_PORT;
> +	m->port = RTE_MBUF_PORT_INVALID;
>  	rte_mbuf_refcnt_set(m, 1);
>  	m->next = NULL;
>  }
> @@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp,
>  	/* init some constant fields */
>  	m->pool = mp;
>  	m->nb_segs = 1;
> -	m->port = MBUF_INVALID_PORT;
> +	m->port = RTE_MBUF_PORT_INVALID;
>  	m->ol_flags = EXT_ATTACHED_MBUF;
>  	rte_mbuf_refcnt_set(m, 1);
>  	m->next = NULL;
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 7259575a7..406d3abb2 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -554,12 +554,36 @@ __rte_experimental
>  int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
>  		   const char **reason);
>  
> -#define MBUF_RAW_ALLOC_CHECK(m) do {				\
> -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
> -	RTE_ASSERT((m)->next == NULL);				\
> -	RTE_ASSERT((m)->nb_segs == 1);				\
> -	__rte_mbuf_sanity_check(m, 0);				\
> -} while (0)
> +#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG)

I don't see why this #if is needed. Wouldn't it work to have only
one function definition with the __rte_unused attribute?

> +/**
> + * Sanity checks on a reinitialized mbuf.
> + *
> + * Check the consistency of the given reinitialized mbuf.
> + * The function will cause a panic if corruption is detected.
> + *
> + * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
> + * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
> + *

Maybe indicate that these checks are only done when debug is on.

> + * @param m
> + *   The mbuf to be checked.
> + */
> +static __rte_always_inline void
> +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m)
> +{
> +	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> +	RTE_ASSERT(m->next == NULL);
> +	RTE_ASSERT(m->nb_segs == 1);
> +	__rte_mbuf_sanity_check(m, 0);
> +}
> +#else
> +static __rte_always_inline void
> +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m __rte_unused)
> +{
> +    /* Nothing here. */
> +}
> +#endif
> +/** For backwards compatibility. */
> +#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)

It looks that MBUF_RAW_ALLOC_CHECK() is also used in drivers/net/sfc,
I think it should be updated too.

>  
>  /**
>   * Allocate an uninitialized mbuf from mempool *mp*.
> @@ -586,7 +610,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
>  
>  	if (rte_mempool_get(mp, (void **)&m) < 0)
>  		return NULL;
> -	MBUF_RAW_ALLOC_CHECK(m);
> +	__rte_mbuf_raw_sanity_check(m);
>  	return m;
>  }
>  
> @@ -609,10 +633,7 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
>  {
>  	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
>  		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
> -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> -	RTE_ASSERT(m->next == NULL);
> -	RTE_ASSERT(m->nb_segs == 1);
> -	__rte_mbuf_sanity_check(m, 0);
> +	__rte_mbuf_raw_sanity_check(m);
>  	rte_mempool_put(m->pool, m);
>  }
>  
> @@ -858,8 +879,6 @@ static inline void rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
>   * @param m
>   *   The packet mbuf to be reset.
>   */
> -#define MBUF_INVALID_PORT UINT16_MAX
> -
>  static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
>  {
>  	m->next = NULL;
> @@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
>  	m->vlan_tci = 0;
>  	m->vlan_tci_outer = 0;
>  	m->nb_segs = 1;
> -	m->port = MBUF_INVALID_PORT;
> +	m->port = RTE_MBUF_PORT_INVALID;
>  
>  	m->ol_flags &= EXT_ATTACHED_MBUF;
>  	m->packet_type = 0;
> @@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
>  	switch (count % 4) {
>  	case 0:
>  		while (idx != count) {
> -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
>  			rte_pktmbuf_reset(mbufs[idx]);
>  			idx++;
>  			/* fall-through */
>  	case 3:
> -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
>  			rte_pktmbuf_reset(mbufs[idx]);
>  			idx++;
>  			/* fall-through */
>  	case 2:
> -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
>  			rte_pktmbuf_reset(mbufs[idx]);
>  			idx++;
>  			/* fall-through */
>  	case 1:
> -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
>  			rte_pktmbuf_reset(mbufs[idx]);
>  			idx++;
>  			/* fall-through */
> diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> index 8cd7137ac..4ac5609e3 100644
> --- a/lib/librte_mbuf/rte_mbuf_core.h
> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> @@ -272,7 +272,7 @@ extern "C" {
>   * mbuf 'vlan_tci' & 'vlan_tci_outer' must be valid when this flag is set.
>   */
>  #define PKT_TX_QINQ        (1ULL << 49)
> -/* this old name is deprecated */
> +/** This old name is deprecated. */
>  #define PKT_TX_QINQ_PKT    PKT_TX_QINQ
>  
>  /**
> @@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info {
>  	};
>  };
>  
> -/**< Maximum number of nb_segs allowed. */
> +/** Maximum number of nb_segs allowed. */
>  #define RTE_MBUF_MAX_NB_SEGS	UINT16_MAX
>  
>  /**
> @@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info {
>  #define RTE_MBUF_DIRECT(mb) \
>  	(!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF)))
>  
> -#define MBUF_INVALID_PORT UINT16_MAX
> +/** NULL value for the uint16_t port type. */
> +#define RTE_MBUF_PORT_INVALID UINT16_MAX

I don't really like talking about "NULL". What do you think instead of
this wording?

  /** Uninitialized or unspecified port */

> +/** For backwards compatibility. */
> +#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID
>  
>  /**
>   * A macro that points to an offset into the data in the mbuf.
> -- 
> 2.17.1
> 

Thanks,
Olivier

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: minor cleanup
  2020-10-07  9:16 ` Olivier Matz
@ 2020-10-20 11:55   ` Thomas Monjalon
  2020-11-04 22:17     ` Morten Brørup
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Monjalon @ 2020-10-20 11:55 UTC (permalink / raw)
  To: Morten Brørup; +Cc: dev, Olivier Matz

Hi Morten,
Any update about this patch please?

07/10/2020 11:16, Olivier Matz:
> Hi Morten,
> 
> Thanks for this cleanup. Please see some comments below.
> 
> On Wed, Sep 16, 2020 at 12:40:13PM +0200, Morten Brørup wrote:
> > The mbuf header files had some commenting style errors that affected the
> > API documentation.
> > Also, the RTE_ prefix was missing on a macro and a definition.
> > 
> > Note: This patch does not touch the offload and attachment flags that are
> > also missing the RTE_ prefix.
> > 
> > Changes only affecting documentation:
> > * Removed the MBUF_INVALID_PORT definition from rte_mbuf.h; it is
> >   already defined in rte_mbuf_core.h.
> >   This removal also reestablished the description of the
> >   rte_pktmbuf_reset() function.
> > * Corrected the comment related to RTE_MBUF_MAX_NB_SEGS.
> > * Corrected the comment related to PKT_TX_QINQ_PKT.
> > 
> > Changes regarding missing RTE_ prefix:
> > * Converted the MBUF_RAW_ALLOC_CHECK() macro to an
> >   __rte_mbuf_raw_sanity_check() inline function.
> >   Added backwards compatible macro with the original name.
> > * Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID.
> >   Added backwards compatible definition with the original name.
> > 
> > v2:
> > * Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c.
> > 
> > v3:
> > * The functions/macros used in __rte_mbuf_raw_sanity_check() require
> >   RTE_ENABLE_ASSERT or RTE_LIBRTE_MBUF_DEBUG, or they don't use the mbuf
> >   parameter, which generates a compiler waning. So mark the mbuf parameter
> >   __rte_unused if none of them are defined.
> > 
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst |  7 ----
> >  lib/librte_mbuf/rte_mbuf.c           |  4 +-
> >  lib/librte_mbuf/rte_mbuf.h           | 55 +++++++++++++++++++---------
> >  lib/librte_mbuf/rte_mbuf_core.h      |  9 +++--
> >  4 files changed, 45 insertions(+), 30 deletions(-)
> > 
> > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > index 279eccb04..88d7d0761 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -294,13 +294,6 @@ Deprecation Notices
> >    - https://patches.dpdk.org/patch/71457/
> >    - https://patches.dpdk.org/patch/71456/
> >  
> > -* rawdev: The rawdev APIs which take a device-specific structure as
> > -  parameter directly, or indirectly via a "private" pointer inside another
> > -  structure, will be modified to take an additional parameter of the
> > -  structure size. The affected APIs will include ``rte_rawdev_info_get``,
> > -  ``rte_rawdev_configure``, ``rte_rawdev_queue_conf_get`` and
> > -  ``rte_rawdev_queue_setup``.
> > -
> >  * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value will be removed.
> >    This enum value is not used inside DPDK, while it prevents to add new
> >    classify algorithms without causing an ABI breakage.
> 
> I think this change is not related.
> 
> This makes me think that a deprecation notice could be done for the
> old names without the RTE_ prefix, to be removed in 21.11.
> 
> 
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 8a456e5e6..53a015311 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -104,7 +104,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
> >  	/* init some constant fields */
> >  	m->pool = mp;
> >  	m->nb_segs = 1;
> > -	m->port = MBUF_INVALID_PORT;
> > +	m->port = RTE_MBUF_PORT_INVALID;
> >  	rte_mbuf_refcnt_set(m, 1);
> >  	m->next = NULL;
> >  }
> > @@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp,
> >  	/* init some constant fields */
> >  	m->pool = mp;
> >  	m->nb_segs = 1;
> > -	m->port = MBUF_INVALID_PORT;
> > +	m->port = RTE_MBUF_PORT_INVALID;
> >  	m->ol_flags = EXT_ATTACHED_MBUF;
> >  	rte_mbuf_refcnt_set(m, 1);
> >  	m->next = NULL;
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 7259575a7..406d3abb2 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -554,12 +554,36 @@ __rte_experimental
> >  int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> >  		   const char **reason);
> >  
> > -#define MBUF_RAW_ALLOC_CHECK(m) do {				\
> > -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
> > -	RTE_ASSERT((m)->next == NULL);				\
> > -	RTE_ASSERT((m)->nb_segs == 1);				\
> > -	__rte_mbuf_sanity_check(m, 0);				\
> > -} while (0)
> > +#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG)
> 
> I don't see why this #if is needed. Wouldn't it work to have only
> one function definition with the __rte_unused attribute?
> 
> > +/**
> > + * Sanity checks on a reinitialized mbuf.
> > + *
> > + * Check the consistency of the given reinitialized mbuf.
> > + * The function will cause a panic if corruption is detected.
> > + *
> > + * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
> > + * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
> > + *
> 
> Maybe indicate that these checks are only done when debug is on.
> 
> > + * @param m
> > + *   The mbuf to be checked.
> > + */
> > +static __rte_always_inline void
> > +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m)
> > +{
> > +	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> > +	RTE_ASSERT(m->next == NULL);
> > +	RTE_ASSERT(m->nb_segs == 1);
> > +	__rte_mbuf_sanity_check(m, 0);
> > +}
> > +#else
> > +static __rte_always_inline void
> > +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m __rte_unused)
> > +{
> > +    /* Nothing here. */
> > +}
> > +#endif
> > +/** For backwards compatibility. */
> > +#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
> 
> It looks that MBUF_RAW_ALLOC_CHECK() is also used in drivers/net/sfc,
> I think it should be updated too.
> 
> >  
> >  /**
> >   * Allocate an uninitialized mbuf from mempool *mp*.
> > @@ -586,7 +610,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
> >  
> >  	if (rte_mempool_get(mp, (void **)&m) < 0)
> >  		return NULL;
> > -	MBUF_RAW_ALLOC_CHECK(m);
> > +	__rte_mbuf_raw_sanity_check(m);
> >  	return m;
> >  }
> >  
> > @@ -609,10 +633,7 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
> >  {
> >  	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
> >  		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
> > -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> > -	RTE_ASSERT(m->next == NULL);
> > -	RTE_ASSERT(m->nb_segs == 1);
> > -	__rte_mbuf_sanity_check(m, 0);
> > +	__rte_mbuf_raw_sanity_check(m);
> >  	rte_mempool_put(m->pool, m);
> >  }
> >  
> > @@ -858,8 +879,6 @@ static inline void rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
> >   * @param m
> >   *   The packet mbuf to be reset.
> >   */
> > -#define MBUF_INVALID_PORT UINT16_MAX
> > -
> >  static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
> >  {
> >  	m->next = NULL;
> > @@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
> >  	m->vlan_tci = 0;
> >  	m->vlan_tci_outer = 0;
> >  	m->nb_segs = 1;
> > -	m->port = MBUF_INVALID_PORT;
> > +	m->port = RTE_MBUF_PORT_INVALID;
> >  
> >  	m->ol_flags &= EXT_ATTACHED_MBUF;
> >  	m->packet_type = 0;
> > @@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
> >  	switch (count % 4) {
> >  	case 0:
> >  		while (idx != count) {
> > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> >  			rte_pktmbuf_reset(mbufs[idx]);
> >  			idx++;
> >  			/* fall-through */
> >  	case 3:
> > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> >  			rte_pktmbuf_reset(mbufs[idx]);
> >  			idx++;
> >  			/* fall-through */
> >  	case 2:
> > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> >  			rte_pktmbuf_reset(mbufs[idx]);
> >  			idx++;
> >  			/* fall-through */
> >  	case 1:
> > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> >  			rte_pktmbuf_reset(mbufs[idx]);
> >  			idx++;
> >  			/* fall-through */
> > diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> > index 8cd7137ac..4ac5609e3 100644
> > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > @@ -272,7 +272,7 @@ extern "C" {
> >   * mbuf 'vlan_tci' & 'vlan_tci_outer' must be valid when this flag is set.
> >   */
> >  #define PKT_TX_QINQ        (1ULL << 49)
> > -/* this old name is deprecated */
> > +/** This old name is deprecated. */
> >  #define PKT_TX_QINQ_PKT    PKT_TX_QINQ
> >  
> >  /**
> > @@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info {
> >  	};
> >  };
> >  
> > -/**< Maximum number of nb_segs allowed. */
> > +/** Maximum number of nb_segs allowed. */
> >  #define RTE_MBUF_MAX_NB_SEGS	UINT16_MAX
> >  
> >  /**
> > @@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info {
> >  #define RTE_MBUF_DIRECT(mb) \
> >  	(!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF)))
> >  
> > -#define MBUF_INVALID_PORT UINT16_MAX
> > +/** NULL value for the uint16_t port type. */
> > +#define RTE_MBUF_PORT_INVALID UINT16_MAX
> 
> I don't really like talking about "NULL". What do you think instead of
> this wording?
> 
>   /** Uninitialized or unspecified port */
> 
> > +/** For backwards compatibility. */
> > +#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID
> >  
> >  /**
> >   * A macro that points to an offset into the data in the mbuf.
> 
> Thanks,
> Olivier
> 






^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH v3] mbuf: minor cleanup
  2020-10-20 11:55   ` Thomas Monjalon
@ 2020-11-04 22:17     ` Morten Brørup
  0 siblings, 0 replies; 5+ messages in thread
From: Morten Brørup @ 2020-11-04 22:17 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev, Olivier Matz

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, October 20, 2020 1:56 PM
> 
> Hi Morten,
> Any update about this patch please?

Thomas,

v4 contains all the modifications suggested by Olivier:
https://patchwork.dpdk.org/patch/82198/

Except a depreciation notice about the old names without RTE_. I'm not sure about the formal requirements for depreciation notices. If there are no formal requirements, please feel free to add a notice when merging.

Changes regarding missing RTE_ prefix:
* Converted the MBUF_RAW_ALLOC_CHECK() macro to an
  __rte_mbuf_raw_sanity_check() inline function.
  Added backwards compatible macro with the original name.
* Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID.
  Added backwards compatible definition with the original name.

> 
> 07/10/2020 11:16, Olivier Matz:
> > Hi Morten,
> >
> > Thanks for this cleanup. Please see some comments below.
> >
> > On Wed, Sep 16, 2020 at 12:40:13PM +0200, Morten Brørup wrote:
> > > The mbuf header files had some commenting style errors that affected
> the
> > > API documentation.
> > > Also, the RTE_ prefix was missing on a macro and a definition.
> > >
> > > Note: This patch does not touch the offload and attachment flags that
> are
> > > also missing the RTE_ prefix.
> > >
> > > Changes only affecting documentation:
> > > * Removed the MBUF_INVALID_PORT definition from rte_mbuf.h; it is
> > >   already defined in rte_mbuf_core.h.
> > >   This removal also reestablished the description of the
> > >   rte_pktmbuf_reset() function.
> > > * Corrected the comment related to RTE_MBUF_MAX_NB_SEGS.
> > > * Corrected the comment related to PKT_TX_QINQ_PKT.
> > >
> > > Changes regarding missing RTE_ prefix:
> > > * Converted the MBUF_RAW_ALLOC_CHECK() macro to an
> > >   __rte_mbuf_raw_sanity_check() inline function.
> > >   Added backwards compatible macro with the original name.
> > > * Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID.
> > >   Added backwards compatible definition with the original name.
> > >
> > > v2:
> > > * Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c.
> > >
> > > v3:
> > > * The functions/macros used in __rte_mbuf_raw_sanity_check() require
> > >   RTE_ENABLE_ASSERT or RTE_LIBRTE_MBUF_DEBUG, or they don't use the
> mbuf
> > >   parameter, which generates a compiler waning. So mark the mbuf
> parameter
> > >   __rte_unused if none of them are defined.
> > >
> > > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst |  7 ----
> > >  lib/librte_mbuf/rte_mbuf.c           |  4 +-
> > >  lib/librte_mbuf/rte_mbuf.h           | 55 +++++++++++++++++++---------
> > >  lib/librte_mbuf/rte_mbuf_core.h      |  9 +++--
> > >  4 files changed, 45 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> > > index 279eccb04..88d7d0761 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -294,13 +294,6 @@ Deprecation Notices
> > >    - https://patches.dpdk.org/patch/71457/
> > >    - https://patches.dpdk.org/patch/71456/
> > >
> > > -* rawdev: The rawdev APIs which take a device-specific structure as
> > > -  parameter directly, or indirectly via a "private" pointer inside
> another
> > > -  structure, will be modified to take an additional parameter of the
> > > -  structure size. The affected APIs will include
> ``rte_rawdev_info_get``,
> > > -  ``rte_rawdev_configure``, ``rte_rawdev_queue_conf_get`` and
> > > -  ``rte_rawdev_queue_setup``.
> > > -
> > >  * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value will be removed.
> > >    This enum value is not used inside DPDK, while it prevents to add
> new
> > >    classify algorithms without causing an ABI breakage.
> >
> > I think this change is not related.
> >
> > This makes me think that a deprecation notice could be done for the
> > old names without the RTE_ prefix, to be removed in 21.11.
> >
> >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > > index 8a456e5e6..53a015311 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.c
> > > +++ b/lib/librte_mbuf/rte_mbuf.c
> > > @@ -104,7 +104,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
> > >  	/* init some constant fields */
> > >  	m->pool = mp;
> > >  	m->nb_segs = 1;
> > > -	m->port = MBUF_INVALID_PORT;
> > > +	m->port = RTE_MBUF_PORT_INVALID;
> > >  	rte_mbuf_refcnt_set(m, 1);
> > >  	m->next = NULL;
> > >  }
> > > @@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp,
> > >  	/* init some constant fields */
> > >  	m->pool = mp;
> > >  	m->nb_segs = 1;
> > > -	m->port = MBUF_INVALID_PORT;
> > > +	m->port = RTE_MBUF_PORT_INVALID;
> > >  	m->ol_flags = EXT_ATTACHED_MBUF;
> > >  	rte_mbuf_refcnt_set(m, 1);
> > >  	m->next = NULL;
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 7259575a7..406d3abb2 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -554,12 +554,36 @@ __rte_experimental
> > >  int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
> > >  		   const char **reason);
> > >
> > > -#define MBUF_RAW_ALLOC_CHECK(m) do {				\
> > > -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
> > > -	RTE_ASSERT((m)->next == NULL);				\
> > > -	RTE_ASSERT((m)->nb_segs == 1);				\
> > > -	__rte_mbuf_sanity_check(m, 0);				\
> > > -} while (0)
> > > +#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG)
> >
> > I don't see why this #if is needed. Wouldn't it work to have only
> > one function definition with the __rte_unused attribute?
> >
> > > +/**
> > > + * Sanity checks on a reinitialized mbuf.
> > > + *
> > > + * Check the consistency of the given reinitialized mbuf.
> > > + * The function will cause a panic if corruption is detected.
> > > + *
> > > + * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
> > > + * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
> > > + *
> >
> > Maybe indicate that these checks are only done when debug is on.
> >
> > > + * @param m
> > > + *   The mbuf to be checked.
> > > + */
> > > +static __rte_always_inline void
> > > +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m)
> > > +{
> > > +	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> > > +	RTE_ASSERT(m->next == NULL);
> > > +	RTE_ASSERT(m->nb_segs == 1);
> > > +	__rte_mbuf_sanity_check(m, 0);
> > > +}
> > > +#else
> > > +static __rte_always_inline void
> > > +__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m __rte_unused)
> > > +{
> > > +    /* Nothing here. */
> > > +}
> > > +#endif
> > > +/** For backwards compatibility. */
> > > +#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
> >
> > It looks that MBUF_RAW_ALLOC_CHECK() is also used in drivers/net/sfc,
> > I think it should be updated too.
> >
> > >
> > >  /**
> > >   * Allocate an uninitialized mbuf from mempool *mp*.
> > > @@ -586,7 +610,7 @@ static inline struct rte_mbuf
> *rte_mbuf_raw_alloc(struct rte_mempool *mp)
> > >
> > >  	if (rte_mempool_get(mp, (void **)&m) < 0)
> > >  		return NULL;
> > > -	MBUF_RAW_ALLOC_CHECK(m);
> > > +	__rte_mbuf_raw_sanity_check(m);
> > >  	return m;
> > >  }
> > >
> > > @@ -609,10 +633,7 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
> > >  {
> > >  	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
> > >  		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
> > > -	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
> > > -	RTE_ASSERT(m->next == NULL);
> > > -	RTE_ASSERT(m->nb_segs == 1);
> > > -	__rte_mbuf_sanity_check(m, 0);
> > > +	__rte_mbuf_raw_sanity_check(m);
> > >  	rte_mempool_put(m->pool, m);
> > >  }
> > >
> > > @@ -858,8 +879,6 @@ static inline void
> rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
> > >   * @param m
> > >   *   The packet mbuf to be reset.
> > >   */
> > > -#define MBUF_INVALID_PORT UINT16_MAX
> > > -
> > >  static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
> > >  {
> > >  	m->next = NULL;
> > > @@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct
> rte_mbuf *m)
> > >  	m->vlan_tci = 0;
> > >  	m->vlan_tci_outer = 0;
> > >  	m->nb_segs = 1;
> > > -	m->port = MBUF_INVALID_PORT;
> > > +	m->port = RTE_MBUF_PORT_INVALID;
> > >
> > >  	m->ol_flags &= EXT_ATTACHED_MBUF;
> > >  	m->packet_type = 0;
> > > @@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct
> rte_mempool *pool,
> > >  	switch (count % 4) {
> > >  	case 0:
> > >  		while (idx != count) {
> > > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> > >  			rte_pktmbuf_reset(mbufs[idx]);
> > >  			idx++;
> > >  			/* fall-through */
> > >  	case 3:
> > > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> > >  			rte_pktmbuf_reset(mbufs[idx]);
> > >  			idx++;
> > >  			/* fall-through */
> > >  	case 2:
> > > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> > >  			rte_pktmbuf_reset(mbufs[idx]);
> > >  			idx++;
> > >  			/* fall-through */
> > >  	case 1:
> > > -			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
> > > +			__rte_mbuf_raw_sanity_check(mbufs[idx]);
> > >  			rte_pktmbuf_reset(mbufs[idx]);
> > >  			idx++;
> > >  			/* fall-through */
> > > diff --git a/lib/librte_mbuf/rte_mbuf_core.h
> b/lib/librte_mbuf/rte_mbuf_core.h
> > > index 8cd7137ac..4ac5609e3 100644
> > > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > > @@ -272,7 +272,7 @@ extern "C" {
> > >   * mbuf 'vlan_tci' & 'vlan_tci_outer' must be valid when this flag is
> set.
> > >   */
> > >  #define PKT_TX_QINQ        (1ULL << 49)
> > > -/* this old name is deprecated */
> > > +/** This old name is deprecated. */
> > >  #define PKT_TX_QINQ_PKT    PKT_TX_QINQ
> > >
> > >  /**
> > > @@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info {
> > >  	};
> > >  };
> > >
> > > -/**< Maximum number of nb_segs allowed. */
> > > +/** Maximum number of nb_segs allowed. */
> > >  #define RTE_MBUF_MAX_NB_SEGS	UINT16_MAX
> > >
> > >  /**
> > > @@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info {
> > >  #define RTE_MBUF_DIRECT(mb) \
> > >  	(!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF)))
> > >
> > > -#define MBUF_INVALID_PORT UINT16_MAX
> > > +/** NULL value for the uint16_t port type. */
> > > +#define RTE_MBUF_PORT_INVALID UINT16_MAX
> >
> > I don't really like talking about "NULL". What do you think instead of
> > this wording?
> >
> >   /** Uninitialized or unspecified port */
> >
> > > +/** For backwards compatibility. */
> > > +#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID
> > >
> > >  /**
> > >   * A macro that points to an offset into the data in the mbuf.
> >
> > Thanks,
> > Olivier
> >
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [dpdk-dev] [PATCH v3] mbuf: minor cleanup
  2020-09-16  8:17 [dpdk-dev] [PATCH 1/1] " Morten Brørup
@ 2020-09-16 10:43 ` Morten Brørup
  0 siblings, 0 replies; 5+ messages in thread
From: Morten Brørup @ 2020-09-16 10:43 UTC (permalink / raw)
  To: olivier.matz, thomas; +Cc: dev, Morten Brørup

The mbuf header files had some commenting style errors that affected the
API documentation.
Also, the RTE_ prefix was missing on a macro and a definition.

Note: This patch does not touch the offload and attachment flags that are
also missing the RTE_ prefix.

Changes only affecting documentation:
* Removed the MBUF_INVALID_PORT definition from rte_mbuf.h; it is
  already defined in rte_mbuf_core.h.
  This removal also reestablished the description of the
  rte_pktmbuf_reset() function.
* Corrected the comment related to RTE_MBUF_MAX_NB_SEGS.
* Corrected the comment related to PKT_TX_QINQ_PKT.

Changes regarding missing RTE_ prefix:
* Converted the MBUF_RAW_ALLOC_CHECK() macro to an
  __rte_mbuf_raw_sanity_check() inline function.
  Added backwards compatible macro with the original name.
* Renamed the MBUF_INVALID_PORT definition to RTE_MBUF_PORT_INVALID.
  Added backwards compatible definition with the original name.

v2:
* Use RTE_MBUF_PORT_INVALID instead of MBUF_INVALID_PORT in rte_mbuf.c.

v3:
* The functions/macros used in __rte_mbuf_raw_sanity_check() require
  RTE_ENABLE_ASSERT or RTE_LIBRTE_MBUF_DEBUG, or they don't use the mbuf
  parameter, which generates a compiler waning. So mark the mbuf parameter
  __rte_unused if none of them are defined.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 doc/guides/rel_notes/deprecation.rst |  7 ----
 lib/librte_mbuf/rte_mbuf.c           |  4 +-
 lib/librte_mbuf/rte_mbuf.h           | 55 +++++++++++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h      |  9 +++--
 4 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 279eccb04..88d7d0761 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -294,13 +294,6 @@ Deprecation Notices
   - https://patches.dpdk.org/patch/71457/
   - https://patches.dpdk.org/patch/71456/
 
-* rawdev: The rawdev APIs which take a device-specific structure as
-  parameter directly, or indirectly via a "private" pointer inside another
-  structure, will be modified to take an additional parameter of the
-  structure size. The affected APIs will include ``rte_rawdev_info_get``,
-  ``rte_rawdev_configure``, ``rte_rawdev_queue_conf_get`` and
-  ``rte_rawdev_queue_setup``.
-
 * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value will be removed.
   This enum value is not used inside DPDK, while it prevents to add new
   classify algorithms without causing an ABI breakage.
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 8a456e5e6..53a015311 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -104,7 +104,7 @@ rte_pktmbuf_init(struct rte_mempool *mp,
 	/* init some constant fields */
 	m->pool = mp;
 	m->nb_segs = 1;
-	m->port = MBUF_INVALID_PORT;
+	m->port = RTE_MBUF_PORT_INVALID;
 	rte_mbuf_refcnt_set(m, 1);
 	m->next = NULL;
 }
@@ -207,7 +207,7 @@ __rte_pktmbuf_init_extmem(struct rte_mempool *mp,
 	/* init some constant fields */
 	m->pool = mp;
 	m->nb_segs = 1;
-	m->port = MBUF_INVALID_PORT;
+	m->port = RTE_MBUF_PORT_INVALID;
 	m->ol_flags = EXT_ATTACHED_MBUF;
 	rte_mbuf_refcnt_set(m, 1);
 	m->next = NULL;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7259575a7..406d3abb2 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -554,12 +554,36 @@ __rte_experimental
 int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 		   const char **reason);
 
-#define MBUF_RAW_ALLOC_CHECK(m) do {				\
-	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);		\
-	RTE_ASSERT((m)->next == NULL);				\
-	RTE_ASSERT((m)->nb_segs == 1);				\
-	__rte_mbuf_sanity_check(m, 0);				\
-} while (0)
+#if defined(RTE_ENABLE_ASSERT) || defined(RTE_LIBRTE_MBUF_DEBUG)
+/**
+ * Sanity checks on a reinitialized mbuf.
+ *
+ * Check the consistency of the given reinitialized mbuf.
+ * The function will cause a panic if corruption is detected.
+ *
+ * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
+ * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
+ *
+ * @param m
+ *   The mbuf to be checked.
+ */
+static __rte_always_inline void
+__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m)
+{
+	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
+	RTE_ASSERT(m->next == NULL);
+	RTE_ASSERT(m->nb_segs == 1);
+	__rte_mbuf_sanity_check(m, 0);
+}
+#else
+static __rte_always_inline void
+__rte_mbuf_raw_sanity_check(const struct rte_mbuf *m __rte_unused)
+{
+    /* Nothing here. */
+}
+#endif
+/** For backwards compatibility. */
+#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
 
 /**
  * Allocate an uninitialized mbuf from mempool *mp*.
@@ -586,7 +610,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
 
 	if (rte_mempool_get(mp, (void **)&m) < 0)
 		return NULL;
-	MBUF_RAW_ALLOC_CHECK(m);
+	__rte_mbuf_raw_sanity_check(m);
 	return m;
 }
 
@@ -609,10 +633,7 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
 {
 	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
 		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
-	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
-	RTE_ASSERT(m->next == NULL);
-	RTE_ASSERT(m->nb_segs == 1);
-	__rte_mbuf_sanity_check(m, 0);
+	__rte_mbuf_raw_sanity_check(m);
 	rte_mempool_put(m->pool, m);
 }
 
@@ -858,8 +879,6 @@ static inline void rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
  * @param m
  *   The packet mbuf to be reset.
  */
-#define MBUF_INVALID_PORT UINT16_MAX
-
 static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 {
 	m->next = NULL;
@@ -868,7 +887,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 	m->vlan_tci = 0;
 	m->vlan_tci_outer = 0;
 	m->nb_segs = 1;
-	m->port = MBUF_INVALID_PORT;
+	m->port = RTE_MBUF_PORT_INVALID;
 
 	m->ol_flags &= EXT_ATTACHED_MBUF;
 	m->packet_type = 0;
@@ -931,22 +950,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
 	switch (count % 4) {
 	case 0:
 		while (idx != count) {
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 3:
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 2:
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 1:
-			MBUF_RAW_ALLOC_CHECK(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check(mbufs[idx]);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 8cd7137ac..4ac5609e3 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -272,7 +272,7 @@ extern "C" {
  * mbuf 'vlan_tci' & 'vlan_tci_outer' must be valid when this flag is set.
  */
 #define PKT_TX_QINQ        (1ULL << 49)
-/* this old name is deprecated */
+/** This old name is deprecated. */
 #define PKT_TX_QINQ_PKT    PKT_TX_QINQ
 
 /**
@@ -686,7 +686,7 @@ struct rte_mbuf_ext_shared_info {
 	};
 };
 
-/**< Maximum number of nb_segs allowed. */
+/** Maximum number of nb_segs allowed. */
 #define RTE_MBUF_MAX_NB_SEGS	UINT16_MAX
 
 /**
@@ -714,7 +714,10 @@ struct rte_mbuf_ext_shared_info {
 #define RTE_MBUF_DIRECT(mb) \
 	(!((mb)->ol_flags & (IND_ATTACHED_MBUF | EXT_ATTACHED_MBUF)))
 
-#define MBUF_INVALID_PORT UINT16_MAX
+/** NULL value for the uint16_t port type. */
+#define RTE_MBUF_PORT_INVALID UINT16_MAX
+/** For backwards compatibility. */
+#define MBUF_INVALID_PORT RTE_MBUF_PORT_INVALID
 
 /**
  * A macro that points to an offset into the data in the mbuf.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-04 22:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 10:40 [dpdk-dev] [PATCH v3] mbuf: minor cleanup Morten Brørup
2020-10-07  9:16 ` Olivier Matz
2020-10-20 11:55   ` Thomas Monjalon
2020-11-04 22:17     ` Morten Brørup
  -- strict thread matches above, loose matches on Subject: below --
2020-09-16  8:17 [dpdk-dev] [PATCH 1/1] " Morten Brørup
2020-09-16 10:43 ` [dpdk-dev] [PATCH v3] " Morten Brørup

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).