From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH v11 1/6] ethdev: add Tx preparation Date: Fri, 28 Oct 2016 16:44:48 +0530 Message-ID: <20161028111446.GA4005@localhost.localdomain> References: <1477327917-18564-1-git-send-email-tomaszx.kulasek@intel.com> <1499338.8Le0ABsxDG@xps13> <2601191342CEEE43887BDE71AB9772583F0CD83D@irsmsx105.ger.corp.intel.com> <2078955.d1Aiqtukxu@xps13> <2601191342CEEE43887BDE71AB9772583F0CE8E3@irsmsx105.ger.corp.intel.com> <3042915272161B4EB253DA4D77EB373A14F45162@IRSMSX102.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772583F0CEAEE@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: "dev@dpdk.org" , Thomas Monjalon To: "Ananyev, Konstantin" Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0071.outbound.protection.outlook.com [104.47.33.71]) by dpdk.org (Postfix) with ESMTP id E05892C4D for ; Fri, 28 Oct 2016 13:15:20 +0200 (CEST) Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0CEAEE@irsmsx105.ger.corp.intel.com> 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" On Fri, Oct 28, 2016 at 10:15:47AM +0000, Ananyev, Konstantin wrote: > Hi Tomasz, > > > > > > Not sure why? > > > > > If tx_pkt_prep == 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 by > > > 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 > > > drivers.. > > > My bad, missed that part completely. > > > Yes, then I suppose for now we'll need to support both (with and without) > > > code paths for testpmd. > > > Probably a new fwd mode or just extra parameter for the existing one? > > > Any other suggestions? > > > > > > > I had sent txprep engine in v2 (http://dpdk.org/dev/patchwork/patch/15775/), but I'm opened on the suggestions. If you like it I can resent > > it in place of csumonly modification. > > I still not sure it is worth to have another version of csum... > Can we introduce a new global variable in testpmd and a new command: > testpmd> csum tx_prep Just my 2 cents, As "tx_prep" is a generic API and if PMD tries to fix-up some other limitation(not csum) then in that case it is difficult for the application to know in which PMD&application combination it needs be used. > or so? > Looking at current testpmd patch, I suppose the changes will be minimal. > What do you think? > Konstantin > > > > > Tomasz > > > > > > > > > > > > > struct rte_eth_dev { > > > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive > > > function. */ > > > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit > > > > > > > function. */ > > > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit > > > > > > > +prepare 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 > > > datapath_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 not > > > 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 introduced > > > 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 me. > > > Konstantin > > > > > > >