[PATCH 0/3] refactor the nfp log subsystem

[PATCH 0/3] refactor the nfp log subsystem

[Chaoyong He]

Follow the advice from community reviewer, we get rid of the use of
RTE log level type and RTE_LOG_*() macro, and also wrap the rte_log()
with our own log macro.

Chaoyong He (3):
  net/nfp: add the log source file
  net/nfp: get rid of the usage of RTE log level type
  net/nfp: get rid of the usage of RTE log macro

 drivers/net/nfp/flower/nfp_flower.c        | 10 ++---
 drivers/net/nfp/flower/nfp_flower_ctrl.c   |  2 +-
 drivers/net/nfp/meson.build                |  1 +
 drivers/net/nfp/nfp_common.c               |  3 --
 drivers/net/nfp/nfp_cpp_bridge.c           | 48 ++++++++++------------
 drivers/net/nfp/nfp_ethdev.c               |  4 +-
 drivers/net/nfp/nfp_ethdev_vf.c            |  4 +-
 drivers/net/nfp/nfp_logs.c                 | 20 +++++++++
 drivers/net/nfp/nfp_logs.h                 |  8 +++-
 drivers/net/nfp/nfp_rxtx.c                 | 10 ++---
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |  1 +
 11 files changed, 65 insertions(+), 46 deletions(-)
 create mode 100644 drivers/net/nfp/nfp_logs.c

-- 
2.29.3

[PATCH 1/3] net/nfp: add the log source file

[Chaoyong He]

Prepare for adding more log functionality by moving the existing log
functionality to its own file.

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/nfp/meson.build  |  1 +
 drivers/net/nfp/nfp_common.c |  3 ---
 drivers/net/nfp/nfp_logs.c   | 10 ++++++++++
 3 files changed, 11 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/nfp/nfp_logs.c

diff --git a/drivers/net/nfp/meson.build b/drivers/net/nfp/meson.build
index fc8fd906bc..b60eaed2b7 100644
--- a/drivers/net/nfp/meson.build
+++ b/drivers/net/nfp/meson.build
@@ -28,6 +28,7 @@ sources = files(
         'nfp_ethdev_vf.c',
         'nfp_ethdev.c',
         'nfp_flow.c',
+        'nfp_logs.c',
         'nfp_mtr.c',
 )
 
diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
index c9401055d4..21737ff51d 100644
--- a/drivers/net/nfp/nfp_common.c
+++ b/drivers/net/nfp/nfp_common.c
@@ -1562,9 +1562,6 @@ nfp_net_set_vxlan_port(struct nfp_net_hw *hw,
 	return ret;
 }
 
-RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
-RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
-RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
 /*
  * Local variables:
  * c-file-style: "Linux"
diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
new file mode 100644
index 0000000000..48c42fe53f
--- /dev/null
+++ b/drivers/net/nfp/nfp_logs.c
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2023 Corigine, Inc.
+ * All rights reserved.
+ */
+
+#include "nfp_logs.h"
+
+RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
+RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
+RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
-- 
2.29.3

[PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Chaoyong He]

Register the own RX/TX debug log level type, and get rid of the
usage of RTE_LOGTYPE_*. Then we can control the log by a independent
switch.

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/nfp/nfp_logs.c | 10 ++++++++++
 drivers/net/nfp/nfp_logs.h |  8 ++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
index 48c42fe53f..cd58bcee43 100644
--- a/drivers/net/nfp/nfp_logs.c
+++ b/drivers/net/nfp/nfp_logs.c
@@ -5,6 +5,16 @@
 
 #include "nfp_logs.h"
 
+#include <rte_ethdev.h>
+
 RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
 RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
+
+#ifdef RTE_ETHDEV_DEBUG_RX
+RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG)
+#endif
+
+#ifdef RTE_ETHDEV_DEBUG_TX
+RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG)
+#endif
diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
index b7632ee72c..315a57811c 100644
--- a/drivers/net/nfp/nfp_logs.h
+++ b/drivers/net/nfp/nfp_logs.h
@@ -15,15 +15,19 @@ extern int nfp_logtype_init;
 #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
 
 #ifdef RTE_ETHDEV_DEBUG_RX
+extern int nfp_logtype_rx;
 #define PMD_RX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
+	rte_log(RTE_LOG_ ## level, nfp_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 nfp_logtype_tx;
 #define PMD_TX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
+	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
+		"%s(): " fmt "\n", __func__, ## args)
 #else
 #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
 #endif
-- 
2.29.3

[PATCH 3/3] net/nfp: get rid of the usage of RTE log macro

[Chaoyong He]

Replace the usage of RTE_LOG* macro with PMD specific logging.

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/nfp/flower/nfp_flower.c        | 10 ++---
 drivers/net/nfp/flower/nfp_flower_ctrl.c   |  2 +-
 drivers/net/nfp/nfp_cpp_bridge.c           | 48 ++++++++++------------
 drivers/net/nfp/nfp_ethdev.c               |  4 +-
 drivers/net/nfp/nfp_ethdev_vf.c            |  4 +-
 drivers/net/nfp/nfp_rxtx.c                 | 10 ++---
 drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c |  1 +
 7 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/drivers/net/nfp/flower/nfp_flower.c b/drivers/net/nfp/flower/nfp_flower.c
index f1424a010d..42014295ab 100644
--- a/drivers/net/nfp/flower/nfp_flower.c
+++ b/drivers/net/nfp/flower/nfp_flower.c
@@ -330,7 +330,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		 * DPDK just checks the queue is lower than max queues
 		 * enabled. But the queue needs to be configured
 		 */
-		RTE_LOG_DP(ERR, PMD, "RX Bad queue\n");
+		PMD_RX_LOG(ERR, "RX Bad queue");
 		return 0;
 	}
 
