Skip to contentSkip to conversation

net/mana: use RTE_LOG_DP for logs on datapath

PATCH Applied [email protected] 4 participants 13 messages Feb 21, 2023 – May 2, 2023 Network drivers

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 net/mana: use RTE_LOG_DP for logs on datapath

From: Long Li <[email protected]>

Change all the logging statements on the datapath to use RTE_LOG_DP() to reduce CPU overhead when logging is not enabled for datapath.

Fixes 517ed6e2d5 ("net/mana: add basic driver with build environment")
Cc [email protected]
Signed-off-by Long Li <[email protected]>
---
 drivers/net/mana/gdma.c | 56 +++++++++++++++---------------
 drivers/net/mana/mana.h |  3 ++
 drivers/net/mana/mp.c   |  4 +--
 drivers/net/mana/mr.c   | 77 ++++++++++++++++++++---------------------
 drivers/net/mana/rx.c   | 32 ++++++++---------
 drivers/net/mana/tx.c   | 66 +++++++++++++++++------------------
 6 files changed, 120 insertions(+), 118 deletions(-)
drivers/net/mana/gdma.c +28 -28
index 3d4039014f..f637084137 100644
@@ -14,12 +14,12 @@ gdma_get_wqe_pointer(struct mana_gdma_queue *queue)
 		(queue->head * GDMA_WQE_ALIGNMENT_UNIT_SIZE) &
 		(queue->size - 1);
 
-	DRV_LOG(DEBUG, "txq sq_head %u sq_size %u offset_in_bytes %u",
-		queue->head, queue->size, offset_in_bytes);
+	DP_LOG(DEBUG, "txq sq_head %u sq_size %u offset_in_bytes %u",
+	       queue->head, queue->size, offset_in_bytes);
 
 	if (offset_in_bytes + GDMA_WQE_ALIGNMENT_UNIT_SIZE > queue->size)
-		DRV_LOG(ERR, "fatal error: offset_in_bytes %u too big",
-			offset_in_bytes);
+		DP_LOG(ERR, "fatal error: offset_in_bytes %u too big",
+		       offset_in_bytes);
 
 	return ((uint8_t *)queue->buffer) + offset_in_bytes;
 }

