pdump: small enhancements

PATCH Superseded Stephen Hemminger 3 participants 9 messages Dec 27, 2020 – Jan 19, 2021

lore

Share & tools

lore

Stephen Hemminger

author

2020-12-27 03:33 UTC #0 lore raw

cover pdump: small enhancements

Found these while doing pcapng support and new capture command. They are standalone enhancements.

Stephen Hemminger (2):
pdump: use rte_pktmbuf_free bulk
pdump: replace DEVICE_ID_SIZE with RTE_DEV_NAME_MAX_LEN

 app/pdump/main.c             |  8 ++++----
 lib/librte_pdump/rte_pdump.c | 19 ++++++++-----------
 2 files changed, 12 insertions(+), 15 deletions(-)

Signature · 1 line

2.29.2

Stephen Hemminger

author

2020-12-27 03:34 UTC #1 lore raw

PATCH 1/2 pdump: use rte_pktmbuf_free bulk

Use rte_pktmbuf_free_bulk instead of loop when freeing packets.

Signed-off-by Stephen Hemminger <[email protected]>

---
 app/pdump/main.c             | 8 ++++----
 lib/librte_pdump/rte_pdump.c | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

app/pdump/main.c +4 -4

index b34bf335317b..f986c7e9d67e 100644
@@ -477,10 +477,10 @@ pdump_rxtx(struct rte_ring *ring, uint16_t vdev_id, struct pdump_stats *stats)
 		stats->tx_pkts += nb_in_txd;
 
 		if (unlikely(nb_in_txd < nb_in_deq)) {
-			do {
-				rte_pktmbuf_free(rxtx_bufs[nb_in_txd]);
-				stats->freed_pkts++;
-			} while (++nb_in_txd < nb_in_deq);
+			unsigned int drops = nb_in_deq - nb_in_txd;
+
+			rte_pktmbuf_free_bulk(&rxtx_bufs[nb_in_txd], drops);
+			stats->freed_pkts += drops;;
 		}
 	}
 }

lib/librte_pdump/rte_pdump.c +3 -3

index b3c8d5ce4384..3c11bd795bc1 100644
@@ -93,11 +93,11 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
 
 	ring_enq = rte_ring_enqueue_burst(ring, (void *)dup_bufs, d_pkts, NULL);
 	if (unlikely(ring_enq < d_pkts)) {
+		unsigned int drops = d_pkts - ring_enq;
+
 		PDUMP_LOG(DEBUG,
 			"only %d of packets enqueued to ring\n", ring_enq);
-		do {
-			rte_pktmbuf_free(dup_bufs[ring_enq]);
-		} while (++ring_enq < d_pkts);
+		rte_pktmbuf_free_bulk(&dup_bufs[ring_enq], drops);
 	}
 }

Signature · 1 line

2.29.2

Morten Brørup

Acked-by on v1 1/2

2020-12-29 05:40 UTC #2 lore raw

Re: [dpdk-dev] [PATCH 1/2] pdump: use rte_pktmbuf_free bulk

Quoted 19 lines

From: dev [mailto:[email protected]] On Behalf Of Stephen Hemminger
Sent: Sunday, December 27, 2020 4:34 AM
Use rte_pktmbuf_free_bulk instead of loop when freeing packets.
Signed-off-by Stephen Hemminger <[email protected]>
---
 app/pdump/main.c             | 8 ++++----
 lib/librte_pdump/rte_pdump.c | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)
app/pdump/main.c +0 -0
index b34bf335317b..f986c7e9d67e 100644
@@ -477,10 +477,10 @@ pdump_rxtx(struct rte_ring *ring, uint16_t
 vdev_id, struct pdump_stats *stats)
 		stats->tx_pkts += nb_in_txd;

app/pdump/main.c pdump_rxtx(struct rte_ring *ring, uint16_t · line 484

Quoted 10 lines

app/pdump/main.c +4 -4

 
 		if (unlikely(nb_in_txd < nb_in_deq)) {
-			do {
-				rte_pktmbuf_free(rxtx_bufs[nb_in_txd]);
-				stats->freed_pkts++;
-			} while (++nb_in_txd < nb_in_deq);
+			unsigned int drops = nb_in_deq - nb_in_txd;
+
+			rte_pktmbuf_free_bulk(&rxtx_bufs[nb_in_txd], drops);
+			stats->freed_pkts += drops;;

One semicolon should be enough.

And shouldn't checkpatch find typos like this?

Quoted 25 lines

app/pdump/main.c +0 -0

 		}
 	}

}

