* [dpdk-dev] [PATCH 0/1] mbuf: minor cleanup
@ 2020-09-16 8:17 Morten Brørup
2020-09-16 8:17 ` [dpdk-dev] [PATCH 1/1] " Morten Brørup
0 siblings, 1 reply; 10+ messages in thread
From: Morten Brørup @ 2020-09-16 8:17 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.
This cleanup was also discussed here:
http://inbox.dpdk.org/dev/98CBD80474FA8B44BF855DF32C47DC35C6111E@smartserver.smartshare.dk/
Please note:
This patch does not touch the offload and attachment flags that are missing
the RTE_ prefix. I would rather leave that to another patch from someone who
can also write a devtools script to help converting drivers and applications
to the new names.
Morten Brørup (1):
mbuf: minor cleanup
lib/librte_mbuf/rte_mbuf.h | 47 ++++++++++++++++++++-------------
lib/librte_mbuf/rte_mbuf_core.h | 9 ++++---
2 files changed, 35 insertions(+), 21 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [dpdk-dev] [PATCH 1/1] mbuf: minor cleanup
2020-09-16 8:17 [dpdk-dev] [PATCH 0/1] mbuf: minor cleanup Morten Brørup
@ 2020-09-16 8:17 ` Morten Brørup
2020-09-16 10:43 ` [dpdk-dev] [PATCH v3] " Morten Brørup
2020-10-26 14:42 ` [dpdk-dev] [PATCH v4] " Morten Brørup
0 siblings, 2 replies; 10+ messages in thread
From: Morten Brørup @ 2020-09-16 8:17 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.
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
lib/librte_mbuf/rte_mbuf.h | 47 ++++++++++++++++++++-------------
lib/librte_mbuf/rte_mbuf_core.h | 9 ++++---
2 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7259575a7..ec48da67d 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -554,12 +554,28 @@ __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)
+/**
+ * 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);
+}
+/** For backwards compatibility. */
+#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
/**
* Allocate an uninitialized mbuf from mempool *mp*.
@@ -586,7 +602,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 +625,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 +871,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 +879,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 +942,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] 10+ 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
2020-10-26 14:42 ` [dpdk-dev] [PATCH v4] " Morten Brørup
1 sibling, 0 replies; 10+ 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] 10+ messages in thread
* [dpdk-dev] [PATCH v4] mbuf: minor cleanup
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
@ 2020-10-26 14:42 ` Morten Brørup
2020-11-05 13:36 ` Olivier Matz
1 sibling, 1 reply; 10+ messages in thread
From: Morten Brørup @ 2020-10-26 14:42 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.
v4:
* Removed compile time variants of __rte_mbuf_raw_sanity_check(); keeping
the one variant where the mbuf parameter is marked __rte_unused.
* Reflect in the __rte_mbuf_raw_sanity_check() description headline that
it is for debug mode.
* Improved the description of RTE_MBUF_PORT_INVALID.
* Updated sfc driver to use __rte_mbuf_raw_sanity_check() instead of
MBUF_RAW_ALLOC_CHECK().
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
drivers/net/sfc/sfc_ef100_rx.c | 6 ++--
drivers/net/sfc/sfc_ef10_essb_rx.c | 4 +--
drivers/net/sfc/sfc_ef10_rx.c | 4 +--
drivers/net/sfc/sfc_rx.c | 2 +-
lib/librte_mbuf/rte_mbuf.c | 4 +--
lib/librte_mbuf/rte_mbuf.h | 48 +++++++++++++++++++-----------
lib/librte_mbuf/rte_mbuf_core.h | 9 ++++--
7 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/drivers/net/sfc/sfc_ef100_rx.c b/drivers/net/sfc/sfc_ef100_rx.c
index 5e761601b..c1c56d0e7 100644
--- a/drivers/net/sfc/sfc_ef100_rx.c
+++ b/drivers/net/sfc/sfc_ef100_rx.c
@@ -168,7 +168,7 @@ sfc_ef100_rx_qrefill(struct sfc_ef100_rxq *rxq)
struct sfc_ef100_rx_sw_desc *rxd;
rte_iova_t phys_addr;
- MBUF_RAW_ALLOC_CHECK(m);
+ __rte_mbuf_raw_sanity_check(m);
SFC_ASSERT((id & ~ptr_mask) == 0);
rxd = &rxq->sw_ring[id];
@@ -481,7 +481,7 @@ sfc_ef100_rx_process_ready_pkts(struct sfc_ef100_rxq *rxq,
rxq->ready_pkts--;
pkt = sfc_ef100_rx_next_mbuf(rxq);
- MBUF_RAW_ALLOC_CHECK(pkt);
+ __rte_mbuf_raw_sanity_check(pkt);
RTE_BUILD_BUG_ON(sizeof(pkt->rearm_data[0]) !=
sizeof(rxq->rearm_data));
@@ -505,7 +505,7 @@ sfc_ef100_rx_process_ready_pkts(struct sfc_ef100_rxq *rxq,
struct rte_mbuf *seg;
seg = sfc_ef100_rx_next_mbuf(rxq);
- MBUF_RAW_ALLOC_CHECK(seg);
+ __rte_mbuf_raw_sanity_check(seg);
seg->data_off = RTE_PKTMBUF_HEADROOM;
diff --git a/drivers/net/sfc/sfc_ef10_essb_rx.c b/drivers/net/sfc/sfc_ef10_essb_rx.c
index 97c81c823..b167e01c5 100644
--- a/drivers/net/sfc/sfc_ef10_essb_rx.c
+++ b/drivers/net/sfc/sfc_ef10_essb_rx.c
@@ -125,7 +125,7 @@ sfc_ef10_essb_next_mbuf(const struct sfc_ef10_essb_rxq *rxq,
struct rte_mbuf *m;
m = (struct rte_mbuf *)((uintptr_t)mbuf + rxq->buf_stride);
- MBUF_RAW_ALLOC_CHECK(m);
+ __rte_mbuf_raw_sanity_check(m);
return m;
}
@@ -136,7 +136,7 @@ sfc_ef10_essb_mbuf_by_index(const struct sfc_ef10_essb_rxq *rxq,
struct rte_mbuf *m;
m = (struct rte_mbuf *)((uintptr_t)mbuf + idx * rxq->buf_stride);
- MBUF_RAW_ALLOC_CHECK(m);
+ __rte_mbuf_raw_sanity_check(m);
return m;
}
diff --git a/drivers/net/sfc/sfc_ef10_rx.c b/drivers/net/sfc/sfc_ef10_rx.c
index a43554a13..0a1bc1878 100644
--- a/drivers/net/sfc/sfc_ef10_rx.c
+++ b/drivers/net/sfc/sfc_ef10_rx.c
@@ -148,7 +148,7 @@ sfc_ef10_rx_qrefill(struct sfc_ef10_rxq *rxq)
struct sfc_ef10_rx_sw_desc *rxd;
rte_iova_t phys_addr;
- MBUF_RAW_ALLOC_CHECK(m);
+ __rte_mbuf_raw_sanity_check(m);
SFC_ASSERT((id & ~ptr_mask) == 0);
rxd = &rxq->sw_ring[id];
@@ -297,7 +297,7 @@ sfc_ef10_rx_process_event(struct sfc_ef10_rxq *rxq, efx_qword_t rx_ev,
rxd = &rxq->sw_ring[pending++ & ptr_mask];
m = rxd->mbuf;
- MBUF_RAW_ALLOC_CHECK(m);
+ __rte_mbuf_raw_sanity_check(m);
m->data_off = RTE_PKTMBUF_HEADROOM;
rte_pktmbuf_data_len(m) = seg_len;
diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index de0773b8a..3415dcad7 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c
@@ -109,7 +109,7 @@ sfc_efx_rx_qrefill(struct sfc_efx_rxq *rxq)
++i, id = (id + 1) & rxq->ptr_mask) {
m = objs[i];
- MBUF_RAW_ALLOC_CHECK(m);
+ __rte_mbuf_raw_sanity_check(m);
rxd = &rxq->sw_desc[id];
rxd->mbuf = m;
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 a1414ed7c..f2cd395d2 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -540,12 +540,29 @@ __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)
+/**
+ * Sanity checks on a reinitialized mbuf in debug mode.
+ *
+ * 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(__rte_unused 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);
+}
+
+/** For backwards compatibility. */
+#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
/**
* Allocate an uninitialized mbuf from mempool *mp*.
@@ -572,7 +589,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;
}
@@ -595,10 +612,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);
}
@@ -844,8 +858,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;
@@ -854,7 +866,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;
@@ -917,22 +929,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 065d87d28..f68d467bd 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -279,7 +279,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
/**
@@ -679,7 +679,7 @@ struct rte_mbuf_ext_shared_info {
uint16_t refcnt;
};
-/**< Maximum number of nb_segs allowed. */
+/** Maximum number of nb_segs allowed. */
#define RTE_MBUF_MAX_NB_SEGS UINT16_MAX
/**
@@ -707,7 +707,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
+/** Uninitialized or unspecified port. */
+#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] 10+ messages in thread
* Re: [dpdk-dev] [PATCH v4] mbuf: minor cleanup
2020-10-26 14:42 ` [dpdk-dev] [PATCH v4] " Morten Brørup
@ 2020-11-05 13:36 ` Olivier Matz
2020-11-05 16:58 ` Thomas Monjalon
0 siblings, 1 reply; 10+ messages in thread
From: Olivier Matz @ 2020-11-05 13:36 UTC (permalink / raw)
To: Morten Brørup; +Cc: thomas, dev
On Mon, Oct 26, 2020 at 03:42:01PM +0100, 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.
>
> v4:
> * Removed compile time variants of __rte_mbuf_raw_sanity_check(); keeping
> the one variant where the mbuf parameter is marked __rte_unused.
> * Reflect in the __rte_mbuf_raw_sanity_check() description headline that
> it is for debug mode.
> * Improved the description of RTE_MBUF_PORT_INVALID.
> * Updated sfc driver to use __rte_mbuf_raw_sanity_check() instead of
> MBUF_RAW_ALLOC_CHECK().
>
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH v4] mbuf: minor cleanup
2020-11-05 13:36 ` Olivier Matz
@ 2020-11-05 16:58 ` Thomas Monjalon
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2020-11-05 16:58 UTC (permalink / raw)
To: Morten Brørup; +Cc: dev, Olivier Matz
05/11/2020 14:36, Olivier Matz:
> On Mon, Oct 26, 2020 at 03:42:01PM +0100, 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.
> >
> > v4:
> > * Removed compile time variants of __rte_mbuf_raw_sanity_check(); keeping
> > the one variant where the mbuf parameter is marked __rte_unused.
> > * Reflect in the __rte_mbuf_raw_sanity_check() description headline that
> > it is for debug mode.
> > * Improved the description of RTE_MBUF_PORT_INVALID.
> > * Updated sfc driver to use __rte_mbuf_raw_sanity_check() instead of
> > MBUF_RAW_ALLOC_CHECK().
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
> Thanks!
Applied, thanks
^ permalink raw reply [flat|nested] 10+ messages in thread
* [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; 10+ 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] 10+ messages in thread
* Re: [dpdk-dev] [PATCH v3] mbuf: minor cleanup
2020-09-16 10:40 [dpdk-dev] [PATCH v3] " Morten Brørup
@ 2020-10-07 9:16 ` Olivier Matz
2020-10-20 11:55 ` Thomas Monjalon
0 siblings, 1 reply; 10+ 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] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread
end of thread, other threads:[~2020-11-05 16:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 8:17 [dpdk-dev] [PATCH 0/1] mbuf: minor cleanup Morten Brørup
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
2020-10-26 14:42 ` [dpdk-dev] [PATCH v4] " Morten Brørup
2020-11-05 13:36 ` Olivier Matz
2020-11-05 16:58 ` Thomas Monjalon
2020-09-16 10:40 [dpdk-dev] [PATCH v3] " 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
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).