From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id AAC4E569F for ; Fri, 23 Sep 2016 11:42:02 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 23 Sep 2016 02:41:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,381,1470726000"; d="scan'208";a="1055262386" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga002.jf.intel.com with ESMTP; 23 Sep 2016 02:41:54 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX152.ger.corp.intel.com (163.33.192.66) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 23 Sep 2016 10:41:53 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.196]) by IRSMSX156.ger.corp.intel.com ([169.254.3.80]) with mapi id 14.03.0248.002; Fri, 23 Sep 2016 10:41:53 +0100 From: "Ananyev, Konstantin" To: Jerin Jacob CC: "Kulasek, TomaszX" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 1/6] ethdev: add Tx preparation Thread-Index: AQHSDQSdbAMPwzKA5ECqoX7z1xdg36CAxzYwgAAjHICAAApzgIAAQB+ggAJk+oCAAbH68P//+XYAgABhE/A= Date: Fri, 23 Sep 2016 09:41:52 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583F0BA7E0@irsmsx105.ger.corp.intel.com> References: <1472228578-6980-1-git-send-email-tomaszx.kulasek@intel.com> <1473691487-10032-1-git-send-email-tomaszx.kulasek@intel.com> <1473691487-10032-2-git-send-email-tomaszx.kulasek@intel.com> <2601191342CEEE43887BDE71AB9772583F0B583F@irsmsx105.ger.corp.intel.com> <3042915272161B4EB253DA4D77EB373A14F1A294@IRSMSX102.ger.corp.intel.com> <20160919160630.GA18610@localhost.localdomain> <2601191342CEEE43887BDE71AB9772583F0B8F33@irsmsx105.ger.corp.intel.com> <20160921082956.GA5358@localhost.localdomain> <2601191342CEEE43887BDE71AB9772583F0BA159@irsmsx105.ger.corp.intel.com> <20160922095947.GA6420@localhost.localdomain> In-Reply-To: <20160922095947.GA6420@localhost.localdomain> Accept-Language: en-IE, 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 v2 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: Fri, 23 Sep 2016 09:42:03 -0000 Hi Jerin, >=20 > Hi Konstantin, >=20 > > > > Hi Jerin, > > > > > > > > > > Hi Jerin, > > > > > > Hi Konstantin, > > > > > > > > > > > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > + > > > > > > > > +#ifdef RTE_ETHDEV_TX_PREP > > > > > > > > > > > > > > Sorry for being a bit late on that discussion, but what the > > > > > > > point of having that config macro (RTE_ETHDEV_TX_PREP ) at al= l? > > > > > > > As I can see right now, if driver doesn't setup tx_pkt_prep, > > > > > > > then nb_pkts would be return anyway... > > > > > > > > > > > > > > BTW, there is my another question - should it be that way? > > > > > > > Shouldn't we return 0 (and set rte_errno=3DENOTSUP) here if > > > > > > > dev->tx_pk_prep =3D=3D NULL? > > > > > > > > > > > > > > > > > > > It's an answer to the Jerin's request discussed here: > > > > > > http://dpdk.org/ml/archives/dev/2016-September/046437.html > > > > > > > > > > > > When driver doesn't support tx_prep, default behavior is "we > > > > > > don't know requirements, so we have nothing to do here". It > > > > > > will simplify > > > > > application logic and improve performance for these drivers, I th= ink. Catching this error with every burst may be problematic. > > > > > > > > > > > > As for RTE_ETHDEV_TX_PREP macro, suggested by Jerin in the > > > > > > same thread, I still don't think It's the best solution of the > > > > > > problem > > > > > described by him. I have added it here for further discussion. > > > > > > > > > > > > Jerin, have you something to add? > > > > > > > > > > Nothing very specific to add here. I think, I have tried to > > > > > share the rational in, http://dpdk.org/ml/archives/dev/2016- > > > > > September/046437.html > > > > > > > > > > > > > Ok, not sure I am fully understand your intention here. > > > > I think I understand why you propose rte_eth_tx_prep() to do: > > > > if (!dev->tx_pkt_prep) > > > > return nb_pkts; > > > > > > > > That allows drivers to NOOP the tx_prep functionality without > > > > paying the price for callback invocation. > > > > > > In true sense, returning the nb_pkts makes it functional NOP as > > > well(i.e The PMD does not have any limitation on Tx side, so all > > > packets are _good_(no preparation is required)) > > > > > > > > > > What I don't understand, why with that in place we also need a > > > > NOOP for the whole rte_eth_tx_prep(): > > > > +static inline uint16_t > > > > +rte_eth_tx_prep(uint8_t port_id __rte_unused, uint16_t queue_id __= rte_unused, > > > > + struct rte_mbuf **tx_pkts __rte_unused, uint16_t nb_pkts) { > > > > + return nb_pkts; > > > > +} > > > > + > > > > +#endif > > > > > > > > What are you trying to save here: just reading ' dev->tx_pkt_prep'? > > > > If so, then it seems not that performance critical for me. > > > > Something else? > > > > > > The proposed scheme can make it as true NOOP from compiler > > > perspective too if a target decided to do that, I have checked the in= struction generation with arm Assembly, a non true compiler > NOOP has following instructions overhead at minimum. > > > > > > # 1 load > > > # 1 mov > > > if (!dev->tx_pkt_prep) > > > return nb_pkts; > > > > Yep. > > > > > > > > # compile can't predict this function needs be executed or not so > > > # pressure on register allocation and mostly likely it call for > > > # some stack push and pop based load on outer function(as it is an > > > # inline function) > > > > > > Well, I suppose compiler wouldn't try to fill function argument registe= rs before the branch above. >=20 > Not the case with arm gcc compiler(may be based outer function load). Ok, so for my own curiosity (I am not very familiar with the ARM arch): gcc generates several conditional execution instructions in a row to spill= /fill=20 function arguments registers, and that comes at a price at execution time i= f condition is not met?=20 > The recent, external pool manager function pointer conversion reduced aro= und 700kpps/core in local cache mode(even though the > function pointers are not executed) >=20 > > > > > > > > return (*dev->tx_pkt_prep)(dev->data->tx_queues[queue_id], tx_pkts, > > > nb_pkts); > > > > > > # 1 branch > > > if (unlikely(nb_prep < nb_rx)) { > > > # bunch of code not executed, but pressure on i cache > > > int i; > > > for (i =3D nb_prep; i < nb_rx; i++) > > > rte_pktmbuf_free(pkts_burst[i]); > > > } > > > > > > From a server target(IA or high-end armv8) with external PCIe based > > > system makes sense to have RTE_ETHDEV_TX_PREP option enabled(which > > > is the case in proposed patch) but the low end arm platforms with > > > - limited cores > > > - less i cache > > > - IPC =3D=3D 1 > > > - running around 1GHz > > > - most importantly, _integrated_ nic controller with no external PCIE > > > support > > > does not make much sense to waste the cycles/time for it. > > > cycle saved is cycle earned. > > > > Ok, so it is all to save one memory de-refrence and a comparison plus b= ranch. > > Do you have aby estimation how badly it would hit low-end cpu performan= ce? >=20 > around 400kpps/core. On four core systems, around 2 mpps.(4 core with > 10G*2 ports) So it is about ~7% for 2x10G, correct? I agree that seems big enough to keep the config option, even though I am not quite happy with introducing new config option.=20 So no more objections from my side here. Thanks=20 Konstantin >=20 > > The reason I am asking: obviously I would prefer to avoid to introduce > > new build config option (that's the common dpdk coding practice these d= ays) unless it is really important. > Practice is something we need to revisit based on the new use case/usage. > I think, the scheme of non external pcie based NW cards is new to DPDK. >=20 > > > > > > > > Since DPDK compilation is based on _target_, I really don't see any > > > issue with this approach nor it does not hurt anything on server targ= et. > > > So, IMO, It should be upto the target to decide what works better for= the target. > > > > > > Jerin > > > > > > > From my point of view NOOP on the driver level is more than enough. > > > > Again I would prefer to introduce new config option, if possible. > > > > > > > > Konstantin > > > >