lib/librte_pdump/rte_pdump.c +3 -3

index b3c8d5ce4384..3c11bd795bc1 100644
@@ -93,11 +93,11 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t
 nb_pkts, void *user_params)
 
 	ring_enq = rte_ring_enqueue_burst(ring, (void *)dup_bufs, d_pkts,
 NULL);
 	if (unlikely(ring_enq < d_pkts)) {
+		unsigned int drops = d_pkts - ring_enq;
+
 		PDUMP_LOG(DEBUG,
 			"only %d of packets enqueued to ring\n", ring_enq);
-		do {
-			rte_pktmbuf_free(dup_bufs[ring_enq]);
-		} while (++ring_enq < d_pkts);
+		rte_pktmbuf_free_bulk(&dup_bufs[ring_enq], drops);
 	}

}

Quoted 3 lines

--
2.29.2

Acked-by Morten Brørup <[email protected]>

Stephen Hemminger

author

2020-12-27 03:34 UTC #3 lore raw

PATCH 2/2 pdump: replace DEVICE_ID_SIZE with RTE_DEV_NAME_MAX_LEN

In reply to Stephen Hemminger

The device string has an existing size in rte_dev.h use that instead of defining our own.

Signed-off-by Stephen Hemminger <[email protected]>

---
 lib/librte_pdump/rte_pdump.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

lib/librte_pdump/rte_pdump.c +5 -8

index 3c11bd795bc1..14a392ef0340 100644
@@ -12,8 +12,6 @@
 
 #include "rte_pdump.h"
 
-#define DEVICE_ID_SIZE 64
-
 RTE_LOG_REGISTER(pdump_logtype, lib.pdump, NOTICE);
 
 /* Macro for printing using RTE_LOG */