@@ -39,11 +39,11 @@ write_dma_client_oob(uint8_t *work_queue_buffer_pointer,
 		client_oob_size / sizeof(uint32_t);
 	header->client_data_unit = work_request->client_data_unit;
 
-	DRV_LOG(DEBUG, "queue buf %p sgl %u oob_h %u du %u oob_buf %p oob_b %u",
-		work_queue_buffer_pointer, header->num_sgl_entries,
-		header->inline_client_oob_size_in_dwords,
-		header->client_data_unit, work_request->inline_oob_data,
-		work_request->inline_oob_size_in_bytes);
+	DP_LOG(DEBUG, "queue buf %p sgl %u oob_h %u du %u oob_buf %p oob_b %u",
+	       work_queue_buffer_pointer, header->num_sgl_entries,
+	       header->inline_client_oob_size_in_dwords,
+	       header->client_data_unit, work_request->inline_oob_data,
+	       work_request->inline_oob_size_in_bytes);
 
 	p += sizeof(struct gdma_wqe_dma_oob);
 	if (work_request->inline_oob_data &&

@@ -73,8 +73,8 @@ write_scatter_gather_list(uint8_t *work_queue_head_pointer,
 	uint32_t size_to_queue_end;
 	uint32_t sge_list_size;
 
-	DRV_LOG(DEBUG, "work_queue_cur_pointer %p work_request->flags %x",
-		work_queue_cur_pointer, work_request->flags);
+	DP_LOG(DEBUG, "work_queue_cur_pointer %p work_request->flags %x",
+	       work_queue_cur_pointer, work_request->flags);
 
 	num_sge = work_request->num_sgl_elements;
 	sge_list = work_request->sgl;

@@ -110,9 +110,9 @@ write_scatter_gather_list(uint8_t *work_queue_head_pointer,
 		sge_list_size = size;
 	}
 
-	DRV_LOG(DEBUG, "sge %u address 0x%" PRIx64 " size %u key %u list_s %u",
-		num_sge, sge_list->address, sge_list->size,
-		sge_list->memory_key, sge_list_size);
+	DP_LOG(DEBUG, "sge %u address 0x%" PRIx64 " size %u key %u list_s %u",
+	       num_sge, sge_list->address, sge_list->size,
+	       sge_list->memory_key, sge_list_size);
 
 	return sge_list_size;
 }

@@ -141,13 +141,13 @@ gdma_post_work_request(struct mana_gdma_queue *queue,
 	uint32_t queue_free_units = queue->count - (queue->head - queue->tail);
 
 	if (wqe_size / GDMA_WQE_ALIGNMENT_UNIT_SIZE > queue_free_units) {
-		DRV_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
-			wqe_size, queue->count, queue->head, queue->tail);
+		DP_LOG(DEBUG, "WQE size %u queue count %u head %u tail %u",
+		       wqe_size, queue->count, queue->head, queue->tail);
 		return -EBUSY;
 	}
 
-	DRV_LOG(DEBUG, "client_oob_size %u sgl_data_size %u wqe_size %u",
-		client_oob_size, sgl_data_size, wqe_size);
+	DP_LOG(DEBUG, "client_oob_size %u sgl_data_size %u wqe_size %u",
+	       client_oob_size, sgl_data_size, wqe_size);
 
 	if (wqe_info) {
 		wqe_info->wqe_index =

@@ -242,15 +242,15 @@ mana_ring_doorbell(void *db_page, enum gdma_queue_types queue_type,
 		break;
 
 	default:
-		DRV_LOG(ERR, "Unsupported queue type %d", queue_type);
+		DP_LOG(ERR, "Unsupported queue type %d", queue_type);
 		return -1;
 	}
 
 	/* Ensure all writes are done before ringing doorbell */
 	rte_wmb();
 
-	DRV_LOG(DEBUG, "db_page %p addr %p queue_id %u type %u tail %u arm %u",
-		db_page, addr, queue_id, queue_type, tail, arm);
+	DP_LOG(DEBUG, "db_page %p addr %p queue_id %u type %u tail %u arm %u",
+	       db_page, addr, queue_id, queue_type, tail, arm);
 
 	rte_write64(e.as_uint64, addr);
 	return 0;

@@ -274,15 +274,15 @@ gdma_poll_completion_queue(struct mana_gdma_queue *cq, struct gdma_comp *comp)
 				COMPLETION_QUEUE_OWNER_MASK;
 	cqe_owner_bits = cqe->owner_bits;
 
-	DRV_LOG(DEBUG, "comp cqe bits 0x%x owner bits 0x%x",
-		cqe_owner_bits, old_owner_bits);
+	DP_LOG(DEBUG, "comp cqe bits 0x%x owner bits 0x%x",
+	       cqe_owner_bits, old_owner_bits);
 
 	if (cqe_owner_bits == old_owner_bits)
 		return 0; /* No new entry */
 
 	if (cqe_owner_bits != new_owner_bits) {
-		DRV_LOG(ERR, "CQ overflowed, ID %u cqe 0x%x new 0x%x",
-			cq->id, cqe_owner_bits, new_owner_bits);
+		DP_LOG(ERR, "CQ overflowed, ID %u cqe 0x%x new 0x%x",
+		       cq->id, cqe_owner_bits, new_owner_bits);
 		return -1;
 	}
 

@@ -296,8 +296,8 @@ gdma_poll_completion_queue(struct mana_gdma_queue *cq, struct gdma_comp *comp)
 
 	cq->head++;
 
-	DRV_LOG(DEBUG, "comp new 0x%x old 0x%x cqe 0x%x wq %u sq %u head %u",
-		new_owner_bits, old_owner_bits, cqe_owner_bits,
-		comp->work_queue_number, comp->send_work_queue, cq->head);
+	DP_LOG(DEBUG, "comp new 0x%x old 0x%x cqe 0x%x wq %u sq %u head %u",
+	       new_owner_bits, old_owner_bits, cqe_owner_bits,
+	       comp->work_queue_number, comp->send_work_queue, cq->head);
 	return 1;
 }
drivers/net/mana/mana.h +3 -0
index 4a05238a96..7b8c27df2a 100644
@@ -447,6 +447,9 @@ extern int mana_logtype_init;
 	rte_log(RTE_LOG_ ## level, mana_logtype_driver, "%s(): " fmt "\n", \
 		__func__, ## args)
 
+#define DP_LOG(level, fmt, args...) \
+	RTE_LOG_DP(level, PMD, fmt, ## args)
+
 #define PMD_INIT_LOG(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, mana_logtype_init, "%s(): " fmt "\n",\
 		__func__, ## args)
drivers/net/mana/mp.c +2 -2
index 92432c431d..738487f65a 100644
@@ -28,8 +28,8 @@ mana_mp_mr_create(struct mana_priv *priv, uintptr_t addr, uint32_t len)
 	if (!ibv_mr)
 		return -errno;
 
-	DRV_LOG(DEBUG, "MR (2nd) lkey %u addr %p len %zu",
-		ibv_mr->lkey, ibv_mr->addr, ibv_mr->length);
+	DP_LOG(DEBUG, "MR (2nd) lkey %u addr %p len %zu",
+	       ibv_mr->lkey, ibv_mr->addr, ibv_mr->length);
 
 	mr = rte_calloc("MANA MR", 1, sizeof(*mr), 0);
 	if (!mr) {
drivers/net/mana/mr.c +38 -39

182 lines not on this page · raw message

drivers/net/mana/rx.c +16 -16
index 55247889c1..10392ae292 100644
@@ -42,7 +42,7 @@ mana_rq_ring_doorbell(struct mana_rxq *rxq, uint8_t arm)
 			 arm);
 
 	if (ret)
-		DRV_LOG(ERR, "failed to ring RX doorbell ret %d", ret);
+		DP_LOG(ERR, "failed to ring RX doorbell ret %d", ret);
 
 	return ret;
 }

@@ -66,7 +66,7 @@ mana_alloc_and_post_rx_wqe(struct mana_rxq *rxq)
 
 	mr = mana_find_pmd_mr(&rxq->mr_btree, priv, mbuf);
 	if (!mr) {
-		DRV_LOG(ERR, "failed to register RX MR");
+		DP_LOG(ERR, "failed to register RX MR");
 		rte_pktmbuf_free(mbuf);
 		return -ENOMEM;
 	}

@@ -97,7 +97,7 @@ mana_alloc_and_post_rx_wqe(struct mana_rxq *rxq)
 		desc->wqe_size_in_bu = wqe_info.wqe_size_in_bu;
 		rxq->desc_ring_head = (rxq->desc_ring_head + 1) % rxq->num_desc;
 	} else {
-		DRV_LOG(ERR, "failed to post recv ret %d", ret);
+		DP_LOG(DEBUG, "failed to post recv ret %d", ret);
 		return ret;
 	}
 

@@ -116,7 +116,7 @@ mana_alloc_and_post_rx_wqes(struct mana_rxq *rxq)
 	for (i = 0; i < rxq->num_desc; i++) {
 		ret = mana_alloc_and_post_rx_wqe(rxq);
 		if (ret) {
-			DRV_LOG(ERR, "failed to post RX ret = %d", ret);
+			DP_LOG(ERR, "failed to post RX ret = %d", ret);
 			return ret;
 		}
 	}

@@ -395,8 +395,8 @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			(struct mana_rx_comp_oob *)&comp.completion_data[0];
 
 		if (comp.work_queue_number != rxq->gdma_rq.id) {
-			DRV_LOG(ERR, "rxq comp id mismatch wqid=0x%x rcid=0x%x",
-				comp.work_queue_number, rxq->gdma_rq.id);
+			DP_LOG(ERR, "rxq comp id mismatch wqid=0x%x rcid=0x%x",
+			       comp.work_queue_number, rxq->gdma_rq.id);
 			rxq->stats.errors++;
 			break;
 		}

@@ -411,22 +411,22 @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			break;
 
 		case CQE_RX_TRUNCATED:
-			DRV_LOG(ERR, "Drop a truncated packet");
+			DP_LOG(DEBUG, "Drop a truncated packet");
 			rxq->stats.errors++;
 			rte_pktmbuf_free(mbuf);
 			goto drop;
 
 		case CQE_RX_COALESCED_4:
-			DRV_LOG(ERR, "RX coalescing is not supported");
+			DP_LOG(ERR, "RX coalescing is not supported");
 			continue;
 
 		default:
-			DRV_LOG(ERR, "Unknown RX CQE type %d",
-				oob->cqe_hdr.cqe_type);
+			DP_LOG(ERR, "Unknown RX CQE type %d",
+			       oob->cqe_hdr.cqe_type);
 			continue;
 		}
 
-		DRV_LOG(DEBUG, "mana_rx_comp_oob CQE_RX_OKAY rxq %p", rxq);
+		DP_LOG(DEBUG, "mana_rx_comp_oob CQE_RX_OKAY rxq %p", rxq);
 
 		mbuf->data_off = RTE_PKTMBUF_HEADROOM;
 		mbuf->nb_segs = 1;

@@ -470,7 +470,7 @@ mana_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
 		/* Post another request */
 		ret = mana_alloc_and_post_rx_wqe(rxq);
 		if (ret) {
-			DRV_LOG(ERR, "failed to post rx wqe ret=%d", ret);
+			DP_LOG(ERR, "failed to post rx wqe ret=%d", ret);
 			break;
 		}
 

@@ -490,8 +490,8 @@ mana_arm_cq(struct mana_rxq *rxq, uint8_t arm)
 	uint32_t head = rxq->gdma_cq.head %
 		(rxq->gdma_cq.count << COMPLETION_QUEUE_ENTRY_OWNER_BITS_SIZE);
 
-	DRV_LOG(ERR, "Ringing completion queue ID %u head %u arm %d",
-		rxq->gdma_cq.id, head, arm);
+	DP_LOG(DEBUG, "Ringing completion queue ID %u head %u arm %d",
+	       rxq->gdma_cq.id, head, arm);
 
 	return mana_ring_doorbell(priv->db_page, GDMA_QUEUE_COMPLETION,
 				  rxq->gdma_cq.id, head, arm);

@@ -521,8 +521,8 @@ mana_rx_intr_disable(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 
 	if (ret) {
 		if (ret != EAGAIN)
-			DRV_LOG(ERR, "Can't disable RX intr queue %d",
-				rx_queue_id);
+			DP_LOG(ERR, "Can't disable RX intr queue %d",
+			       rx_queue_id);
 	} else {
 		ibv_ack_cq_events(rxq->cq, 1);
 	}
drivers/net/mana/tx.c +33 -33

129 lines not on this page · raw message

Signature · 1 line
2.32.0
#1 lore raw
Quoted 17 lines · On 2/21/2023 8:42 PM, [email protected] wrote:
drivers/net/mana/tx.c +3 -4
index 300bf27cc1..a45b5e289c 100644
@@ -183,17 +183,17 @@ mana_tx_burst(void *dpdk_txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 			(struct mana_tx_comp_oob *)&comp.completion_data[0];
 
 		if (oob->cqe_hdr.cqe_type != CQE_TX_OKAY) {
-			DRV_LOG(ERR,
-				"mana_tx_comp_oob cqe_type %u vendor_err %u",
-				oob->cqe_hdr.cqe_type, oob->cqe_hdr.vendor_err);
+			DP_LOG(ERR,
+			       "mana_tx_comp_oob cqe_type %u vendor_err %u",
+			       oob->cqe_hdr.cqe_type, oob->cqe_hdr.vendor_err);
 			txq->stats.errors++;
 		} else {
-			DRV_LOG(DEBUG, "mana_tx_comp_oob CQE_TX_OKAY");

drivers/net/mana/tx.c mana_tx_burst(void *dpdk_txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) · line 199

Quoted 10 lines
drivers/net/mana/tx.c +2 -1
+			DP_LOG(DEBUG, "mana_tx_comp_oob CQE_TX_OKAY");
 			txq->stats.packets++;
 		}
 
 		if (!desc->pkt) {
-			DRV_LOG(ERR, "mana_txq_desc has a NULL pkt");
+			DP_LOG(ERR, "mana_txq_desc has a NULL pkt");
 		} else {
 			txq->stats.bytes += desc->pkt->data_len;
 			rte_pktmbuf_free(desc->pkt);

I have some doubts about data path logging, let me use above example to discuss this, these are not specific to this driver. TLDR; jump [1].

Above 'ERR' log will leak into the production code, since 'RTE_LOG_DP_LEVEL' value is 'RTE_LOG_INFO' by default.

Question is who will benefit from these error logs, like descriptor having null packet etc..

I can think of following users for the dpdk logs:
a) DPDK library/driver developer
b) DPDK application developer
c) Who deploys/maintains DPDK application
d) DPDK end user

