From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ananyev, Konstantin" Subject: Re: [PATCH v11 1/6] ethdev: add Tx preparation Date: Fri, 28 Oct 2016 11:34:49 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583F0CEC6E@irsmsx105.ger.corp.intel.com> References: <1477327917-18564-1-git-send-email-tomaszx.kulasek@intel.com> <2078955.d1Aiqtukxu@xps13> <2601191342CEEE43887BDE71AB9772583F0CE8E3@irsmsx105.ger.corp.intel.com> <2500924.jYNDaNt7Th@xps13> <2601191342CEEE43887BDE71AB9772583F0CEC55@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: "Ananyev, Konstantin" , Thomas Monjalon Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 017CC568A for ; Fri, 28 Oct 2016 13:34:51 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEC55@irsmsx105.ger.corp.intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >=20 > Hi Thomasz, >=20 > > > > 2016-10-27 16:24, Ananyev, Konstantin: > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > > > 2016-10-27 15:52, Ananyev, Konstantin: > > > > > > Hi Tomasz, > > > > > > > > > > > > This is a major new function in the API and I still have some c= omments. > > > > > > > > > > > > 2016-10-26 14:56, Tomasz Kulasek: > > > > > > > --- a/config/common_base > > > > > > > +++ b/config/common_base > > > > > > > +CONFIG_RTE_ETHDEV_TX_PREP=3Dy > > > > > > > > > > > > We cannot enable it until it is implemented in every drivers. > > > > > > > > > > Not sure why? > > > > > If tx_pkt_prep =3D=3D NULL, then rte_eth_tx_prep() would just act= as noop. > > > > > Right now it is not mandatory for the PMD to implement it. > > > > > > > > If it is not implemented, the application must do the preparation b= y itself. > > > > From patch 6: > > > > " > > > > Removed pseudo header calculation for udp/tcp/tso packets from > > > > application and used Tx preparation API for packet preparation and > > > > verification. > > > > " > > > > So how does it behave with other drivers? > > > > > > Hmm so it seems that we broke testpmd csumonly mode for non-intel dri= vers.. > > > My bad, missed that part completely. > > > Yes, then I suppose for now we'll need to support both (with and with= out) code paths for testpmd. > > > Probably a new fwd mode or just extra parameter for the existing one? > > > Any other suggestions? > > > > Please think how we can use it in every applications. > > It is not ready. > > Either we introduce the API without enabling it, or we implement it > > in every drivers. >=20 > I understand your position here, but just like to point that: > 1) It is a new functionality optional to use. > The app is free not to use that functionality and still do the prepa= ration itself > (as it has to do it now). > All existing apps would keep working as expected without using that f= unction. > Though if the app developer knows that for all HW models he plans to = run on > tx_prep is implemented - he is free to use it. > 2) It would be difficult for Tomasz (and other Intel guys) to impleme= nt tx_prep() > for all non-Intel HW that DPDK supports right now. > We just don't have all the actual HW in stock and probably adequate = knowledge of it. > So we depend here on the good will of other PMD mainaners/developers = to implement > tx_prep() for these devices. > From other side, if it will be disabled by default, then, I think, > PMD developers just wouldn't be motivated to implement it. > So it will be left untested and unused forever. Actually as another thought: Can we have it enabled by default, but mark it as experimental or so? If memory serves me right, we've done that for cryptodev in the past, no? Konstantin >=20 > > > > > > > > > struct rte_eth_dev { > > > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive fu= nction. */ > > > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit f= unction. */ > > > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit pre= pare function. */ > > > > > > > struct rte_eth_dev_data *data; /**< Pointer to device data= */ > > > > > > > const struct eth_driver *driver;/**< Driver for this device= */ > > > > > > > const struct eth_dev_ops *dev_ops; /**< Functions exported = by PMD */ > > > > > > > > > > > > Could you confirm why tx_pkt_prep is not in dev_ops? > > > > > > I guess we want to have several implementations? > > > > > > > > > > Yes, it depends on configuration options, same as tx_pkt_burst. > > > > > > > > > > > > > > > > > Shouldn't we have a const struct control_dev_ops and a struct d= atapath_dev_ops? > > > > > > > > > > That's probably a good idea, but I suppose it is out of scope for= that patch. > > > > > > > > No it's not out of scope. > > > > It answers to the question "why is it added in this structure and n= ot dev_ops". > > > > We won't do this change when nothing else is changed in the struct. > > > > > > Not sure I understood you here: > > > Are you saying datapath_dev_ops/controlpath_dev_ops have to be introd= uced as part of that patch? > > > But that's a lot of changes all over rte_ethdev.[h,c]. > > > It definitely worse a separate patch (might be some discussion) for m= e. > > > > Yes it could be a separate patch in the same patchset. >=20 > Honestly, I think it is a good idea, but it is too late and too risky to = do such change right now. > We are on RC2 right now, just few days before RC3... > Can't that wait till 17.02? > From my understanding - it is pure code restructuring, without any functi= onality affected. > Konstantin >=20