Re: [dpdk-dev] [PATCH] ethdev: warn only once for badly behaving applications

[Thomas Monjalon <thomas@monjalon.net>]

26/10/2021 16:58, David Marchand:
> Warning continuously is a pain when developping or if a unit test
> is/gets broken.
> 
> It could also be a problem if application behaves badly only in some
> corner cases and a DoS results of those logs being continuously displayed.
> 
> Let's warn once per port and per rx/tx.
> 
> Getting such a log is scary, but let's make it more eye catching by
> dumping a backtrace with it.
[...]
> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into separate structure")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
[...]
> +static struct dummy_queue *dummy_queues_ref[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> +static struct dummy_queue dummy_queues[RTE_MAX_ETHPORTS];

I feel we could better name those arrays, maybe adding a comment.
First one is really queues array while the second one is to share
the same value with all queues of a port. Right?

> +RTE_INIT(dummy_queue_init)
> +{
> +	uint16_t port_id;
> +
> +	for (port_id = 0; port_id < RTE_DIM(dummy_queues); port_id++) {
> +		unsigned int i;

q would be a better name than i

> +
> +		for (i = 0; i < RTE_DIM(dummy_queues_ref[port_id]); i++)
> +			dummy_queues_ref[port_id][i] = &dummy_queues[port_id];
> +	}
> +}
> +
>  static uint16_t
> -dummy_eth_rx_burst(__rte_unused void *rxq,
> +dummy_eth_rx_burst(void *rxq,
>  		__rte_unused struct rte_mbuf **rx_pkts,
>  		__rte_unused uint16_t nb_pkts)
>  {
> -	RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for not ready port\n");
> +	struct dummy_queue *q = rxq;
> +
> +	if (!q->rx_warn_once) {
> +		uint16_t port_id = q - dummy_queues;
> +
> +		RTE_ETHDEV_LOG(ERR, "lcore %u called rx_pkt_burst for not ready port %"PRIu16"\n",
> +			rte_lcore_id(), port_id);
> +		rte_dump_stack();
> +		q->rx_warn_once = true;
> +	}
>  	rte_errno = ENOTSUP;
>  	return 0;
>  }

OK with this log.

[...]
>  eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
>  {
>  	static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> -	static const struct rte_eth_fp_ops dummy_ops = {
> +	uint16_t port_id = fpo - rte_eth_fp_ops;
> +
> +	dummy_queues[port_id].rx_warn_once = false;
> +	dummy_queues[port_id].tx_warn_once = false;
> +	*fpo = (struct rte_eth_fp_ops) {
>  		.rx_pkt_burst = dummy_eth_rx_burst,
>  		.tx_pkt_burst = dummy_eth_tx_burst,
> -		.rxq = {.data = dummy_data, .clbk = dummy_data,},
> -		.txq = {.data = dummy_data, .clbk = dummy_data,},
> +		.rxq = (struct rte_ethdev_qdata) {

Why this cast? rte_eth_fp_ops.rxq is of type rte_ethdev_qdata.

> +			.data = (void **)&dummy_queues_ref[port_id],
> +			.clbk = dummy_data,
> +		},
> +		.txq = (struct rte_ethdev_qdata) {
> +			.data = (void **)&dummy_queues_ref[port_id],
> +			.clbk = dummy_data,
> +		},
>  	};
> -
> -	*fpo = dummy_ops;
>  }