Unless DPDK packagers or who deploys DPDK application updates 'RTE_LOG_DP_LEVEL', all above parties will observe these datapath logs but I assume intended user of it is only (a).

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused approach.

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath logging?

#2 lore raw
Quoted 20 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

Quoted 7 lines

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused approach.

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath logging?

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same thing. Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

#3 lore raw
Quoted 41 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 20 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

Quoted 7 lines

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused approach.

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath logging?

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same thing. Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

Quoted 6 lines

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for application to see.

---
 drivers/net/ixgbe/ixgbe_ethdev.c |   7 --
 drivers/net/ixgbe/ixgbe_logs.h   |  18 -----
 drivers/net/ixgbe/ixgbe_rxtx.c   |  95 +++++++-------------------
 drivers/net/ixgbe/ixgbe_trace.h  | 110 +++++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 97 deletions(-)
 create mode 100644 drivers/net/ixgbe/ixgbe_trace.h
drivers/net/ixgbe/ixgbe_ethdev.c +0 -7
index 88118bc30560..07f8ccdd97cb 100644
@@ -8317,10 +8317,3 @@ RTE_PMD_REGISTER_PARAM_STRING(net_ixgbe_vf,
 
 RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_driver, driver, NOTICE);
-
-#ifdef RTE_ETHDEV_DEBUG_RX
-RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_rx, rx, DEBUG);
-#endif
-#ifdef RTE_ETHDEV_DEBUG_TX
-RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_tx, tx, DEBUG);
-#endif
drivers/net/ixgbe/ixgbe_logs.h +0 -18
index 00ef797ca103..f70fa1fd4afd 100644
@@ -12,24 +12,6 @@ extern int ixgbe_logtype_init;
 
 #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
 
