From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [67.231.154.164]) by dpdk.org (Postfix) with ESMTP id 5A320F94 for ; Mon, 23 Jul 2018 22:07:42 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id BF6EC400086; Mon, 23 Jul 2018 20:07:40 +0000 (UTC) Received: from [192.168.1.16] (85.187.13.33) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Mon, 23 Jul 2018 21:07:20 +0100 To: "Ananyev, Konstantin" , Matan Azrad , Aaron Conole CC: "dev@dpdk.org" , "Yigit, Ferruh" , Marcelo Leitner , Shahaf Shuler , Ori Kam , Thomas Monjalon References: <20180722112116.8419-1-aconole@redhat.com> <2601191342CEEE43887BDE71AB977258DF51B4D9@irsmsx105.ger.corp.intel.com> From: Andrew Rybchenko Message-ID: <541740c9-efee-c95f-ad0f-dfe168fc440c@solarflare.com> Date: Mon, 23 Jul 2018 23:07:00 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB977258DF51B4D9@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.33] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.2.1013-23986.004 X-TM-AS-Result: No-9.833400-8.000000-10 X-TMASE-MatchedRID: fE0JoqABJp0OwH4pD14DsPHkpkyUphL9t7k6BDMlB1j61pkeRaBywyN1 djvDVHBaJOCwZ29g6j39AoOUQOta/PnVY0DWsTq3j5hLPCX3ZdPAmOfzKotTohUb9+x7ts88YeK /8Aj84WSVeaQzRhkjcTUM4gX4c0g4wwTYGyiIokwZokyW9/FkXk4Z9+xRT+QNteu8BQ9YmijZ6K rkcB2NwV4II5XWmiQD2mT5DR7XEdBm1pXqgYWmDhHRbGr1ECgHCQ3xS+zL6e3agsZM0qVv14Rjb 6q99slJIiChBmUR/ndwoI7ADD6NSEIjaJSsaV6q2OSj4qJA9QYWTveLitVUgbQScaSot0SMwfph NnCDsrBO9UxJ8vboSYUlXTB9n66TSBiSH4+du39QiFNNqFvt1fQ7szeVKdNbmyiLZetSf8nJ4y0 wP1A6AKEwgORH8p/AjaPj0W1qn0SyO81X3yak8wjSfEzp9sZbtETrPzMNhN0NJha4xlI3biKbeb YK6NsvBoHb0eGUIVZ+3BndfXUhXQ== X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--9.833400-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.2.1013-23986.004 X-MDID: 1532376461-tjULQlj1cyGi 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 20:07:42 -0000 On 23.07.2018 17:19, Ananyev, Konstantin wrote: >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matan Azrad >> Sent: Monday, July 23, 2018 1:14 PM >> To: Aaron Conole >> Cc: dev@dpdk.org; Yigit, Ferruh ; Marcelo Leitner ; Shahaf Shuler >> ; Ori Kam ; Thomas Monjalon >> Subject: Re: [dpdk-dev] [PATCH] ethdev: move sanity checks to non-debug paths >> >> >> Hi Aaron >> From: Aaron Conole >>> Sent: Monday, July 23, 2018 2:52 PM >>> To: Matan Azrad >>> Cc: dev@dpdk.org; Ferruh Yigit ; Marcelo Leitner >>> ; Shahaf Shuler ; Ori Kam >>> ; Thomas Monjalon >>> Subject: Re: [dpdk-dev] [PATCH] ethdev: move sanity checks to non-debug paths >>> >>> 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. >> Yes, I understand your point, but think about that, if we are going to defend each user mistake it will cost a lot. >> For example in Tx path, Adding checks for each mbuf pointer and mbuf data validity will be very expensive. >> >> I think the best way is to check the common user mistakes in DEBUG mode to help for application debugging and that's it. > +1 > The problem is that user provided an invalid input parameters. > Adding just extra checks inside data-path functions wouldn't solve it. > Konstantin +1, I agree with Matan and Konstantin So, NACK >>>>> 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