From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 2AD8337A8 for ; Thu, 8 Sep 2016 18:09:09 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 08 Sep 2016 09:09:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,301,1470726000"; d="scan'208";a="165903376" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by fmsmga004.fm.intel.com with ESMTP; 08 Sep 2016 09:09:08 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.123]) by IRSMSX152.ger.corp.intel.com ([169.254.6.43]) with mapi id 14.03.0248.002; Thu, 8 Sep 2016 17:09:06 +0100 From: "Kulasek, TomaszX" To: Jerin Jacob CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 1/6] ethdev: add Tx preparation Thread-Index: AQHR/7Y6xWufAhJLBU+dYw8D9l774aBvNRWAgACBvZA= Date: Thu, 8 Sep 2016 16:09:05 +0000 Message-ID: <3042915272161B4EB253DA4D77EB373A14F050BA@IRSMSX102.ger.corp.intel.com> References: <1472228578-6980-1-git-send-email-tomaszx.kulasek@intel.com> <1472228578-6980-2-git-send-email-tomaszx.kulasek@intel.com> <20160908072845.GA7333@localhost.localdomain> In-Reply-To: <20160908072845.GA7333@localhost.localdomain> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 1/6] ethdev: add Tx preparation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Sep 2016 16:09:09 -0000 Hi Jerin, > -----Original Message----- > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > Sent: Thursday, September 8, 2016 09:29 > To: Kulasek, TomaszX > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 1/6] ethdev: add Tx preparation >=20 [...] > > +static inline uint16_t > > +rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf > **tx_pkts, > > + uint16_t nb_pkts) > > +{ > > + struct rte_eth_dev *dev =3D &rte_eth_devices[port_id]; > > + > > + if (!dev->tx_pkt_prep) { > > + rte_errno =3D -ENOTSUP; >=20 > rte_errno update may not be necessary here. see below >=20 > > + return 0; > IMO, We should return "nb_pkts" here instead of 0(i.e, all the packets ar= e > valid in-case PMD does not have tx_prep function) and in-case of "0" > the following check in the application also will fail for no reason if > (nb_prep < nb_pkts) { > printf("tx_prep failed\n"); > } >=20 Yes, it seems to be reasonable. >=20 > > + } > > + > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > + if (queue_id >=3D dev->data->nb_tx_queues) { > > + RTE_PMD_DEBUG_TRACE("Invalid TX queue_id=3D%d\n", queue_id); > > + rte_errno =3D -EINVAL; > > + return 0; > > + } > > +#endif > > + > > + return (*dev->tx_pkt_prep)(dev->data->tx_queues[queue_id], > > + tx_pkts, nb_pkts); > > +} > > + >=20 > IMO, We need to provide a compile time option for rte_eth_tx_prep as NOOP= . > Default option should be non NOOP but incase a _target_ want to override > to NOOP it should be possible, the reasons is: >=20 > - Low-end ARMv7,ARMv8 targets may not have PCIE-RC support and it may hav= e > only integrated NIC controller. On those targets, where integrated NIC > controller does not use tx_prep service it can made it as NOOP to save > cycles on following "rte_eth_tx_prep" and associated "if (unlikely(nb_pre= p > < nb_rx))" checks in the application. >=20 > /* Prepare burst of TX packets */ > nb_prep =3D rte_eth_tx_prep(fs->rx_port, 0, pkts_burst, nb_rx); >=20 > if (unlikely(nb_prep < nb_rx)) { > int i; > for (i =3D nb_prep; i < nb_rx; i++) > rte_pktmbuf_free(pkts_burst[i]); } >=20 You mean to have a code for NOOP like: /* Prepare burst of TX packets */ nb_prep =3D nb_rx; /* rte_eth_tx_prep(fs->rx_port, 0, pkts_burst, nb_rx); = */ =20 if (unlikely(nb_prep < nb_rx)) { int i; for (i =3D nb_prep; i < nb_rx; i++) rte_pktmbuf_free(pkts_burst[i]); } and let optimizer to remove unused parts? IMHO it should be an application issue to use tx_prep or not. While part of the job is done by the driver (verification and preparation),= and part by application (error handling), such a global compile time optio= n can introduce inconsistency, if application will not handle both cases. If someone wants to turn off this functionality, it should be done on appli= cation level, e.g. with compilation option. =20 >=20 > Jerin >=20