-#ifdef RTE_ETHDEV_DEBUG_RX
-extern int ixgbe_logtype_rx;
-#define PMD_RX_LOG(level, fmt, args...)			\
-	rte_log(RTE_LOG_ ## level, ixgbe_logtype_rx,	\
-		"%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_RX_LOG(level, fmt, args...) do { } while (0)
-#endif
-
-#ifdef RTE_ETHDEV_DEBUG_TX
-extern int ixgbe_logtype_tx;
-#define PMD_TX_LOG(level, fmt, args...)			\
-	rte_log(RTE_LOG_ ## level, ixgbe_logtype_tx,	\
-		"%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_TX_LOG(level, fmt, args...) do { } while (0)
-#endif
-
 extern int ixgbe_logtype_driver;
 #define PMD_DRV_LOG_RAW(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, ixgbe_logtype_driver, "%s(): " fmt, \
drivers/net/ixgbe/ixgbe_rxtx.c +23 -72

186 lines not on this page · raw message

drivers/net/ixgbe/ixgbe_trace.h new file +110 -0

110 lines not on this page · raw message

Signature · 1 line
2.39.1
#4 lore raw
Quoted 54 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 31 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 20 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

Quoted 7 lines

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused approach.

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath logging?

Quoted 6 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Quoted 6 lines

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for application to see.

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Long

Quoted 409 lines

---
 drivers/net/ixgbe/ixgbe_ethdev.c |   7 --
 drivers/net/ixgbe/ixgbe_logs.h   |  18 -----
 drivers/net/ixgbe/ixgbe_rxtx.c   |  95 +++++++-------------------
 drivers/net/ixgbe/ixgbe_trace.h  | 110
+++++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 97 deletions(-)  create mode 100644

drivers/net/ixgbe/ixgbe_trace.h

drivers/net/ixgbe/ixgbe_ethdev.c +0 -0
index 88118bc30560..07f8ccdd97cb 100644
@@ -8317,10 +8317,3 @@
 RTE_PMD_REGISTER_PARAM_STRING(net_ixgbe_vf,
 
 RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_init, init, NOTICE);

RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_driver, driver, NOTICE);

drivers/net/ixgbe/ixgbe_ethdev.c +0 -3
-
-#ifdef RTE_ETHDEV_DEBUG_RX
-RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_rx, rx, DEBUG); -#endif -#ifdef

RTE_ETHDEV_DEBUG_TX -RTE_LOG_REGISTER_SUFFIX(ixgbe_logtype_tx, tx, DEBUG); -#endif diff --git a/drivers/net/ixgbe/ixgbe_logs.h b/drivers/net/ixgbe/ixgbe_logs.h index 00ef797ca103..f70fa1fd4afd 100644

drivers/net/ixgbe/ixgbe_logs.h +0 -16
@@ -12,24 +12,6 @@ extern int ixgbe_logtype_init;
 
 #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
 
-#ifdef RTE_ETHDEV_DEBUG_RX
-extern int ixgbe_logtype_rx;
-#define PMD_RX_LOG(level, fmt, args...)			\
-	rte_log(RTE_LOG_ ## level, ixgbe_logtype_rx,	\
-		"%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_RX_LOG(level, fmt, args...) do { } while (0) -#endif
-
-#ifdef RTE_ETHDEV_DEBUG_TX
-extern int ixgbe_logtype_tx;
-#define PMD_TX_LOG(level, fmt, args...)			\
-	rte_log(RTE_LOG_ ## level, ixgbe_logtype_tx,	\
-		"%s(): " fmt "\n", __func__, ## args)
-#else
-#define PMD_TX_LOG(level, fmt, args...) do { } while (0) -#endif
-
 extern int ixgbe_logtype_driver;
 #define PMD_DRV_LOG_RAW(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, ixgbe_logtype_driver, "%s(): " fmt, \ diff --

git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c index c9d6ca9efea4..f9f0ffebee8e 100644

drivers/net/ixgbe/ixgbe_rxtx.c +6 -10
@@ -46,6 +46,7 @@
 #include <rte_vect.h>
 
 #include "ixgbe_logs.h"
+#include "ixgbe_trace.h"
 #include "base/ixgbe_api.h"
 #include "base/ixgbe_vf.h"
 #include "ixgbe_ethdev.h"

@@ -581,11 +582,8 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)
 	desc_to_clean_to = sw_ring[desc_to_clean_to].last_id;
 	status = txr[desc_to_clean_to].wb.status;
 	if (!(status & rte_cpu_to_le_32(IXGBE_TXD_STAT_DD))) {
-		PMD_TX_LOG(DEBUG,
-			   "TX descriptor %4u is not done"
-			   "(port=%d queue=%d)",
-			   desc_to_clean_to,
-			   txq->port_id, txq->queue_id);
+		ixgbe_trace_tx_not_done(txq->port_id, txq->queue_id,
+desc_to_clean_to);
+
 		/* Failed to clean any descriptors, better luck next time */
 		return -(1);
 	}

@@ -598,11 +596,8 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)
 		nb_tx_to_clean = (uint16_t)(desc_to_clean_to -
 						last_desc_cleaned);
 
