[dpdk-dev] [PATCH] ethdev: move sanity checks to non-debug paths

Aaron Conole aconole at redhat.com
Mon Jul 23 13:51:52 CEST 2018


Matan Azrad <matan at mellanox.com> writes:

> Hi Aaron
>
> From: Aaron Conole
>> These checks would have prevented a reported crash in the field.  If a user
>> builds without ETHDEV_DEBUG, it should make their application more stable,
>> not less.
>> 
>> Many of these functions immediately dereference arrays based on the passed
>> in values, so the sanity checks are quite important.
>> 
>
> These functions are datapath functions.
> Do you really want to add more 3 checks + calculations per each burst call?
> Did you check the performance impact?
> I think that performance numbers must be added for the discussion of this patch.

I'll dig up performance numbers - but performance doesn't mean anything
if the application isn't running any longer due to crash.

>> The logs are left as DEBUG only.
>> 
>> Cc: Marcelo Leitner <mleitner at redhat.com>
>> Signed-off-by: Aaron Conole <aconole at redhat.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.h | 29 +++++++++++++----------------
>>  1 file changed, 13 insertions(+), 16 deletions(-)
>> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index f5f593b31..bfd6a3406 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -3805,15 +3805,16 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t
>> queue_id,
>>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>  	uint16_t nb_rx;
>> 
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
>> 
>>  	if (queue_id >= dev->data->nb_rx_queues) {
>> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  		RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n",
>> queue_id);
>> +#endif
>>  		return 0;
>>  	}
>> -#endif
>> +
>>  	nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
>>  				     rx_pkts, nb_pkts);
>> 
>> @@ -3928,14 +3929,12 @@ rte_eth_rx_descriptor_status(uint16_t port_id,
>> uint16_t queue_id,
>>  	struct rte_eth_dev *dev;
>>  	void *rxq;
>> 
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); -#endif
>>  	dev = &rte_eth_devices[port_id];
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>> +
>>  	if (queue_id >= dev->data->nb_rx_queues)
>>  		return -ENODEV;
>> -#endif
>> +
>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_status, -
>> ENOTSUP);
>>  	rxq = dev->data->rx_queues[queue_id];
>> 
>> @@ -3985,14 +3984,12 @@ static inline int
>> rte_eth_tx_descriptor_status(uint16_t port_id,
>>  	struct rte_eth_dev *dev;
>>  	void *txq;
>> 
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); -#endif
>>  	dev = &rte_eth_devices[port_id];
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>> +
>>  	if (queue_id >= dev->data->nb_tx_queues)
>>  		return -ENODEV;
>> -#endif
>> +
>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_descriptor_status, -
>> ENOTSUP);
>>  	txq = dev->data->tx_queues[queue_id];
>> 
>> @@ -4071,15 +4068,15 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t
>> queue_id,  {
>>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>> 
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
>> 
>>  	if (queue_id >= dev->data->nb_tx_queues) {
>> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
>> queue_id);
>> +#endif
>>  		return 0;
>>  	}
>> -#endif
>> 
>>  #ifdef RTE_ETHDEV_RXTX_CALLBACKS
>>  	struct rte_eth_rxtx_callback *cb = dev->pre_tx_burst_cbs[queue_id];
>> @@ -4160,23 +4157,23 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t
>> queue_id,  {
>>  	struct rte_eth_dev *dev;
>> 
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	if (!rte_eth_dev_is_valid_port(port_id)) {
>> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  		RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
>> +#endif
>>  		rte_errno = -EINVAL;
>>  		return 0;
>>  	}
>> -#endif
>> 
>>  	dev = &rte_eth_devices[port_id];
>> 
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	if (queue_id >= dev->data->nb_tx_queues) {
>> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n",
>> queue_id);
>> +#endif
>>  		rte_errno = -EINVAL;
>>  		return 0;
>>  	}
>> -#endif
>> 
>>  	if (!dev->tx_pkt_prepare)
>>  		return nb_pkts;
>> --
>> 2.14.3


More information about the dev mailing list