Matan Azrad <ma...@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 <mleit...@redhat.com>
>> Signed-off-by: Aaron Conole <acon...@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