@@ -39,14 +37,14 @@ struct pdump_request {
 	uint32_t flags;
 	union pdump_data {
 		struct enable_v1 {
-			char device[DEVICE_ID_SIZE];
+			char device[RTE_DEV_NAME_MAX_LEN];
 			uint16_t queue;
 			struct rte_ring *ring;
 			struct rte_mempool *mp;
 			void *filter;
 		} en_v1;
 		struct disable_v1 {
-			char device[DEVICE_ID_SIZE];
+			char device[RTE_DEV_NAME_MAX_LEN];
 			uint16_t queue;
 			struct rte_ring *ring;
 			struct rte_mempool *mp;

@@ -485,9 +483,8 @@ rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
 			struct rte_mempool *mp,
 			void *filter)
 {
-
-	int ret = 0;
-	char name[DEVICE_ID_SIZE];
+	int ret;
+	char name[RTE_DEV_NAME_MAX_LEN];
 
 	ret = pdump_validate_port(port, name);
 	if (ret < 0)

@@ -531,7 +528,7 @@ int
 rte_pdump_disable(uint16_t port, uint16_t queue, uint32_t flags)
 {
 	int ret = 0;
-	char name[DEVICE_ID_SIZE];
+	char name[RTE_DEV_NAME_MAX_LEN];
 
 	ret = pdump_validate_port(port, name);
 	if (ret < 0)

Signature · 1 line

2.29.2

Stephen Hemminger

author

2020-12-29 20:09 UTC #4 lore raw

v2 cover pdump: small enhancements

In reply to Stephen Hemminger

Found these while doing pcapng support and new capture command. They are standalone enhancements.

Stephen Hemminger (3):
pdump: use rte_pktmbuf_free bulk
pdump: replace DEVICE_ID_SIZE with RTE_DEV_NAME_MAX_LEN
pdump: cleanup checkpatch warnings

v2 - add more checkpatch fixes

 app/pdump/main.c             |  8 +++---
 lib/librte_pdump/rte_pdump.c | 47 +++++++++++++++++-------------------
 2 files changed, 26 insertions(+), 29 deletions(-)

Signature · 1 line

2.29.2

Stephen Hemminger

author

2020-12-29 20:09 UTC #5 lore raw

PATCH v2 1/3 pdump: use rte_pktmbuf_free bulk

Use rte_pktmbuf_free_bulk instead of loop when freeing packets.

Signed-off-by Stephen Hemminger <[email protected]>

---
 app/pdump/main.c             | 8 ++++----
 lib/librte_pdump/rte_pdump.c | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

app/pdump/main.c +4 -4

index b34bf335317b..63bbe65cd843 100644
@@ -477,10 +477,10 @@ pdump_rxtx(struct rte_ring *ring, uint16_t vdev_id, struct pdump_stats *stats)
 		stats->tx_pkts += nb_in_txd;
 
 		if (unlikely(nb_in_txd < nb_in_deq)) {
-			do {
-				rte_pktmbuf_free(rxtx_bufs[nb_in_txd]);
-				stats->freed_pkts++;
-			} while (++nb_in_txd < nb_in_deq);
+			unsigned int drops = nb_in_deq - nb_in_txd;
+
+			rte_pktmbuf_free_bulk(&rxtx_bufs[nb_in_txd], drops);
+			stats->freed_pkts += drops;
 		}
 	}
 }

lib/librte_pdump/rte_pdump.c +3 -3

index b3c8d5ce4384..3c11bd795bc1 100644
@@ -93,11 +93,11 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
 
 	ring_enq = rte_ring_enqueue_burst(ring, (void *)dup_bufs, d_pkts, NULL);
 	if (unlikely(ring_enq < d_pkts)) {
+		unsigned int drops = d_pkts - ring_enq;
+
 		PDUMP_LOG(DEBUG,
 			"only %d of packets enqueued to ring\n", ring_enq);
-		do {
-			rte_pktmbuf_free(dup_bufs[ring_enq]);
-		} while (++ring_enq < d_pkts);
+		rte_pktmbuf_free_bulk(&dup_bufs[ring_enq], drops);
 	}
 }

Signature · 1 line

2.29.2

Stephen Hemminger

author

2020-12-29 20:09 UTC #6 lore raw

PATCH v2 2/3 pdump: replace DEVICE_ID_SIZE with RTE_DEV_NAME_MAX_LEN

In reply to Stephen Hemminger

The device string has an existing size in rte_dev.h use that instead of defining our own.

Signed-off-by Stephen Hemminger <[email protected]>

---
 lib/librte_pdump/rte_pdump.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

lib/librte_pdump/rte_pdump.c +5 -8

index 3c11bd795bc1..14a392ef0340 100644
@@ -12,8 +12,6 @@
 
 #include "rte_pdump.h"
 
-#define DEVICE_ID_SIZE 64
-
 RTE_LOG_REGISTER(pdump_logtype, lib.pdump, NOTICE);
 
 /* Macro for printing using RTE_LOG */

@@ -39,14 +37,14 @@ struct pdump_request {
 	uint32_t flags;
 	union pdump_data {
 		struct enable_v1 {
-			char device[DEVICE_ID_SIZE];
+			char device[RTE_DEV_NAME_MAX_LEN];
 			uint16_t queue;
 			struct rte_ring *ring;
 			struct rte_mempool *mp;
 			void *filter;
 		} en_v1;
 		struct disable_v1 {
-			char device[DEVICE_ID_SIZE];
+			char device[RTE_DEV_NAME_MAX_LEN];
 			uint16_t queue;
 			struct rte_ring *ring;
 			struct rte_mempool *mp;

@@ -485,9 +483,8 @@ rte_pdump_enable(uint16_t port, uint16_t queue, uint32_t flags,
 			struct rte_mempool *mp,
 			void *filter)
 {
-
-	int ret = 0;
-	char name[DEVICE_ID_SIZE];
+	int ret;
+	char name[RTE_DEV_NAME_MAX_LEN];
 
 	ret = pdump_validate_port(port, name);
 	if (ret < 0)

@@ -531,7 +528,7 @@ int
 rte_pdump_disable(uint16_t port, uint16_t queue, uint32_t flags)
 {
 	int ret = 0;
-	char name[DEVICE_ID_SIZE];
+	char name[RTE_DEV_NAME_MAX_LEN];
 
 	ret = pdump_validate_port(port, name);
 	if (ret < 0)

Signature · 1 line

2.29.2

Stephen Hemminger

author

2020-12-29 20:10 UTC #7 lore raw

PATCH v2 3/3 pdump: cleanup checkpatch warnings

In reply to Stephen Hemminger

Checkpatch prefers 'unsigned int' over bare 'unsigned'. Reword the error messages for brevity and clarity so they don't have to be split across multiple lines.

Signed-off-by Stephen Hemminger <[email protected]>

---
 lib/librte_pdump/rte_pdump.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

lib/librte_pdump/rte_pdump.c +14 ~~-14~~

index 14a392ef0340..d20e944e0454 100644
@@ -71,7 +71,7 @@ tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
 static inline void
 pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
 {
-	unsigned i;
+	unsigned int i;
 	int ring_enq;
 	uint16_t d_pkts = 0;
 	struct rte_mbuf *dup_bufs[nb_pkts];

@@ -131,8 +131,7 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 		if (cbs && operation == ENABLE) {
 			if (cbs->cb) {
 				PDUMP_LOG(ERR,
-					"failed to add rx callback for port=%d "
-					"and queue=%d, callback already exists\n",
+					"rx callback for port=%d queue=%d, already exists\n",
 					port, qid);
 				return -EEXIST;
 			}

@@ -152,8 +151,7 @@ pdump_register_rx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 
 			if (cbs->cb == NULL) {
 				PDUMP_LOG(ERR,
-					"failed to delete non existing rx "
-					"callback for port=%d and queue=%d\n",
+					"no existing rx callback for port=%d queue=%d\n",
 					port, qid);
 				return -EINVAL;
 			}

@@ -186,8 +184,7 @@ pdump_register_tx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 		if (cbs && operation == ENABLE) {
 			if (cbs->cb) {
 				PDUMP_LOG(ERR,
-					"failed to add tx callback for port=%d "
-					"and queue=%d, callback already exists\n",
+					"tx callback for port=%d queue=%d, already exists\n",
 					port, qid);
 				return -EEXIST;
 			}

@@ -207,8 +204,7 @@ pdump_register_tx_callbacks(uint16_t end_q, uint16_t port, uint16_t queue,
 
 			if (cbs->cb == NULL) {
 				PDUMP_LOG(ERR,
-					"failed to delete non existing tx "
-					"callback for port=%d and queue=%d\n",
+					"no existing tx callback for port=%d queue=%d\n",
 					port, qid);
 				return -EINVAL;
 			}

@@ -351,7 +347,9 @@ pdump_server(const struct rte_mp_msg *mp_msg, const void *peer)
 int
 rte_pdump_init(void)
 {
-	int ret = rte_mp_action_register(PDUMP_MP, pdump_server);
+	int ret;
+
+	ret = rte_mp_action_register(PDUMP_MP, pdump_server);
 	if (ret && rte_errno != ENOTSUP)
 		return -1;
 	return 0;

@@ -374,14 +372,16 @@ pdump_validate_ring_mp(struct rte_ring *ring, struct rte_mempool *mp)
 		return -1;
 	}
 	if (mp->flags & MEMPOOL_F_SP_PUT || mp->flags & MEMPOOL_F_SC_GET) {
-		PDUMP_LOG(ERR, "mempool with either SP or SC settings"
-		" is not valid for pdump, should have MP and MC settings\n");
+		PDUMP_LOG(ERR,
+			  "mempool with SP or SC set not valid for pdump,"
+			  "must have MP and MC set\n");
 		rte_errno = EINVAL;
 		return -1;
 	}
 	if (rte_ring_is_prod_single(ring) || rte_ring_is_cons_single(ring)) {
-		PDUMP_LOG(ERR, "ring with either SP or SC settings"
-		" is not valid for pdump, should have MP and MC settings\n");
+		PDUMP_LOG(ERR,
+			  "ring with SP or SC set is not valid for pdump,"
+			  "must have MP and MC set\n");
 		rte_errno = EINVAL;
 		return -1;
 	}

Signature · 1 line

2.29.2

Thomas Monjalon

Applied

2021-01-19 14:25 UTC #8 lore raw

In reply to Stephen Hemminger

29/12/2020 21:08, Stephen Hemminger:

Quoted 9 lines

Found these while doing pcapng support and new capture command. They are standalone enhancements.
Stephen Hemminger (3):
pdump: use rte_pktmbuf_free bulk
pdump: replace DEVICE_ID_SIZE with RTE_DEV_NAME_MAX_LEN
pdump: cleanup checkpatch warnings
v2 - add more checkpatch fixes

Applied, thanks