From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id CF66098 for ; Mon, 23 Jul 2018 13:51:55 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0B8A2B798; Mon, 23 Jul 2018 11:51:55 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (ovpn-120-39.rdu2.redhat.com [10.10.120.39]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CC4CE2026D65; Mon, 23 Jul 2018 11:51:53 +0000 (UTC) From: Aaron Conole To: Matan Azrad Cc: "dev\@dpdk.org" , Ferruh Yigit , Marcelo Leitner , Shahaf Shuler , Ori Kam , Thomas Monjalon References: <20180722112116.8419-1-aconole@redhat.com> Date: Mon, 23 Jul 2018 07:51:52 -0400 In-Reply-To: (Matan Azrad's message of "Mon, 23 Jul 2018 06:12:31 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.0.90 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Mon, 23 Jul 2018 11:51:55 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Mon, 23 Jul 2018 11:51:55 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'aconole@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [PATCH] ethdev: move sanity checks to non-debug paths X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Jul 2018 11:51:56 -0000 Matan Azrad 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 >> Signed-off-by: Aaron Conole >> --- >> 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