-	PMD_TX_LOG(DEBUG,
-		   "Cleaning %4u TX descriptors: %4u to %4u "
-		   "(port=%d queue=%d)",
-		   nb_tx_to_clean, last_desc_cleaned, desc_to_clean_to,
-		   txq->port_id, txq->queue_id);
+	ixgbe_trace_tx_done(txq->port_id, txq->queue_id,
+			    nb_tx_to_clean, last_desc_cleaned,
 desc_to_clean_to);
 
 	/*

* The last descriptor to clean is done, so that means all the @@ -
734,26 +729,14 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,

drivers/net/ixgbe/ixgbe_rxtx.c +1 -8
 		if (tx_last >= txq->nb_tx_desc)
 			tx_last = (uint16_t) (tx_last - txq->nb_tx_desc);
 
-		PMD_TX_LOG(DEBUG, "port_id=%u queue_id=%u pktlen=%u"
-			   " tx_first=%u tx_last=%u",
-			   (unsigned) txq->port_id,
-			   (unsigned) txq->queue_id,
-			   (unsigned) pkt_len,
-			   (unsigned) tx_id,
-			   (unsigned) tx_last);
-
+		ixgbe_trace_tx(txq->port_id, txq->queue_id, pkt_len, tx_id,

tx_last);

drivers/net/ixgbe/ixgbe_rxtx.c +1 -6
 		/*
 		 * Make sure there are enough TX descriptors available to
 		 * transmit the entire packet.
 		 * nb_used better be less than or equal to txq->tx_rs_thresh
 		 */
 		if (nb_used > txq->nb_tx_free) {
-			PMD_TX_LOG(DEBUG,
-				   "Not enough free TX descriptors "
-				   "nb_used=%4u nb_free=%4u "
-				   "(port=%d queue=%d)",
-				   nb_used, txq->nb_tx_free,
-				   txq->port_id, txq->queue_id);
+			ixgbe_trace_tx_full(txq->port_id, txq->queue_id,

nb_used,

drivers/net/ixgbe/ixgbe_rxtx.c +1 -0
+txq->nb_tx_free);
 
 			if (ixgbe_xmit_cleanup(txq) != 0) {
 				/* Could not clean any descriptors */ @@ -

764,17 +747,8 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,

drivers/net/ixgbe/ixgbe_rxtx.c +0 -2
 			/* nb_used better be <= txq->tx_rs_thresh */
 			if (unlikely(nb_used > txq->tx_rs_thresh)) {
-				PMD_TX_LOG(DEBUG,
-					   "The number of descriptors needed

to "

drivers/net/ixgbe/ixgbe_rxtx.c +1 -9
-					   "transmit the packet exceeds the "
-					   "RS bit threshold. This will impact "
-					   "performance."
-					   "nb_used=%4u nb_free=%4u "
-					   "tx_rs_thresh=%4u. "
-					   "(port=%d queue=%d)",
-					   nb_used, txq->nb_tx_free,
-					   txq->tx_rs_thresh,
-					   txq->port_id, txq->queue_id);
+				ixgbe_trace_tx_thresh(txq->port_id, txq-
Quoted 1 line

queue_id,

drivers/net/ixgbe/ixgbe_rxtx.c +1 -0
+						      nb_used, txq->nb_tx_free,

txq->tx_rs_thresh);

drivers/net/ixgbe/ixgbe_rxtx.c +0 -0
 				/*
 				 * Loop here until there are enough TX
 				 * descriptors or until the ring cannot be @@ -

918,10 +892,7 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,

drivers/net/ixgbe/ixgbe_rxtx.c +1 -4
 		/* Set RS bit only on threshold packets' last descriptor */
 		if (txq->nb_tx_used >= txq->tx_rs_thresh) {
-			PMD_TX_LOG(DEBUG,
-				   "Setting RS bit on TXD id="
-				   "%4u (port=%d queue=%d)",
-				   tx_last, txq->port_id, txq->queue_id);
+			ixgbe_trace_tx_rs(txq->port_id, txq->queue_id,

tx_last);

drivers/net/ixgbe/ixgbe_rxtx.c +1 -3
 			cmd_type_len |= IXGBE_TXD_CMD_RS;

@@ -944,9 +915,7 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf
 **tx_pkts,
 	/*
 	 * Set the Transmit Descriptor Tail (TDT)
 	 */
-	PMD_TX_LOG(DEBUG, "port_id=%u queue_id=%u tx_tail=%u
 nb_tx=%u",
-		   (unsigned) txq->port_id, (unsigned) txq->queue_id,
-		   (unsigned) tx_id, (unsigned) nb_tx);
+	ixgbe_trace_tx_tail(txq->port_id, txq->queue_id, tx_id, nb_tx);
 	IXGBE_PCI_REG_WC_WRITE_RELAXED(txq->tdt_reg_addr, tx_id);
drivers/net/ixgbe/ixgbe_rxtx.c +0 -1
 	txq->tx_tail = tx_id;

@@ -1717,9 +1686,7 @@ rx_recv_pkts(void *rx_queue, struct rte_mbuf
 **rx_pkts,
 		if (ixgbe_rx_alloc_bufs(rxq, true) != 0) {
 			int i, j;
 
-			PMD_RX_LOG(DEBUG, "RX mbuf alloc failed

port_id=%u "

drivers/net/ixgbe/ixgbe_rxtx.c +1 -2
-				   "queue_id=%u", (unsigned) rxq->port_id,
-				   (unsigned) rxq->queue_id);
+			ixgbe_trace_rx_mbuf_alloc_failed(rxq->port_id, rxq-
Quoted 1 line

queue_id);

rte_eth_devices[rxq->port_id].data-

Quoted 1 line

rx_mbuf_alloc_failed +=

drivers/net/ixgbe/ixgbe_rxtx.c +2 -6
 				rxq->rx_free_thresh;

@@ -1851,17 +1818,12 @@ ixgbe_recv_pkts(void *rx_queue, struct
 rte_mbuf **rx_pkts,
 		 * to happen by sending specific "back-pressure" flow control
 		 * frames to its peer(s).
 		 */
-		PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
-			   "ext_err_stat=0x%08x pkt_len=%u",
-			   (unsigned) rxq->port_id, (unsigned) rxq->queue_id,
-			   (unsigned) rx_id, (unsigned) staterr,
-			   (unsigned) rte_le_to_cpu_16(rxd.wb.upper.length));
+		ixgbe_trace_rx(rxq->port_id, rxq->queue_id, rx_id,
+			       staterr, rte_le_to_cpu_16(rxd.wb.upper.length));
 
 		nmb = rte_mbuf_raw_alloc(rxq->mb_pool);
 		if (nmb == NULL) {
-			PMD_RX_LOG(DEBUG, "RX mbuf alloc failed

port_id=%u "

drivers/net/ixgbe/ixgbe_rxtx.c +1 -2
-				   "queue_id=%u", (unsigned) rxq->port_id,
-				   (unsigned) rxq->queue_id);
+			ixgbe_trace_rx_mbuf_alloc_failed(rxq->port_id, rxq-
Quoted 1 line

queue_id);

rte_eth_devices[rxq->port_id].data-

Quoted 1 line

rx_mbuf_alloc_failed++;

drivers/net/ixgbe/ixgbe_rxtx.c +0 -1
 			break;
 		}

@@ -1959,11 +1921,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf
 **rx_pkts,
 	 */
 	nb_hold = (uint16_t) (nb_hold + rxq->nb_rx_hold);
 	if (nb_hold > rxq->rx_free_thresh) {
-		PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_tail=%u

"

drivers/net/ixgbe/ixgbe_rxtx.c +3 -4
-			   "nb_hold=%u nb_rx=%u",
-			   (unsigned) rxq->port_id, (unsigned) rxq->queue_id,
-			   (unsigned) rx_id, (unsigned) nb_hold,
-			   (unsigned) nb_rx);
+		ixgbe_trace_rx_post(rxq->port_id, rxq->queue_id,
+				    rx_id, nb_hold, nb_rx);
+
 		rx_id = (uint16_t) ((rx_id == 0) ?
 				     (rxq->nb_rx_desc - 1) : (rx_id - 1));
 		IXGBE_PCI_REG_WC_WRITE(rxq->rdt_reg_addr, rx_id); @@ -

2124,17 +2084,13 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts,

drivers/net/ixgbe/ixgbe_rxtx.c +3 -7
 		rxd = *rxdp;
 
-		PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
-				  "staterr=0x%x data_len=%u",
-			   rxq->port_id, rxq->queue_id, rx_id, staterr,
-			   rte_le_to_cpu_16(rxd.wb.upper.length));
+		ixgbe_trace_rx(rxq->port_id, rxq->queue_id, rx_id, staterr,
+			       rte_le_to_cpu_16(rxd.wb.upper.length));
 
 		if (!bulk_alloc) {
 			nmb = rte_mbuf_raw_alloc(rxq->mb_pool);
 			if (nmb == NULL) {
-				PMD_RX_LOG(DEBUG, "RX mbuf alloc failed "
-						  "port_id=%u queue_id=%u",
-					   rxq->port_id, rxq->queue_id);
+				ixgbe_trace_rx_mbuf_alloc_failed(rxq-
Quoted 1 line

port_id, rxq->queue_id);

rte_eth_devices[rxq->port_id].data->

	rx_mbuf_alloc_failed++;
drivers/net/ixgbe/ixgbe_rxtx.c +1 -4
@@ -2150,10 +2106,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct
 rte_mbuf **rx_pkts, uint16_t nb_pkts,
 							next_rdt);
 				nb_hold -= rxq->rx_free_thresh;
 			} else {
-				PMD_RX_LOG(DEBUG, "RX bulk alloc failed "
-						  "port_id=%u queue_id=%u",
-					   rxq->port_id, rxq->queue_id);
-
+				ixgbe_trace_rx_mbuf_alloc_failed(rxq-
 >port_id, rxq->queue_id);
 				rte_eth_devices[rxq->port_id].data->

	rx_mbuf_alloc_failed++;
drivers/net/ixgbe/ixgbe_rxtx.c +0 -1
 				break;

@@ -2307,9 +2260,7 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct
 rte_mbuf **rx_pkts, uint16_t nb_pkts,
 	 * hardware point of view...
 	 */
 	if (!bulk_alloc && nb_hold > rxq->rx_free_thresh) {
-		PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_tail=%u

"

drivers/net/ixgbe/ixgbe_rxtx.c +1 -2
-			   "nb_hold=%u nb_rx=%u",
-			   rxq->port_id, rxq->queue_id, rx_id, nb_hold, nb_rx);
+		ixgbe_trace_rx_post(rxq->port_id, rxq->queue_id, rx_id,

nb_hold,

drivers/net/ixgbe/ixgbe_rxtx.c +1 -0
+nb_rx);
 
 		rte_wmb();
 		IXGBE_PCI_REG_WC_WRITE_RELAXED(rxq->rdt_reg_addr,

prev_id); diff --git a/drivers/net/ixgbe/ixgbe_trace.h b/drivers/net/ixgbe/ixgbe_trace.h new file mode 100644 index 000000000000..f260edd6fe3a

drivers/net/ixgbe/ixgbe_trace.h new file +110 -1
@@ -0,0 +1,110 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+
+#ifndef _IXGBE_TRACE_H_
+#define _IXGBE_TRACE_H_
+
+#include <rte_trace_point.h>
+
+RTE_TRACE_POINT(
+	ixgbe_trace_rx_mbuf_alloc_failed,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(rx_queue_id);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_rx,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id,
+			     uint16_t rx_id, uint32_t staterr, uint16_t len),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(rx_queue_id);
+	rte_trace_point_emit_u16(rx_id);
+	rte_trace_point_emit_u32(staterr);
+	rte_trace_point_emit_u16(len);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_rx_post,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id,
+			     uint16_t rx_id, uint16_t nb_hold, uint16_t nb_rx),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(rx_queue_id);
+	rte_trace_point_emit_u16(rx_id);
+	rte_trace_point_emit_u16(nb_hold);
+	rte_trace_point_emit_u16(nb_rx);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_tx_done,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t nb_to_clean, uint16_t last_desc_cleaned,
+			     uint16_t desc_to_clean),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(nb_to_clean);
+	rte_trace_point_emit_u16(last_desc_cleaned);
+	rte_trace_point_emit_u16(desc_to_clean);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_tx_not_done,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t desc_to_clean),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(desc_to_clean);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_tx,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t pktlen, uint16_t tx_first, uint16_t tx_last),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(pktlen);
+	rte_trace_point_emit_u16(tx_first);
+	rte_trace_point_emit_u16(tx_last);
+);
+
+RTE_TRACE_POINT(
+	ixgbe_trace_tx_full,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t nb_used, uint16_t nb_free),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(nb_used);
+	rte_trace_point_emit_u16(nb_free);
+);
+
+RTE_TRACE_POINT(
+	ixgbe_trace_tx_thresh,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t nb_used, uint16_t nb_free,
+			     uint16_t tx_rs_thresh),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(nb_used);
+	rte_trace_point_emit_u16(nb_free);
+	rte_trace_point_emit_u16(tx_rs_thresh);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_tx_rs,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t txd_id),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(txd_id);
+);
+
+RTE_TRACE_POINT_FP(
+	ixgbe_trace_tx_tail,
+	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
+			     uint16_t tx_tail, uint16_t nb_tx),
+	rte_trace_point_emit_u16(port_id);
+	rte_trace_point_emit_u16(tx_queue_id);
+	rte_trace_point_emit_u16(tx_tail);
+	rte_trace_point_emit_u16(nb_tx);
+);
+
+#endif /* _IXGBE_LOGS_H_ */
--

