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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/21/2023 8:42 PM, longli@linuxonhyperv.com wrote:
> diff --git a/drivers/net/mana/tx.c b/drivers/net/mana/tx.c
> index 300bf27cc1..a45b5e289c 100644
> --- a/drivers/net/mana/tx.c
> +++ b/drivers/net/mana/tx.c
> @@ -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");
> +			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?