@@ -343,7 +343,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 	while (avail + avail_multiplexed < nb_pkts) {
 		rxb = &rxq->rxbufs[rxq->rd_p];
 		if (unlikely(rxb == NULL)) {
-			RTE_LOG_DP(ERR, PMD, "rxb does not exist!\n");
+			PMD_RX_LOG(ERR, "rxb does not exist!");
 			break;
 		}
 
@@ -363,8 +363,8 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		 */
 		new_mb = rte_pktmbuf_alloc(rxq->mem_pool);
 		if (unlikely(new_mb == NULL)) {
-			RTE_LOG_DP(DEBUG, PMD,
-			"RX mbuf alloc failed port_id=%u queue_id=%d\n",
+			PMD_RX_LOG(DEBUG,
+			"RX mbuf alloc failed port_id=%u queue_id=%d",
 				rxq->port_id, rxq->qidx);
 			nfp_net_mbuf_alloc_failed(rxq);
 			break;
@@ -391,7 +391,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 			 * responsibility of avoiding it. But we have
 			 * to give some info about the error
 			 */
-			RTE_LOG_DP(ERR, PMD,
+			PMD_RX_LOG(ERR,
 				"mbuf overflow likely due to the RX offset.\n"
 				"\t\tYour mbuf size should have extra space for"
 				" RX offset=%u bytes.\n"
diff --git a/drivers/net/nfp/flower/nfp_flower_ctrl.c b/drivers/net/nfp/flower/nfp_flower_ctrl.c
index 03a2e2e622..6e7545bc39 100644
--- a/drivers/net/nfp/flower/nfp_flower_ctrl.c
+++ b/drivers/net/nfp/flower/nfp_flower_ctrl.c
@@ -91,7 +91,7 @@ nfp_flower_ctrl_vnic_recv(void *rx_queue,
 			 * responsibility of avoiding it. But we have
 			 * to give some info about the error
 			 */
-			RTE_LOG_DP(ERR, PMD,
+			PMD_RX_LOG(ERR,
 				"mbuf overflow likely due to the RX offset.\n"
 				"\t\tYour mbuf size should have extra space for"
 				" RX offset=%u bytes.\n"
diff --git a/drivers/net/nfp/nfp_cpp_bridge.c b/drivers/net/nfp/nfp_cpp_bridge.c
index 4aa36eb581..8e29cfb6d3 100644
--- a/drivers/net/nfp/nfp_cpp_bridge.c
+++ b/drivers/net/nfp/nfp_cpp_bridge.c
@@ -162,14 +162,14 @@ nfp_cpp_bridge_serve_write(int sockfd, struct nfp_cpp *cpp)
 		area = nfp_cpp_area_alloc_with_name(cpp, cpp_id, "nfp.cdev",
 						    nfp_offset, curlen);
 		if (area == NULL) {
-			RTE_LOG(ERR, PMD, "%s: area alloc fail\n", __func__);
+			PMD_CPP_LOG(ERR, "area alloc fail");
 			return -EIO;
 		}
 
 		/* mapping the target */
 		err = nfp_cpp_area_acquire(area);
 		if (err < 0) {
-			RTE_LOG(ERR, PMD, "area acquire failed\n");
+			PMD_CPP_LOG(ERR, "area acquire failed");
 			nfp_cpp_area_free(area);
 			return -EIO;
 		}
@@ -183,16 +183,16 @@ nfp_cpp_bridge_serve_write(int sockfd, struct nfp_cpp *cpp)
 					   len, count);
 			err = recv(sockfd, tmpbuf, len, MSG_WAITALL);
 			if (err != (int)len) {
-				RTE_LOG(ERR, PMD,
-					"%s: error when receiving, %d of %zu\n",
-					__func__, err, count);
+				PMD_CPP_LOG(ERR,
+					"error when receiving, %d of %zu",
+					err, count);
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
 			}
 			err = nfp_cpp_area_write(area, pos, tmpbuf, len);
 			if (err < 0) {
-				RTE_LOG(ERR, PMD, "nfp_cpp_area_write error\n");
+				PMD_CPP_LOG(ERR, "nfp_cpp_area_write error");
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
@@ -262,13 +262,13 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 		area = nfp_cpp_area_alloc_with_name(cpp, cpp_id, "nfp.cdev",
 						    nfp_offset, curlen);
 		if (area == NULL) {
-			RTE_LOG(ERR, PMD, "%s: area alloc failed\n", __func__);
+			PMD_CPP_LOG(ERR, "area alloc failed");
 			return -EIO;
 		}
 
 		err = nfp_cpp_area_acquire(area);
 		if (err < 0) {
-			RTE_LOG(ERR, PMD, "area acquire failed\n");
+			PMD_CPP_LOG(ERR, "area acquire failed");
 			nfp_cpp_area_free(area);
 			return -EIO;
 		}
@@ -280,7 +280,7 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 
 			err = nfp_cpp_area_read(area, pos, tmpbuf, len);
 			if (err < 0) {
-				RTE_LOG(ERR, PMD, "nfp_cpp_area_read error\n");
+				PMD_CPP_LOG(ERR, "nfp_cpp_area_read error");
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
@@ -290,9 +290,9 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 
 			err = send(sockfd, tmpbuf, len, 0);
 			if (err != (int)len) {
-				RTE_LOG(ERR, PMD,
-					"%s: error when sending: %d of %zu\n",
-					__func__, err, count);
+				PMD_CPP_LOG(ERR,
+					"error when sending: %d of %zu",
+					err, count);
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
@@ -325,19 +325,19 @@ nfp_cpp_bridge_serve_ioctl(int sockfd, struct nfp_cpp *cpp)
 	/* Reading now the IOCTL command */
 	err = recv(sockfd, &cmd, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: read error from socket\n", __func__);
+		PMD_CPP_LOG(ERR, "read error from socket");
 		return -EIO;
 	}
 
 	/* Only supporting NFP_IOCTL_CPP_IDENTIFICATION */
 	if (cmd != NFP_IOCTL_CPP_IDENTIFICATION) {
-		RTE_LOG(ERR, PMD, "%s: unknown cmd %d\n", __func__, cmd);
+		PMD_CPP_LOG(ERR, "unknown cmd %d", cmd);
 		return -EINVAL;
 	}
 
 	err = recv(sockfd, &ident_size, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: read error from socket\n", __func__);
+		PMD_CPP_LOG(ERR, "read error from socket");
 		return -EIO;
 	}
 
@@ -347,7 +347,7 @@ nfp_cpp_bridge_serve_ioctl(int sockfd, struct nfp_cpp *cpp)
 
 	err = send(sockfd, &tmp, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: error writing to socket\n", __func__);
+		PMD_CPP_LOG(ERR, "error writing to socket");
 		return -EIO;
 	}
 
@@ -357,7 +357,7 @@ nfp_cpp_bridge_serve_ioctl(int sockfd, struct nfp_cpp *cpp)
 
 	err = send(sockfd, &tmp, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: error writing to socket\n", __func__);
+		PMD_CPP_LOG(ERR, "error writing to socket");
 		return -EIO;
 	}
 
@@ -384,8 +384,7 @@ nfp_cpp_bridge_service_func(void *args)
 	unlink("/tmp/nfp_cpp");
 	sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (sockfd < 0) {
-		RTE_LOG(ERR, PMD, "%s: socket creation error. Service failed\n",
-			__func__);
+		PMD_CPP_LOG(ERR, "socket creation error. Service failed");
 		return -EIO;
 	}
 
@@ -399,16 +398,14 @@ nfp_cpp_bridge_service_func(void *args)
 	ret = bind(sockfd, (const struct sockaddr *)&address,
 		   sizeof(struct sockaddr));
 	if (ret < 0) {
-		RTE_LOG(ERR, PMD, "%s: bind error (%d). Service failed\n",
-				  __func__, errno);
+		PMD_CPP_LOG(ERR, "bind error (%d). Service failed", errno);
 		close(sockfd);
 		return ret;
 	}
 
 	ret = listen(sockfd, 20);
 	if (ret < 0) {
-		RTE_LOG(ERR, PMD, "%s: listen error(%d). Service failed\n",
-				  __func__, errno);
+		PMD_CPP_LOG(ERR, "listen error(%d). Service failed", errno);
 		close(sockfd);
 		return ret;
 	}
@@ -421,9 +418,8 @@ nfp_cpp_bridge_service_func(void *args)
 			if (errno == EAGAIN || errno == EWOULDBLOCK)
 				continue;
 
-			RTE_LOG(ERR, PMD, "%s: accept call error (%d)\n",
-					  __func__, errno);
-			RTE_LOG(ERR, PMD, "%s: service failed\n", __func__);
+			PMD_CPP_LOG(ERR, "accept call error (%d)", errno);
+			PMD_CPP_LOG(ERR, "service failed");
 			close(sockfd);
 			return -EIO;
 		}
diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index f05c50ac88..139f3090aa 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -519,8 +519,8 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
 
 	/* NFP can not handle DMA addresses requiring more than 40 bits */
 	if (rte_mem_check_dma_mask(40)) {
-		RTE_LOG(ERR, PMD,
-			"device %s can not be used: restricted dma mask to 40 bits!\n",
+		PMD_INIT_LOG(ERR,
+			"device %s can not be used: restricted dma mask to 40 bits!",
 			pci_dev->device.name);
 		return -ENODEV;
 	}
diff --git a/drivers/net/nfp/nfp_ethdev_vf.c b/drivers/net/nfp/nfp_ethdev_vf.c
index 07a2e17ef8..cbe5c5c5c8 100644
--- a/drivers/net/nfp/nfp_ethdev_vf.c
+++ b/drivers/net/nfp/nfp_ethdev_vf.c
@@ -293,8 +293,8 @@ nfp_netvf_init(struct rte_eth_dev *eth_dev)
 
 	/* NFP can not handle DMA addresses requiring more than 40 bits */
 	if (rte_mem_check_dma_mask(40)) {
-		RTE_LOG(ERR, PMD,
-			"device %s can not be used: restricted dma mask to 40 bits!\n",
+		PMD_INIT_LOG(ERR,
+			"device %s can not be used: restricted dma mask to 40 bits!",
 			pci_dev->device.name);
 		return -ENODEV;
 	}
diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
index 79a66b6e44..09cc24741a 100644
--- a/drivers/net/nfp/nfp_rxtx.c
+++ b/drivers/net/nfp/nfp_rxtx.c
@@ -353,7 +353,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		 * DPDK just checks the queue is lower than max queues
 		 * enabled. But the queue needs to be configured
 		 */
-		RTE_LOG_DP(ERR, PMD, "RX Bad queue\n");
+		PMD_RX_LOG(ERR, "RX Bad queue");
 		return avail;
 	}
 
@@ -363,7 +363,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 	while (avail < nb_pkts) {
 		rxb = &rxq->rxbufs[rxq->rd_p];
 		if (unlikely(rxb == NULL)) {
-			RTE_LOG_DP(ERR, PMD, "rxb does not exist!\n");
+			PMD_RX_LOG(ERR, "rxb does not exist!");
 			break;
 		}
 
@@ -383,8 +383,8 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		 */
 		new_mb = rte_pktmbuf_alloc(rxq->mem_pool);
 		if (unlikely(new_mb == NULL)) {
-			RTE_LOG_DP(DEBUG, PMD,
-			"RX mbuf alloc failed port_id=%u queue_id=%u\n",
+			PMD_RX_LOG(DEBUG,
+			"RX mbuf alloc failed port_id=%u queue_id=%u",
 				rxq->port_id, (unsigned int)rxq->qidx);
 			nfp_net_mbuf_alloc_failed(rxq);
 			break;
@@ -412,7 +412,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 			 * responsibility of avoiding it. But we have
 			 * to give some info about the error
 			 */
-			RTE_LOG_DP(ERR, PMD,
+			PMD_RX_LOG(ERR,
 				"mbuf overflow likely due to the RX offset.\n"
 				"\t\tYour mbuf size should have extra space for"
 				" RX offset=%u bytes.\n"
diff --git a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
index d8d1293166..6029bd6c3a 100644
--- a/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
+++ b/drivers/net/nfp/nfpcore/nfp_cpp_pcie_ops.c
@@ -36,6 +36,7 @@
 #include "nfp_logs.h"
 #include "nfp_target.h"
 #include "nfp6000/nfp6000.h"
+#include "../nfp_logs.h"
 
 #define NFP_PCIE_BAR(_pf)	(0x30000 + ((_pf) & 7) * 0xc0)
 
-- 
2.29.3

Re: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Ferruh Yigit]

On 2/17/2023 2:45 AM, Chaoyong He wrote:
> Register the own RX/TX debug log level type, and get rid of the
> usage of RTE_LOGTYPE_*. Then we can control the log by a independent
> switch.
> 
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> ---
>  drivers/net/nfp/nfp_logs.c | 10 ++++++++++
>  drivers/net/nfp/nfp_logs.h |  8 ++++++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
> index 48c42fe53f..cd58bcee43 100644
> --- a/drivers/net/nfp/nfp_logs.c
> +++ b/drivers/net/nfp/nfp_logs.c
> @@ -5,6 +5,16 @@
>  
>  #include "nfp_logs.h"
>  
> +#include <rte_ethdev.h>
> +
>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
> +
> +#ifdef RTE_ETHDEV_DEBUG_RX
> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG)
> +#endif
> +
> +#ifdef RTE_ETHDEV_DEBUG_TX
> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG)
> +#endif
> diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
> index b7632ee72c..315a57811c 100644
> --- a/drivers/net/nfp/nfp_logs.h
> +++ b/drivers/net/nfp/nfp_logs.h
> @@ -15,15 +15,19 @@ extern int nfp_logtype_init;
>  #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
>  
>  #ifdef RTE_ETHDEV_DEBUG_RX
> +extern int nfp_logtype_rx;
>  #define PMD_RX_LOG(level, fmt, args...) \
> -	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
> +	rte_log(RTE_LOG_ ## level, nfp_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 nfp_logtype_tx;
>  #define PMD_TX_LOG(level, fmt, args...) \
> -	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
> +	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
> +		"%s(): " fmt "\n", __func__, ## args)
>  #else
>  #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>  #endif

Intention is to replace 'RTE_LOG_DP' with 'PMD_RX_LOG'/'PMD_TX_LOG', but
these are not exactly same (although difference is minor).

When 'RTE_ETHDEV_DEBUG_RX' is set, ethdev layer also adds some
additional load, although I believe that will small comparing to logging
in driver.
If 'RTE_LOG_DP' used, the ethdev layer cost can be removed.

With 'RTE_LOG_DP', log level more verbose than requested won't cause any
performance impact. Like if ERR level requested, INFO, DEBUG etc logs
will be compiled out and won't cause any performance impact.
But with 'RTE_ETHDEV_DEBUG_RX', even log level only request ERR, all
logging will add cost of at least an if branch (checking log level).


For many cases I am not sure these differences matters, and already many
drivers directly uses 'RTE_ETHDEV_DEBUG_RX' as done here. So you may
prefer to keep as it is.

But if there is a desire for this fine grain approach, it is possible to
add a version of 'RTE_LOG_DP' macro that accepts dynamic log type
(instead of static RTE_LOGTYPE_# type),
what do you think?

RE: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Chaoyong He]

> On 2/17/2023 2:45 AM, Chaoyong He wrote:
> > Register the own RX/TX debug log level type, and get rid of the usage
> > of RTE_LOGTYPE_*. Then we can control the log by a independent switch.
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > ---
> >  drivers/net/nfp/nfp_logs.c | 10 ++++++++++
> > drivers/net/nfp/nfp_logs.h |  8 ++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
> > index 48c42fe53f..cd58bcee43 100644
> > --- a/drivers/net/nfp/nfp_logs.c
> > +++ b/drivers/net/nfp/nfp_logs.c
> > @@ -5,6 +5,16 @@
> >
> >  #include "nfp_logs.h"
> >
> > +#include <rte_ethdev.h>
> > +
> >  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
> > RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
> > RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
> > +
> > +#ifdef RTE_ETHDEV_DEBUG_RX
> > +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG) #endif
> > +
> > +#ifdef RTE_ETHDEV_DEBUG_TX
> > +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG) #endif
> > diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
> > index b7632ee72c..315a57811c 100644
> > --- a/drivers/net/nfp/nfp_logs.h
> > +++ b/drivers/net/nfp/nfp_logs.h
> > @@ -15,15 +15,19 @@ extern int nfp_logtype_init;  #define
> > PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> >
> >  #ifdef RTE_ETHDEV_DEBUG_RX
> > +extern int nfp_logtype_rx;
> >  #define PMD_RX_LOG(level, fmt, args...) \
> > -	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
> > +	rte_log(RTE_LOG_ ## level, nfp_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 nfp_logtype_tx;
> >  #define PMD_TX_LOG(level, fmt, args...) \
> > -	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
> > +	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
> > +		"%s(): " fmt "\n", __func__, ## args)
> >  #else
> >  #define PMD_TX_LOG(level, fmt, args...) do { } while (0)  #endif
> 
> Intention is to replace 'RTE_LOG_DP' with 'PMD_RX_LOG'/'PMD_TX_LOG',
> but these are not exactly same (although difference is minor).
> 
> When 'RTE_ETHDEV_DEBUG_RX' is set, ethdev layer also adds some
> additional load, although I believe that will small comparing to logging in
> driver.
> If 'RTE_LOG_DP' used, the ethdev layer cost can be removed.
> 
> With 'RTE_LOG_DP', log level more verbose than requested won't cause any
> performance impact. Like if ERR level requested, INFO, DEBUG etc logs will
> be compiled out and won't cause any performance impact.
> But with 'RTE_ETHDEV_DEBUG_RX', even log level only request ERR, all
> logging will add cost of at least an if branch (checking log level).
> 
> 
> For many cases I am not sure these differences matters, and already many
> drivers directly uses 'RTE_ETHDEV_DEBUG_RX' as done here. So you may
> prefer to keep as it is.
> 
> But if there is a desire for this fine grain approach, it is possible to add a
> version of 'RTE_LOG_DP' macro that accepts dynamic log type (instead of
> static RTE_LOGTYPE_# type), what do you think?
> 

Thanks for the suggestion.
For now, we prefer to keep as it is.
If we does need the more refined design in the future, we would follow your advice here, thanks again.

Re: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Ferruh Yigit]

On 2/20/2023 1:36 AM, Chaoyong He wrote:
>> On 2/17/2023 2:45 AM, Chaoyong He wrote:
>>> Register the own RX/TX debug log level type, and get rid of the usage
>>> of RTE_LOGTYPE_*. Then we can control the log by a independent switch.
>>>
>>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>> ---
>>>  drivers/net/nfp/nfp_logs.c | 10 ++++++++++
>>> drivers/net/nfp/nfp_logs.h |  8 ++++++--
>>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
>>> index 48c42fe53f..cd58bcee43 100644
>>> --- a/drivers/net/nfp/nfp_logs.c
>>> +++ b/drivers/net/nfp/nfp_logs.c
>>> @@ -5,6 +5,16 @@
>>>
>>>  #include "nfp_logs.h"
>>>
>>> +#include <rte_ethdev.h>
>>> +
>>>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
>>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
>>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
>>> +
>>> +#ifdef RTE_ETHDEV_DEBUG_RX
>>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG) #endif
>>> +
>>> +#ifdef RTE_ETHDEV_DEBUG_TX
>>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG) #endif
>>> diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
>>> index b7632ee72c..315a57811c 100644
>>> --- a/drivers/net/nfp/nfp_logs.h
>>> +++ b/drivers/net/nfp/nfp_logs.h
>>> @@ -15,15 +15,19 @@ extern int nfp_logtype_init;  #define
>>> PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
>>>
>>>  #ifdef RTE_ETHDEV_DEBUG_RX
>>> +extern int nfp_logtype_rx;
>>>  #define PMD_RX_LOG(level, fmt, args...) \
>>> -	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
>>> +	rte_log(RTE_LOG_ ## level, nfp_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 nfp_logtype_tx;
>>>  #define PMD_TX_LOG(level, fmt, args...) \
>>> -	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
>>> +	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
>>> +		"%s(): " fmt "\n", __func__, ## args)
>>>  #else
>>>  #define PMD_TX_LOG(level, fmt, args...) do { } while (0)  #endif
>>
>> Intention is to replace 'RTE_LOG_DP' with 'PMD_RX_LOG'/'PMD_TX_LOG',
>> but these are not exactly same (although difference is minor).
>>
>> When 'RTE_ETHDEV_DEBUG_RX' is set, ethdev layer also adds some
>> additional load, although I believe that will small comparing to logging in
>> driver.
>> If 'RTE_LOG_DP' used, the ethdev layer cost can be removed.
>>
>> With 'RTE_LOG_DP', log level more verbose than requested won't cause any
>> performance impact. Like if ERR level requested, INFO, DEBUG etc logs will
>> be compiled out and won't cause any performance impact.
>> But with 'RTE_ETHDEV_DEBUG_RX', even log level only request ERR, all
>> logging will add cost of at least an if branch (checking log level).
>>
>>
>> For many cases I am not sure these differences matters, and already many
>> drivers directly uses 'RTE_ETHDEV_DEBUG_RX' as done here. So you may
>> prefer to keep as it is.
>>
>> But if there is a desire for this fine grain approach, it is possible to add a
>> version of 'RTE_LOG_DP' macro that accepts dynamic log type (instead of
>> static RTE_LOGTYPE_# type), what do you think?
>>
> 
> Thanks for the suggestion.
> For now, we prefer to keep as it is.
> If we does need the more refined design in the future, we would follow your advice here, thanks again.

ack, I just wanted to double check. I will proceed as it is.

Re: [PATCH 0/3] refactor the nfp log subsystem

[Ferruh Yigit]

On 2/17/2023 2:45 AM, Chaoyong He wrote:
> Follow the advice from community reviewer, we get rid of the use of
> RTE log level type and RTE_LOG_*() macro, and also wrap the rte_log()
> with our own log macro.
> 
> Chaoyong He (3):
>   net/nfp: add the log source file
>   net/nfp: get rid of the usage of RTE log level type
>   net/nfp: get rid of the usage of RTE log macro

Series applied to dpdk-next-net/main, thanks.

Re: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Stephen Hemminger]

On Mon, 20 Feb 2023 10:09:51 +0000
Ferruh Yigit <ferruh.yigit@amd.com> wrote:

> On 2/20/2023 1:36 AM, Chaoyong He wrote:
> >> On 2/17/2023 2:45 AM, Chaoyong He wrote:  
> >>> Register the own RX/TX debug log level type, and get rid of the usage
> >>> of RTE_LOGTYPE_*. Then we can control the log by a independent switch.
> >>>
> >>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> >>> ---
> >>>  drivers/net/nfp/nfp_logs.c | 10 ++++++++++
> >>> drivers/net/nfp/nfp_logs.h |  8 ++++++--
> >>>  2 files changed, 16 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
> >>> index 48c42fe53f..cd58bcee43 100644
> >>> --- a/drivers/net/nfp/nfp_logs.c
> >>> +++ b/drivers/net/nfp/nfp_logs.c
> >>> @@ -5,6 +5,16 @@
> >>>
> >>>  #include "nfp_logs.h"
> >>>
> >>> +#include <rte_ethdev.h>
> >>> +
> >>>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
> >>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
> >>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
> >>> +
> >>> +#ifdef RTE_ETHDEV_DEBUG_RX
> >>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG) #endif
> >>> +
> >>> +#ifdef RTE_ETHDEV_DEBUG_TX
> >>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG) #endif
> >>> diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
> >>> index b7632ee72c..315a57811c 100644
> >>> --- a/drivers/net/nfp/nfp_logs.h
> >>> +++ b/drivers/net/nfp/nfp_logs.h
> >>> @@ -15,15 +15,19 @@ extern int nfp_logtype_init;  #define
> >>> PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
> >>>
> >>>  #ifdef RTE_ETHDEV_DEBUG_RX
> >>> +extern int nfp_logtype_rx;
> >>>  #define PMD_RX_LOG(level, fmt, args...) \
> >>> -	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
> >>> +	rte_log(RTE_LOG_ ## level, nfp_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 nfp_logtype_tx;
> >>>  #define PMD_TX_LOG(level, fmt, args...) \
> >>> -	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
> >>> +	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
> >>> +		"%s(): " fmt "\n", __func__, ## args)
> >>>  #else
> >>>  #define PMD_TX_LOG(level, fmt, args...) do { } while (0)  #endif  
> >>
> >> Intention is to replace 'RTE_LOG_DP' with 'PMD_RX_LOG'/'PMD_TX_LOG',
> >> but these are not exactly same (although difference is minor).
> >>
> >> When 'RTE_ETHDEV_DEBUG_RX' is set, ethdev layer also adds some
> >> additional load, although I believe that will small comparing to logging in
> >> driver.
> >> If 'RTE_LOG_DP' used, the ethdev layer cost can be removed.
> >>
> >> With 'RTE_LOG_DP', log level more verbose than requested won't cause any
> >> performance impact. Like if ERR level requested, INFO, DEBUG etc logs will
> >> be compiled out and won't cause any performance impact.
> >> But with 'RTE_ETHDEV_DEBUG_RX', even log level only request ERR, all
> >> logging will add cost of at least an if branch (checking log level).
> >>
> >>
> >> For many cases I am not sure these differences matters, and already many
> >> drivers directly uses 'RTE_ETHDEV_DEBUG_RX' as done here. So you may
> >> prefer to keep as it is.
> >>
> >> But if there is a desire for this fine grain approach, it is possible to add a
> >> version of 'RTE_LOG_DP' macro that accepts dynamic log type (instead of
> >> static RTE_LOGTYPE_# type), what do you think?
> >>  
> > 
> > Thanks for the suggestion.
> > For now, we prefer to keep as it is.
> > If we does need the more refined design in the future, we would follow your advice here, thanks again.  
> 
> ack, I just wanted to double check. I will proceed as it is.

As part of my patch series (work in progress) to get rid of RTE_LOGTYPE_PMD
needed to add a helper now for RTE_DP_LOG like this.

diff --git a/lib/eal/include/rte_log.h b/lib/eal/include/rte_log.h
index 6d2b0856a565..f377bc6db79b 100644
--- a/lib/eal/include/rte_log.h
+++ b/lib/eal/include/rte_log.h
@@ -336,6 +336,37 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
 	 rte_log(RTE_LOG_ ## l,					\
 		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
 
+/**
+ * Generates a log message for data path.
+ *
+ * Similar to rte_log(), except that it is an inline function that
+ * can be eliminated at compilation time if RTE_LOG_DP_LEVEL configuration
+ * option is lower than the log level argument.
+ *
+ * @param level
+ *   Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
+ * @param logtype
+ *   The log type, for example, RTE_LOGTYPE_EAL.
+ * @param format
+ *   The format string, as in printf(3), followed by the variable arguments
+ *   required by the format.
+ * @return
+ *   - 0: Success.
+ *   - Negative on error.
+ */
+static inline __rte_format_printf(3, 4)
+void rte_log_dp(uint32_t level, uint32_t logtype, const char *format, ...)
+
+{
+	va_list ap;
+
+	if (level <= RTE_LOG_DP_LEVEL) {
+		va_start(ap, format);
+		rte_vlog(level, logtype, format, ap);
+		va_end(ap);
+	}
+}
+
 /**
  * Generates a log message for data path.
  *
@@ -357,10 +388,8 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
  *   - Negative on error.
  */
 #define RTE_LOG_DP(l, t, ...)					\
-	(void)((RTE_LOG_ ## l <= RTE_LOG_DP_LEVEL) ?		\
-	 rte_log(RTE_LOG_ ## l,					\
-		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) :	\
-	 0)
+	rte_log_dp(RTE_LOG_ ## l,				\
+		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
 
 #define RTE_LOG_REGISTER_IMPL(type, name, level)			    \
 int type;								    \

The NFP part is:
diff --git a/drivers/net/nfp/flower/nfp_flower.c b/drivers/net/nfp/flower/nfp_flower.c
index f1424a010dde..15401c0d5ba6 100644
--- a/drivers/net/nfp/flower/nfp_flower.c
+++ b/drivers/net/nfp/flower/nfp_flower.c
@@ -330,7 +330,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		 * DPDK just checks the queue is lower than max queues
 		 * enabled. But the queue needs to be configured
 		 */
-		RTE_LOG_DP(ERR, PMD, "RX Bad queue\n");
+		PMD_DP_LOG(ERR, "RX Bad queue");
 		return 0;
 	}
 
@@ -343,7 +343,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 	while (avail + avail_multiplexed < nb_pkts) {
 		rxb = &rxq->rxbufs[rxq->rd_p];
 		if (unlikely(rxb == NULL)) {
-			RTE_LOG_DP(ERR, PMD, "rxb does not exist!\n");
+			PMD_DP_LOG(ERR, "rxb does not exist!");
 			break;
 		}
 
@@ -363,9 +363,9 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		 */
 		new_mb = rte_pktmbuf_alloc(rxq->mem_pool);
 		if (unlikely(new_mb == NULL)) {
-			RTE_LOG_DP(DEBUG, PMD,
-			"RX mbuf alloc failed port_id=%u queue_id=%d\n",
-				rxq->port_id, rxq->qidx);
+			PMD_DP_LOG(DEBUG,
+				   "RX mbuf alloc failed port_id=%u queue_id=%d\n",
+				   rxq->port_id, rxq->qidx);
 			nfp_net_mbuf_alloc_failed(rxq);
 			break;
 		}
@@ -378,7 +378,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		rxb->mbuf = new_mb;
 
 		PMD_RX_LOG(DEBUG, "Packet len: %u, mbuf_size: %u",
-				rxds->rxd.data_len, rxq->mbuf_size);
+			   rxds->rxd.data_len, rxq->mbuf_size);
 
 		/* Size of this segment */
 		mb->data_len = rxds->rxd.data_len - NFP_DESC_META_LEN(rxds);
@@ -391,15 +391,15 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 			 * responsibility of avoiding it. But we have
 			 * to give some info about the error
 			 */
-			RTE_LOG_DP(ERR, PMD,
-				"mbuf overflow likely due to the RX offset.\n"
-				"\t\tYour mbuf size should have extra space for"
-				" RX offset=%u bytes.\n"
-				"\t\tCurrently you just have %u bytes available"
-				" but the received packet is %u bytes long",
-				hw->rx_offset,
-				rxq->mbuf_size - hw->rx_offset,
-				mb->data_len);
+			PMD_DP_LOG(ERR,
+				   "mbuf overflow likely due to the RX offset.\n"
+				   "\t\tYour mbuf size should have extra space for"
+				   " RX offset=%u bytes.\n"
+				   "\t\tCurrently you just have %u bytes available"
+				   " but the received packet is %u bytes long",
+				   hw->rx_offset,
+				   rxq->mbuf_size - hw->rx_offset,
+				   mb->data_len);
 			rte_pktmbuf_free(mb);
 			break;
 		}
@@ -420,14 +420,14 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		/* Checking the RSS flag */
 		nfp_flower_parse_metadata(rxq, rxds, mb, &meta_portid);
 		PMD_RX_LOG(DEBUG, "Received from port %u type %u",
-				NFP_FLOWER_CMSG_PORT_VNIC(meta_portid),
-				NFP_FLOWER_CMSG_PORT_VNIC_TYPE(meta_portid));
+			   NFP_FLOWER_CMSG_PORT_VNIC(meta_portid),
+			   NFP_FLOWER_CMSG_PORT_VNIC_TYPE(meta_portid));
 
 		/* Checking the checksum flag */
 		nfp_net_rx_cksum(rxq, rxds, mb);
 
 		if ((rxds->rxd.flags & PCIE_DESC_RX_VLAN) &&
-				(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)) {
+		    (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)) {
 			mb->vlan_tci = rte_cpu_to_le_32(rxds->rxd.vlan);
 			mb->ol_flags |= RTE_MBUF_F_RX_VLAN | RTE_MBUF_F_RX_VLAN_STRIPPED;
 		}
@@ -439,7 +439,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 			avail_multiplexed++;
 		} else if (repr != NULL) {
 			PMD_RX_LOG(ERR, "[%u] No ring available for repr_port %s\n",
-					hw->idx, repr->name);
+				   hw->idx, repr->name);
 			PMD_RX_LOG(DEBUG, "Adding the mbuf to the mbuf array passed by the app");
 			rx_pkts[avail++] = mb;
 		} else {
@@ -465,7 +465,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 		return nb_hold;
 
 	PMD_RX_LOG(DEBUG, "RX port_id=%u queue_id=%d, %d packets received",
-			rxq->port_id, rxq->qidx, nb_hold);
+		   rxq->port_id, rxq->qidx, nb_hold);
 
 	nb_hold += rxq->nb_rx_hold;
 
@@ -476,7 +476,7 @@ nfp_flower_pf_recv_pkts(void *rx_queue,
 	rte_wmb();
 	if (nb_hold > rxq->rx_free_thresh) {
 		PMD_RX_LOG(DEBUG, "port=%u queue=%d nb_hold=%u avail=%d",
-				rxq->port_id, rxq->qidx, nb_hold, avail);
+			   rxq->port_id, rxq->qidx, nb_hold, avail);
 		nfp_qcp_ptr_add(rxq->qcp_fl, NFP_QCP_WRITE_PTR, nb_hold);
 		nb_hold = 0;
 	}
diff --git a/drivers/net/nfp/flower/nfp_flower_ctrl.c b/drivers/net/nfp/flower/nfp_flower_ctrl.c
index 03a2e2e6222f..3de3bf1ca2e9 100644
--- a/drivers/net/nfp/flower/nfp_flower_ctrl.c
+++ b/drivers/net/nfp/flower/nfp_flower_ctrl.c
@@ -91,15 +91,15 @@ nfp_flower_ctrl_vnic_recv(void *rx_queue,
 			 * responsibility of avoiding it. But we have
 			 * to give some info about the error
 			 */
-			RTE_LOG_DP(ERR, PMD,
-				"mbuf overflow likely due to the RX offset.\n"
-				"\t\tYour mbuf size should have extra space for"
-				" RX offset=%u bytes.\n"
-				"\t\tCurrently you just have %u bytes available"
-				" but the received packet is %u bytes long",
-				hw->rx_offset,
-				rxq->mbuf_size - hw->rx_offset,
-				mb->data_len);
+			PMD_DP_LOG(ERR,
+				   "mbuf overflow likely due to the RX offset.\n"
+				   "\t\tYour mbuf size should have extra space for"
+				   " RX offset=%u bytes.\n"
+				   "\t\tCurrently you just have %u bytes available"
+				   " but the received packet is %u bytes long",
+				   hw->rx_offset,
+				   rxq->mbuf_size - hw->rx_offset,
+				   mb->data_len);
 			rte_pktmbuf_free(mb);
 			break;
 		}
diff --git a/drivers/net/nfp/nfp_cpp_bridge.c b/drivers/net/nfp/nfp_cpp_bridge.c
index 4aa36eb5814f..3c67a8fd419c 100644
--- a/drivers/net/nfp/nfp_cpp_bridge.c
+++ b/drivers/net/nfp/nfp_cpp_bridge.c
@@ -126,7 +126,7 @@ nfp_cpp_bridge_serve_write(int sockfd, struct nfp_cpp *cpp)
 	size_t count, curlen;
 	int err = 0;
 
-	PMD_CPP_LOG(DEBUG, "%s: offset size %zu, count_size: %zu\n", __func__,
+	PMD_CPP_LOG(DEBUG, "offset size %zu, count_size: %zu",
 		sizeof(off_t), sizeof(size_t));
 
 	/* Reading the count param */
@@ -145,10 +145,9 @@ nfp_cpp_bridge_serve_write(int sockfd, struct nfp_cpp *cpp)
 	cpp_id = (offset >> 40) << 8;
 	nfp_offset = offset & ((1ull << 40) - 1);
 
-	PMD_CPP_LOG(DEBUG, "%s: count %zu and offset %jd\n", __func__, count,
-		offset);
-	PMD_CPP_LOG(DEBUG, "%s: cpp_id %08x and nfp_offset %jd\n", __func__,
-		cpp_id, nfp_offset);
+	PMD_CPP_LOG(DEBUG, "count %zu and offset %jd", count, offset);
+	PMD_CPP_LOG(DEBUG, "cpp_id %08x and nfp_offset %jd",
+		    cpp_id, nfp_offset);
 
 	/* Adjust length if not aligned */
 	if (((nfp_offset + (off_t)count - 1) & ~(NFP_CPP_MEMIO_BOUNDARY - 1)) !=
@@ -162,14 +161,14 @@ nfp_cpp_bridge_serve_write(int sockfd, struct nfp_cpp *cpp)
 		area = nfp_cpp_area_alloc_with_name(cpp, cpp_id, "nfp.cdev",
 						    nfp_offset, curlen);
 		if (area == NULL) {
-			RTE_LOG(ERR, PMD, "%s: area alloc fail\n", __func__);
+			PMD_DRV_LOG(ERR, "area alloc fail");
 			return -EIO;
 		}
 
 		/* mapping the target */
 		err = nfp_cpp_area_acquire(area);
 		if (err < 0) {
-			RTE_LOG(ERR, PMD, "area acquire failed\n");
+			PMD_DRV_LOG(ERR, "area acquire failed");
 			nfp_cpp_area_free(area);
 			return -EIO;
 		}
@@ -179,20 +178,18 @@ nfp_cpp_bridge_serve_write(int sockfd, struct nfp_cpp *cpp)
 			if (len > sizeof(tmpbuf))
 				len = sizeof(tmpbuf);
 
-			PMD_CPP_LOG(DEBUG, "%s: Receive %u of %zu\n", __func__,
-					   len, count);
+			PMD_CPP_LOG(DEBUG, "Receive %u of %zu", len, count);
 			err = recv(sockfd, tmpbuf, len, MSG_WAITALL);
 			if (err != (int)len) {
-				RTE_LOG(ERR, PMD,
-					"%s: error when receiving, %d of %zu\n",
-					__func__, err, count);
+				PMD_DRV_LOG(ERR, "error when receiving, %d of %zu",
+					    err, count);
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
 			}
 			err = nfp_cpp_area_write(area, pos, tmpbuf, len);
 			if (err < 0) {
-				RTE_LOG(ERR, PMD, "nfp_cpp_area_write error\n");
+				PMD_DRV_LOG(ERR, "nfp_cpp_area_write error");
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
@@ -227,7 +224,7 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 	size_t count, curlen;
 	int err = 0;
 
-	PMD_CPP_LOG(DEBUG, "%s: offset size %zu, count_size: %zu\n", __func__,
+	PMD_CPP_LOG(DEBUG, "offset size %zu, count_size: %zu",
 		sizeof(off_t), sizeof(size_t));
 
 	/* Reading the count param */
@@ -246,9 +243,8 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 	cpp_id = (offset >> 40) << 8;
 	nfp_offset = offset & ((1ull << 40) - 1);
 
-	PMD_CPP_LOG(DEBUG, "%s: count %zu and offset %jd\n", __func__, count,
-			   offset);
-	PMD_CPP_LOG(DEBUG, "%s: cpp_id %08x and nfp_offset %jd\n", __func__,
+	PMD_CPP_LOG(DEBUG, "count %zu and offset %jd", count, offset);
+	PMD_CPP_LOG(DEBUG, "cpp_id %08x and nfp_offset %jd",
 			   cpp_id, nfp_offset);
 
 	/* Adjust length if not aligned */
@@ -262,13 +258,13 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 		area = nfp_cpp_area_alloc_with_name(cpp, cpp_id, "nfp.cdev",
 						    nfp_offset, curlen);
 		if (area == NULL) {
-			RTE_LOG(ERR, PMD, "%s: area alloc failed\n", __func__);
+			PMD_DRV_LOG(ERR, "area alloc failed");
 			return -EIO;
 		}
 
 		err = nfp_cpp_area_acquire(area);
 		if (err < 0) {
-			RTE_LOG(ERR, PMD, "area acquire failed\n");
+			PMD_DRV_LOG(ERR, "area acquire failed");
 			nfp_cpp_area_free(area);
 			return -EIO;
 		}
@@ -280,19 +276,19 @@ nfp_cpp_bridge_serve_read(int sockfd, struct nfp_cpp *cpp)
 
 			err = nfp_cpp_area_read(area, pos, tmpbuf, len);
 			if (err < 0) {
-				RTE_LOG(ERR, PMD, "nfp_cpp_area_read error\n");
+				PMD_DRV_LOG(ERR, "nfp_cpp_area_read error");
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
 			}
-			PMD_CPP_LOG(DEBUG, "%s: sending %u of %zu\n", __func__,
+			PMD_CPP_LOG(DEBUG, "sending %u of %zu",
 					   len, count);
 
 			err = send(sockfd, tmpbuf, len, 0);
 			if (err != (int)len) {
-				RTE_LOG(ERR, PMD,
-					"%s: error when sending: %d of %zu\n",
-					__func__, err, count);
+				PMD_DRV_LOG(ERR,
+					    "error when sending: %d of %zu",
+					    err, count);
 				nfp_cpp_area_release(area);
 				nfp_cpp_area_free(area);
 				return -EIO;
@@ -325,39 +321,39 @@ nfp_cpp_bridge_serve_ioctl(int sockfd, struct nfp_cpp *cpp)
 	/* Reading now the IOCTL command */
 	err = recv(sockfd, &cmd, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: read error from socket\n", __func__);
+		PMD_DRV_LOG(ERR, "read error from socket");
 		return -EIO;
 	}
 
 	/* Only supporting NFP_IOCTL_CPP_IDENTIFICATION */
 	if (cmd != NFP_IOCTL_CPP_IDENTIFICATION) {
-		RTE_LOG(ERR, PMD, "%s: unknown cmd %d\n", __func__, cmd);
+		PMD_DRV_LOG(ERR, "unknown cmd %d", cmd);
 		return -EINVAL;
 	}
 
 	err = recv(sockfd, &ident_size, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: read error from socket\n", __func__);
+		PMD_DRV_LOG(ERR, "read error from socket");
 		return -EIO;
 	}
 
 	tmp = nfp_cpp_model(cpp);
 
-	PMD_CPP_LOG(DEBUG, "%s: sending NFP model %08x\n", __func__, tmp);
+	PMD_CPP_LOG(DEBUG, "sending NFP model %08x", tmp);
 
 	err = send(sockfd, &tmp, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: error writing to socket\n", __func__);
+		PMD_DRV_LOG(ERR, "error writing to socket");
 		return -EIO;
 	}
 
 	tmp = cpp->interface;
 
-	PMD_CPP_LOG(DEBUG, "%s: sending NFP interface %08x\n", __func__, tmp);
+	PMD_CPP_LOG(DEBUG, "sending NFP interface %08x", tmp);
 
 	err = send(sockfd, &tmp, 4, 0);
 	if (err != 4) {
-		RTE_LOG(ERR, PMD, "%s: error writing to socket\n", __func__);
+		PMD_DRV_LOG(ERR, "error writing to socket");
 		return -EIO;
 	}
 
@@ -384,8 +380,7 @@ nfp_cpp_bridge_service_func(void *args)
 	unlink("/tmp/nfp_cpp");
 	sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
 	if (sockfd < 0) {
-		RTE_LOG(ERR, PMD, "%s: socket creation error. Service failed\n",
-			__func__);
+		PMD_DRV_LOG(ERR, "socket creation error. Service failed");
 		return -EIO;
 	}
 
@@ -399,16 +394,14 @@ nfp_cpp_bridge_service_func(void *args)
 	ret = bind(sockfd, (const struct sockaddr *)&address,
 		   sizeof(struct sockaddr));
 	if (ret < 0) {
-		RTE_LOG(ERR, PMD, "%s: bind error (%d). Service failed\n",
-				  __func__, errno);
+		PMD_DRV_LOG(ERR, "bind error (%d). Service failed", errno);
 		close(sockfd);
 		return ret;
 	}
 
 	ret = listen(sockfd, 20);
 	if (ret < 0) {
-		RTE_LOG(ERR, PMD, "%s: listen error(%d). Service failed\n",
-				  __func__, errno);
+		PMD_DRV_LOG(ERR, "listen error(%d). Service failed", errno);
 		close(sockfd);
 		return ret;
 	}
@@ -421,9 +414,7 @@ nfp_cpp_bridge_service_func(void *args)
 			if (errno == EAGAIN || errno == EWOULDBLOCK)
 				continue;
 
-			RTE_LOG(ERR, PMD, "%s: accept call error (%d)\n",
-					  __func__, errno);
-			RTE_LOG(ERR, PMD, "%s: service failed\n", __func__);
+			PMD_DRV_LOG(ERR, "accept call error (%d)", errno);
 			close(sockfd);
 			return -EIO;
 		}
@@ -431,12 +422,11 @@ nfp_cpp_bridge_service_func(void *args)
 		while (1) {
 			ret = recv(datafd, &op, 4, 0);
 			if (ret <= 0) {
-				PMD_CPP_LOG(DEBUG, "%s: socket close\n",
-						   __func__);
+				PMD_CPP_LOG(DEBUG, "socket close");
 				break;
 			}
 
-			PMD_CPP_LOG(DEBUG, "%s: getting op %u\n", __func__, op);
+			PMD_CPP_LOG(DEBUG, "getting op %u", op);
 
 			if (op == NFP_BRIDGE_OP_READ)
 				nfp_cpp_bridge_serve_read(datafd, cpp);
diff --git a/drivers/net/nfp/nfp_ethdev.c b/drivers/net/nfp/nfp_ethdev.c
index 290e2fcb41a6..3ea07aa923dd 100644
--- a/drivers/net/nfp/nfp_ethdev.c
+++ b/drivers/net/nfp/nfp_ethdev.c
@@ -521,9 +521,9 @@ nfp_net_init(struct rte_eth_dev *eth_dev)
 
 	/* NFP can not handle DMA addresses requiring more than 40 bits */
 	if (rte_mem_check_dma_mask(40)) {
-		RTE_LOG(ERR, PMD,
-			"device %s can not be used: restricted dma mask to 40 bits!\n",
-			pci_dev->device.name);
+		PMD_DRV_LOG(ERR,
+			    "device %s can not be used: restricted dma mask to 40 bits!",
+			    pci_dev->device.name);
 		return -ENODEV;
 	}
 
diff --git a/drivers/net/nfp/nfp_ethdev_vf.c b/drivers/net/nfp/nfp_ethdev_vf.c
index 07a2e17ef8e7..6272b4c29466 100644
--- a/drivers/net/nfp/nfp_ethdev_vf.c
+++ b/drivers/net/nfp/nfp_ethdev_vf.c
@@ -293,9 +293,9 @@ nfp_netvf_init(struct rte_eth_dev *eth_dev)
 
 	/* NFP can not handle DMA addresses requiring more than 40 bits */
 	if (rte_mem_check_dma_mask(40)) {
-		RTE_LOG(ERR, PMD,
-			"device %s can not be used: restricted dma mask to 40 bits!\n",
-			pci_dev->device.name);
+		PMD_DRV_LOG(ERR,
+			    "device %s can not be used: restricted dma mask to 40 bits!\n",
+			    pci_dev->device.name);
 		return -ENODEV;
 	}
 
diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
index b7632ee72ca1..58dd1da12143 100644
--- a/drivers/net/nfp/nfp_logs.h
+++ b/drivers/net/nfp/nfp_logs.h
@@ -14,16 +14,23 @@ extern int nfp_logtype_init;
 		"%s(): " fmt "\n", __func__, ## args)
 #define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
 
+extern int nfp_logtype_driver;
+#define PMD_DRV_LOG(level, fmt, args...) \
+	rte_log(RTE_LOG_ ## level, nfp_logtype_driver, \
+		"%s(): " fmt "\n", __func__, ## args)
+
+#define PMD_DP_LOG(level, fmt, args...) \
+	rte_log_dp(RTE_LOG_ ## level, nfp_logtype_driver, \
+		   "%s(): " fmt "\n", __func__, ## args)
+
 #ifdef RTE_ETHDEV_DEBUG_RX
-#define PMD_RX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
+#define PMD_RX_LOG(level, fmt, args...) PMD_DP_LOG(level, "rx: " fmt, ## args)
 #else
 #define PMD_RX_LOG(level, fmt, args...) do { } while (0)
 #endif
 
 #ifdef RTE_ETHDEV_DEBUG_TX
-#define PMD_TX_LOG(level, fmt, args...) \
-	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
+#define PMD_TX_LOG(level, fmt, args...) PMD_DP_LOG(level, "tx: " fmt, ## args)
 #else
 #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
 #endif
@@ -33,9 +40,4 @@ extern int nfp_logtype_cpp;
 	rte_log(RTE_LOG_ ## level, nfp_logtype_cpp, \
 		"%s(): " fmt "\n", __func__, ## args)
 
-extern int nfp_logtype_driver;
-#define PMD_DRV_LOG(level, fmt, args...) \
-	rte_log(RTE_LOG_ ## level, nfp_logtype_driver, \
-		"%s(): " fmt "\n", __func__, ## args)
-
 #endif /* _NFP_LOGS_H_ */
diff --git a/drivers/net/nfp/nfp_rxtx.c b/drivers/net/nfp/nfp_rxtx.c
index cfc1a784b185..e5a3dc57ac38 100644
--- a/drivers/net/nfp/nfp_rxtx.c
+++ b/drivers/net/nfp/nfp_rxtx.c
@@ -353,7 +353,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		 * DPDK just checks the queue is lower than max queues
 		 * enabled. But the queue needs to be configured
 		 */
-		RTE_LOG_DP(ERR, PMD, "RX Bad queue\n");
+		PMD_DP_LOG(ERR, "RX Bad queue");
 		return avail;
 	}
 
@@ -363,7 +363,7 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 	while (avail < nb_pkts) {
 		rxb = &rxq->rxbufs[rxq->rd_p];
 		if (unlikely(rxb == NULL)) {
-			RTE_LOG_DP(ERR, PMD, "rxb does not exist!\n");
+			PMD_DP_LOG(ERR, "rxb does not exist!");
 			break;
 		}
 
@@ -383,9 +383,9 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		 */
 		new_mb = rte_pktmbuf_alloc(rxq->mem_pool);
 		if (unlikely(new_mb == NULL)) {
-			RTE_LOG_DP(DEBUG, PMD,
-			"RX mbuf alloc failed port_id=%u queue_id=%u\n",
-				rxq->port_id, (unsigned int)rxq->qidx);
+			PMD_DP_LOG(DEBUG,
+				   "RX mbuf alloc failed port_id=%u queue_id=%u\n",
+				   rxq->port_id, (unsigned int)rxq->qidx);
 			nfp_net_mbuf_alloc_failed(rxq);
 			break;
 		}
@@ -412,15 +412,15 @@ nfp_net_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 			 * responsibility of avoiding it. But we have
 			 * to give some info about the error
 			 */
-			RTE_LOG_DP(ERR, PMD,
-				"mbuf overflow likely due to the RX offset.\n"
-				"\t\tYour mbuf size should have extra space for"
-				" RX offset=%u bytes.\n"
-				"\t\tCurrently you just have %u bytes available"
-				" but the received packet is %u bytes long",
-				hw->rx_offset,
-				rxq->mbuf_size - hw->rx_offset,
-				mb->data_len);
+			PMD_DP_LOG(ERR,
+				   "mbuf overflow likely due to the RX offset.\n"
+				   "\t\tYour mbuf size should have extra space for"
+				   " RX offset=%u bytes.\n"
+				   "\t\tCurrently you just have %u bytes available"
+				   " but the received packet is %u bytes long",
+				   hw->rx_offset,
+				   rxq->mbuf_size - hw->rx_offset,
+				   mb->data_len);
 			rte_pktmbuf_free(mb);
 			break;
 		}

Re: [PATCH 2/3] net/nfp: get rid of the usage of RTE log level type

[Ferruh Yigit]

On 2/20/2023 4:16 PM, Stephen Hemminger wrote:
> On Mon, 20 Feb 2023 10:09:51 +0000
> Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> 
>> On 2/20/2023 1:36 AM, Chaoyong He wrote:
>>>> On 2/17/2023 2:45 AM, Chaoyong He wrote:  
>>>>> Register the own RX/TX debug log level type, and get rid of the usage
>>>>> of RTE_LOGTYPE_*. Then we can control the log by a independent switch.
>>>>>
>>>>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>>>> ---
>>>>>  drivers/net/nfp/nfp_logs.c | 10 ++++++++++
>>>>> drivers/net/nfp/nfp_logs.h |  8 ++++++--
>>>>>  2 files changed, 16 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/nfp/nfp_logs.c b/drivers/net/nfp/nfp_logs.c
>>>>> index 48c42fe53f..cd58bcee43 100644
>>>>> --- a/drivers/net/nfp/nfp_logs.c
>>>>> +++ b/drivers/net/nfp/nfp_logs.c
>>>>> @@ -5,6 +5,16 @@
>>>>>
>>>>>  #include "nfp_logs.h"
>>>>>
>>>>> +#include <rte_ethdev.h>
>>>>> +
>>>>>  RTE_LOG_REGISTER_SUFFIX(nfp_logtype_init, init, NOTICE);
>>>>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_driver, driver, NOTICE);
>>>>> RTE_LOG_REGISTER_SUFFIX(nfp_logtype_cpp, cpp, NOTICE);
>>>>> +
>>>>> +#ifdef RTE_ETHDEV_DEBUG_RX
>>>>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_rx, rx, DEBUG) #endif
>>>>> +
>>>>> +#ifdef RTE_ETHDEV_DEBUG_TX
>>>>> +RTE_LOG_REGISTER_SUFFIX(nfp_logtype_tx, tx, DEBUG) #endif
>>>>> diff --git a/drivers/net/nfp/nfp_logs.h b/drivers/net/nfp/nfp_logs.h
>>>>> index b7632ee72c..315a57811c 100644
>>>>> --- a/drivers/net/nfp/nfp_logs.h
>>>>> +++ b/drivers/net/nfp/nfp_logs.h
>>>>> @@ -15,15 +15,19 @@ extern int nfp_logtype_init;  #define
>>>>> PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
>>>>>
>>>>>  #ifdef RTE_ETHDEV_DEBUG_RX
>>>>> +extern int nfp_logtype_rx;
>>>>>  #define PMD_RX_LOG(level, fmt, args...) \
>>>>> -	RTE_LOG(level, PMD, "%s() rx: " fmt "\n", __func__, ## args)
>>>>> +	rte_log(RTE_LOG_ ## level, nfp_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 nfp_logtype_tx;
>>>>>  #define PMD_TX_LOG(level, fmt, args...) \
>>>>> -	RTE_LOG(level, PMD, "%s() tx: " fmt "\n", __func__, ## args)
>>>>> +	rte_log(RTE_LOG_ ## level, nfp_logtype_tx, \
>>>>> +		"%s(): " fmt "\n", __func__, ## args)
>>>>>  #else
>>>>>  #define PMD_TX_LOG(level, fmt, args...) do { } while (0)  #endif  
>>>>
>>>> Intention is to replace 'RTE_LOG_DP' with 'PMD_RX_LOG'/'PMD_TX_LOG',
>>>> but these are not exactly same (although difference is minor).
>>>>
>>>> When 'RTE_ETHDEV_DEBUG_RX' is set, ethdev layer also adds some
>>>> additional load, although I believe that will small comparing to logging in
>>>> driver.
>>>> If 'RTE_LOG_DP' used, the ethdev layer cost can be removed.
>>>>
>>>> With 'RTE_LOG_DP', log level more verbose than requested won't cause any
>>>> performance impact. Like if ERR level requested, INFO, DEBUG etc logs will
>>>> be compiled out and won't cause any performance impact.
>>>> But with 'RTE_ETHDEV_DEBUG_RX', even log level only request ERR, all
>>>> logging will add cost of at least an if branch (checking log level).
>>>>
>>>>
>>>> For many cases I am not sure these differences matters, and already many
>>>> drivers directly uses 'RTE_ETHDEV_DEBUG_RX' as done here. So you may
>>>> prefer to keep as it is.
>>>>
>>>> But if there is a desire for this fine grain approach, it is possible to add a
>>>> version of 'RTE_LOG_DP' macro that accepts dynamic log type (instead of
>>>> static RTE_LOGTYPE_# type), what do you think?
>>>>  
>>>
>>> Thanks for the suggestion.
>>> For now, we prefer to keep as it is.
>>> If we does need the more refined design in the future, we would follow your advice here, thanks again.  
>>
>> ack, I just wanted to double check. I will proceed as it is.
> 
> As part of my patch series (work in progress) to get rid of RTE_LOGTYPE_PMD
> needed to add a helper now for RTE_DP_LOG like this.
> 
> diff --git a/lib/eal/include/rte_log.h b/lib/eal/include/rte_log.h
> index 6d2b0856a565..f377bc6db79b 100644
> --- a/lib/eal/include/rte_log.h
> +++ b/lib/eal/include/rte_log.h
> @@ -336,6 +336,37 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>  	 rte_log(RTE_LOG_ ## l,					\
>  		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
>  
> +/**
> + * Generates a log message for data path.
> + *
> + * Similar to rte_log(), except that it is an inline function that
> + * can be eliminated at compilation time if RTE_LOG_DP_LEVEL configuration
> + * option is lower than the log level argument.
> + *
> + * @param level
> + *   Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
> + * @param logtype
> + *   The log type, for example, RTE_LOGTYPE_EAL.
> + * @param format
> + *   The format string, as in printf(3), followed by the variable arguments
> + *   required by the format.
> + * @return
> + *   - 0: Success.
> + *   - Negative on error.
> + */
> +static inline __rte_format_printf(3, 4)
> +void rte_log_dp(uint32_t level, uint32_t logtype, const char *format, ...)
> +
> +{
> +	va_list ap;
> +
> +	if (level <= RTE_LOG_DP_LEVEL) {
> +		va_start(ap, format);
> +		rte_vlog(level, logtype, format, ap);
> +		va_end(ap);
> +	}
> +}
> +
>  /**
>   * Generates a log message for data path.
>   *
> @@ -357,10 +388,8 @@ int rte_vlog(uint32_t level, uint32_t logtype, const char *format, va_list ap)
>   *   - Negative on error.
>   */
>  #define RTE_LOG_DP(l, t, ...)					\
> -	(void)((RTE_LOG_ ## l <= RTE_LOG_DP_LEVEL) ?		\
> -	 rte_log(RTE_LOG_ ## l,					\
> -		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__) :	\
> -	 0)
> +	rte_log_dp(RTE_LOG_ ## l,				\
> +		 RTE_LOGTYPE_ ## t, # t ": " __VA_ARGS__)
>  


+1 to have 'rte_log_dp()' as above, this way data path logs can be added
with dynamic log types.