2.39.1

#5 lore raw
Quoted 57 lines · Long Li
Quoted 57 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 31 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 20 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

Quoted 7 lines

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused approach.

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath logging?

Quoted 6 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Here is an example of using tracepoints instead.
Compile tested for example only.

Quoted 8 lines
Quoted 3 lines

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for application to see.

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard (cc'ed) can discuss this.

Until such consensus reached, I think driver should continue with logging.

And for the logging, I suggest option (i) above, I was hoping more comments but since it is missing I hope this can be discussed in techboard for a conclusion.

#6 lore raw
Quoted 86 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 25 lines · Long Li
Quoted 25 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 20 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 17 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused

approach.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath

logging?

Quoted 20 lines
Quoted 20 lines
Quoted 8 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Quoted 6 lines
Quoted 6 lines

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for

application to see.

Quoted 5 lines

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard (cc'ed) can discuss this.

Quoted 7 lines

Until such consensus reached, I think driver should continue with logging.

And for the logging, I suggest option (i) above, I was hoping more comments but since it is missing I hope this can be discussed in techboard for a conclusion.

Hi Ferruh,

Are you suggesting that MANA should use 'RTE_ETHDEV_DEBUG_RX/TX'?

I'm happy to implement the logging in this way.

Long

#7 lore raw
Quoted 94 lines · Long Li
Quoted 86 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 25 lines · Long Li
Quoted 25 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 20 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 17 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused

approach.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath

logging?

Quoted 20 lines
Quoted 20 lines
Quoted 8 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Quoted 6 lines
Quoted 6 lines

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for

application to see.

Quoted 5 lines

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard (cc'ed) can discuss this.

Quoted 7 lines

Until such consensus reached, I think driver should continue with logging.

And for the logging, I suggest option (i) above, I was hoping more comments but since it is missing I hope this can be discussed in techboard for a conclusion.

Quoted 6 lines

Hi Ferruh,

Are you suggesting that MANA should use 'RTE_ETHDEV_DEBUG_RX/TX'?

I'm happy to implement the logging in this way.

Yes, that looks to me better balance for compile time / runtime config for drive developers.

But it prevents product code / end user to get data path logs, although I believe this is OK I am not sure how useful datapath logs in production code, that is why I am looking for more comment for a decision.

Let's wait for next techboard meeting, in case this is discussed there, before making new implementation.

#8 lore raw
Quoted 126 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Friday, March 3, 2023 5:15 PM
To: Long Li <[email protected]>; Stephen Hemminger <[email protected]>
Cc: [email protected]; Thomas Monjalon <[email protected]>; Andrew Rybchenko <[email protected]>; Jerin Jacob Kollanukkaran <[email protected]>; David Marchand <[email protected]>; [email protected]; Ajay Sharma <[email protected]>; [email protected]; Luca Boccassi <[email protected]>; Qi Z Zhang <[email protected]>; Ajit Khaparde <[email protected]>; Bruce Richardson <[email protected]>; Konstantin Ananyev <[email protected]>; Olivier Matz <[email protected]>; Honnappa Nagarahalli <[email protected]>; [email protected]
Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 96 lines · Long Li
Quoted 88 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 25 lines · Long Li
Quoted 25 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 20 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 17 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused

approach.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath

logging?

Quoted 20 lines
Quoted 20 lines
Quoted 8 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Quoted 7 lines
Quoted 7 lines

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for

application to see.

Quoted 5 lines

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard (cc'ed) can discuss this.

Quoted 7 lines

Until such consensus reached, I think driver should continue with logging.

And for the logging, I suggest option (i) above, I was hoping more comments but since it is missing I hope this can be discussed in techboard for a conclusion.

Quoted 6 lines

Hi Ferruh,

Are you suggesting that MANA should use 'RTE_ETHDEV_DEBUG_RX/TX'?

I'm happy to implement the logging in this way.

Yes, that looks to me better balance for compile time / runtime config for drive developers.

Quoted 6 lines

But it prevents product code / end user to get data path logs, although I believe this is OK I am not sure how useful datapath logs in production code, that is why I am looking for more comment for a decision.

Let's wait for next techboard meeting, in case this is discussed there, before making new implementation.

HI Ferruh,

Have we reached a decision on how to implement data path logs?

Thanks,
Long

#9 lore raw
Quoted 130 lines · Long Li
Quoted 130 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Friday, March 3, 2023 5:15 PM
To: Long Li <[email protected]>; Stephen Hemminger <[email protected]>
Cc: [email protected]; Thomas Monjalon <[email protected]>; Andrew Rybchenko <[email protected]>; Jerin Jacob Kollanukkaran <[email protected]>; David Marchand <[email protected]>; [email protected]; Ajay Sharma <[email protected]>; [email protected]; Luca Boccassi <[email protected]>; Qi Z Zhang <[email protected]>; Ajit Khaparde <[email protected]>; Bruce Richardson <[email protected]>; Konstantin Ananyev <[email protected]>; Olivier Matz <[email protected]>; Honnappa Nagarahalli <[email protected]>; [email protected]
Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 96 lines · Long Li
Quoted 88 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 25 lines · Long Li
Quoted 25 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 20 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 17 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused

approach.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath

logging?

Quoted 20 lines
Quoted 20 lines
Quoted 8 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Quoted 7 lines
Quoted 7 lines

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for

application to see.

Quoted 5 lines

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard (cc'ed) can discuss this.

Quoted 7 lines

Until such consensus reached, I think driver should continue with logging.

And for the logging, I suggest option (i) above, I was hoping more comments but since it is missing I hope this can be discussed in techboard for a conclusion.

Quoted 6 lines

Hi Ferruh,

Are you suggesting that MANA should use 'RTE_ETHDEV_DEBUG_RX/TX'?

I'm happy to implement the logging in this way.

Yes, that looks to me better balance for compile time / runtime config for drive developers.

But it prevents product code / end user to get data path logs, although I believe this is OK I am not sure how useful datapath logs in production code, that is why I am looking for more comment for a decision.

Quoted 7 lines
Quoted 2 lines

Let's wait for next techboard meeting, in case this is discussed there, before making new implementation.

HI Ferruh,

Have we reached a decision on how to implement data path logs?

Hi Long,

I am not aware of a result on the matter, let me continue with this patch to not block it more.

#10 lore raw
Quoted 145 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 8 lines · Long Li
Quoted 8 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Friday, March 3, 2023 5:15 PM
To: Long Li <[email protected]>; Stephen Hemminger <[email protected]>
Cc: [email protected]; Thomas Monjalon

<[email protected]>;

Quoted 28 lines
Quoted 28 lines

Andrew Rybchenko <[email protected]>; Jerin Jacob Kollanukkaran <[email protected]>; David Marchand <[email protected]>; [email protected]; Ajay Sharma <[email protected]>; [email protected]; Luca Boccassi <[email protected]>; Qi Z Zhang <[email protected]>; Ajit Khaparde <[email protected]>; Bruce Richardson <[email protected]>; Konstantin Ananyev <[email protected]>; Olivier Matz <[email protected]>; Honnappa Nagarahalli <[email protected]>; [email protected]
Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 15 lines · Long Li
Quoted 15 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 12 lines · Long Li
Quoted 12 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 6 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 3 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath

logging:

Quoted 84 lines
Quoted 84 lines
Quoted 83 lines
Quoted 69 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused

approach.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath

logging?

Quoted 20 lines
Quoted 20 lines
Quoted 8 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Quoted 7 lines
Quoted 7 lines

Here is an example of using tracepoints instead.
Compile tested for example only.

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for

application to see.

Quoted 5 lines

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard
(cc'ed) can discuss this.

Quoted 7 lines

Until such consensus reached, I think driver should continue with logging.

And for the logging, I suggest option (i) above, I was hoping more comments but since it is missing I hope this can be discussed in techboard for a conclusion.

Hi Ferruh,

Are you suggesting that MANA should use 'RTE_ETHDEV_DEBUG_RX/TX'?

I'm happy to implement the logging in this way.

Quoted 6 lines
Quoted 6 lines

Yes, that looks to me better balance for compile time / runtime config for drive developers.

But it prevents product code / end user to get data path logs, although I believe this is OK I am not sure how useful datapath logs in production code, that is why I am looking for more comment for a

decision.

Quoted 6 lines
Quoted 3 lines

Let's wait for next techboard meeting, in case this is discussed there, before making new implementation.

HI Ferruh,

Quoted 7 lines
Quoted 2 lines

Have we reached a decision on how to implement data path logs?

Hi Long,

I am not aware of a result on the matter, let me continue with this patch to not block it more.

Thank you.

I have submitted several other patches since this patch was submitted. There may be some conflicts.

Please let me know if I should rebase this patch to latest code so it will be straightforward to apply.

Long

#11 lore raw
In reply to Ferruh Yigit
Quoted 67 lines · On Fri, 3 Mar 2023 13:54:23 +0000 Ferruh Yigit <[email protected]> wrote:
Quoted 57 lines · Long Li
Quoted 57 lines

Subject: Re: [PATCH] net/mana: use RTE_LOG_DP for logs on datapath

Quoted 31 lines · On Thu, 23 Feb 2023 10:09:17 -0800 Stephen Hemminger…
Quoted 20 lines · On Thu, 23 Feb 2023 14:07:25 +0000 Ferruh Yigit <[email protected]> wrote:

Overall I am not sure if anyone is interested in driver datapath logs other than driver developers themselves.

For datapath logging I think there are two concerns,
1) It should not eat *any* cycles unless explicitly enabled
2) Capability of enable/disable them because of massive amount of log it can generate

Currently there are two existing approaches for driver datapath logging:
i) Controlled by 'RTE_ETHDEV_DEBUG_RX/TX' compile time flag, when enabled 'rte_log()' is used with Rx/Tx specific log type.
ii) 'RTE_LOG_DP' ', compile time control per logtype via
'RTE_LOG_DP_LEVEL',
when enabled 'rte_log()' is used with PMD logtype.

In (ii), need to re-compile code when you need to increase the log verbosity, and it leaks to production code as mentioned above.

Quoted 7 lines

For (i), developer compiles once enabling debug, later can fine grain log level dynamically. This is more DPDK developer focused approach.

[1]
According above, what do you think to retire 'RTE_LOG_DP', (at least within ethdev datapath), and chose (i) as preferred datapath logging?

Quoted 6 lines

I agree, the current tx/rx logging is a mess.
Each driver is different, each driver has to have something to enable it; and it really isn't useful beyond the driver developer.

Using tracing seems like a much better option. Could we agree on a common set of trace points for drivers and fix all drivers to use the same

thing.

Quoted 10 lines

Probably will cause some upset among driver developers:
"where did my nice printf's go, now I have to learn tracing" but DPDK has a good facility here, lets use it.

My proposal would be:
- agree on common set of trace points
- apply to all drivers
- remove RTE_LOG_DP()
- remove per driver RX/TX options
- side effect, more uses of RTE_LOGTYPE_PMD go away.

Here is an example of using tracepoints instead.
Compile tested for example only.

Quoted 8 lines
Quoted 3 lines

Note: using tracepoints it is possible to keep some of the tracepoints even if fastpath is not enabled. Things like running out of Tx or Mbuf is not something that is perf critical; but would be good for application to see.

Thank you for the example.

I sent another patch converting data path logs (mana) to trace points.

Quoted 8 lines

Hi Long,

Thanks for the effort, you were quick on this while discussion is going on.

Although tracepoint is a good feature, I am not sure if it can fully replace the logging.
I think usage is slightly different, trace is missing custom human readable message, which can be very helpful for end user.

Agree in theory, but looking at existing drivers the datapath log messages are all just printf's of existing variables. Plus the interpretation of what those fields and offsets only has meaning and context for someone familiar with the driver.

Quoted 4 lines

And overall, it is a high level decision to switch logging to trace, it is inconsistent to switch only single driver, perhaps techboard (cc'ed) can discuss this.

Code in existing datapath logging can be easily converted driver by driver. The problem is the lack of testing.