From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id DD55BF72 for ; Mon, 23 Jul 2018 16:20:17 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Jul 2018 07:20:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,393,1526367600"; d="scan'208";a="69471143" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga002.fm.intel.com with ESMTP; 23 Jul 2018 07:19:52 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.195]) by IRSMSX101.ger.corp.intel.com ([169.254.1.185]) with mapi id 14.03.0319.002; Mon, 23 Jul 2018 15:19:51 +0100 From: "Ananyev, Konstantin" To: Matan Azrad , Aaron Conole CC: "dev@dpdk.org" , "Yigit, Ferruh" , Marcelo Leitner , Shahaf Shuler , Ori Kam , Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH] ethdev: move sanity checks to non-debug paths Thread-Index: AQHUIa4uqeQYdFHnREWhqTeJxRJtX6ScRCeAgABvvi3///U8AIAAM3PQ Date: Mon, 23 Jul 2018 14:19:50 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258DF51B4D9@irsmsx105.ger.corp.intel.com> References: <20180722112116.8419-1-aconole@redhat.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMWIxNDYzOTctMDU1Yi00MDJhLWI4MDgtMmQ2OGM0ZjNiZDllIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiT0dnbVdwd0lMZ0h6U1dJNFJPcE1tRWtqWFZnSFpPYzNrdEpMZDNSR3hsZUZGUFpRaSt2TnlRdGZFMFNtaEFmSyJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 14:20:18 -0000 > -----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 p= aths >=20 >=20 > Hi Aaron > From: Aaron Conole > > Sent: Monday, July 23, 2018 2:52 PM > > To: Matan Azrad > > Cc: dev@dpdk.org; Ferruh Yigit ; Marcelo Leitne= r > > ; 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. >=20 > Yes, I understand your point, but think about that, if we are going to de= fend 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. >=20 > I think the best way is to check the common user mistakes in DEBUG mode t= o 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 >=20 > > >> 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 =3D &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 >=3D dev->data->nb_rx_queues) { > > >> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > >> RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=3D%u\n", > > queue_id); > > >> +#endif > > >> return 0; > > >> } > > >> -#endif > > >> + > > >> nb_rx =3D (*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 =3D &rte_eth_devices[port_id]; > > >> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > >> + > > >> if (queue_id >=3D dev->data->nb_rx_queues) > > >> return -ENODEV; > > >> -#endif > > >> + > > >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_status, - > > >> ENOTSUP); > > >> rxq =3D 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 =3D &rte_eth_devices[port_id]; > > >> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > >> + > > >> if (queue_id >=3D dev->data->nb_tx_queues) > > >> return -ENODEV; > > >> -#endif > > >> + > > >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_descriptor_status, - > > >> ENOTSUP); > > >> txq =3D 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 =3D &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 >=3D dev->data->nb_tx_queues) { > > >> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > >> RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=3D%u\n", > > queue_id); > > >> +#endif > > >> return 0; > > >> } > > >> -#endif > > >> > > >> #ifdef RTE_ETHDEV_RXTX_CALLBACKS > > >> struct rte_eth_rxtx_callback *cb =3D dev->pre_tx_burst_cbs[queue_i= d]; > > >> @@ -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=3D%u\n", port_id); > > >> +#endif > > >> rte_errno =3D -EINVAL; > > >> return 0; > > >> } > > >> -#endif > > >> > > >> dev =3D &rte_eth_devices[port_id]; > > >> > > >> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > >> if (queue_id >=3D dev->data->nb_tx_queues) { > > >> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > >> RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=3D%u\n", > > queue_id); > > >> +#endif > > >> rte_errno =3D -EINVAL; > > >> return 0; > > >> } > > >> -#endif > > >> > > >> if (!dev->tx_pkt_prepare) > > >> return nb_pkts; > > >> -- > > >> 2.14.3