From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kulasek, TomaszX" Subject: Re: [PATCH v11 1/6] ethdev: add Tx preparation Date: Thu, 27 Oct 2016 16:29:59 +0000 Message-ID: <3042915272161B4EB253DA4D77EB373A14F45144@IRSMSX102.ger.corp.intel.com> References: <1477327917-18564-1-git-send-email-tomaszx.kulasek@intel.com> <1477486575-25148-1-git-send-email-tomaszx.kulasek@intel.com> <1477486575-25148-2-git-send-email-tomaszx.kulasek@intel.com> <1499338.8Le0ABsxDG@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Thomas Monjalon Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 635382BF0 for ; Thu, 27 Oct 2016 18:31:14 +0200 (CEST) In-Reply-To: <1499338.8Le0ABsxDG@xps13> 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" Hi Thomas, > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Thursday, October 27, 2016 17:01 > To: Kulasek, TomaszX > Cc: dev@dpdk.org; olivier.matz@6wind.com > Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation >=20 > Hi Tomasz, >=20 > This is a major new function in the API and I still have some comments. >=20 > 2016-10-26 14:56, Tomasz Kulasek: > > --- a/config/common_base > > +++ b/config/common_base > > +CONFIG_RTE_ETHDEV_TX_PREP=3Dy >=20 > We cannot enable it until it is implemented in every drivers. >=20 For most of drivers it's safe to enable it by default and if this feature i= s not supported, no checks/modifications are done. In that meaning the proc= essing path is the same as without using Tx preparation.=20 Introducing this macro was discussed in the threads: http://dpdk.org/ml/archives/dev/2016-September/046437.html http://dpdk.org/dev/patchwork/patch/15770/ Short conclusion: Jerin Jacob pointed, that it can have significant impact on some architectu= res (such a low-end ARMv7, ARMv8 targets which may not have PCIE-RC support= and have only integrated NIC controller), even if this feature is not impl= emented. We've added this macro to provide an ability to use NOOP operation and allo= w turn off this feature if will have adverse effect on specific configurati= on/hardware. > > 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 */ >=20 > Could you confirm why tx_pkt_prep is not in dev_ops? > I guess we want to have several implementations? >=20 Yes, the implementation may vary on selected tx_burst path (e.g. vector imp= lementation, simple implementation, full featured, and so on, and can have = another requirements, such a implemented features, performance requirements= for each implementation). The path is chosen based on the application requirements transparently and = we have a pair of callbacks -- tx_burst and corresponding callback (which d= epends directly on tx_burst path). > Shouldn't we have a const struct control_dev_ops and a struct > datapath_dev_ops? >=20 > > +rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf > **tx_pkts, > > + uint16_t nb_pkts) >=20 > The word "prep" can be understood as "prepend". > Why not rte_eth_tx_prepare? >=20 I do not mind. > > +/** > > + * Fix pseudo header checksum > > + * > > + * This function fixes pseudo header checksum for TSO and non-TSO > tcp/udp in > > + * provided mbufs packet data. > > + * > > + * - for non-TSO tcp/udp packets full pseudo-header checksum is counte= d > and set > > + * in packet data, > > + * - for TSO the IP payload length is not included in pseudo header. > > + * > > + * This function expects that used headers are in the first data > segment of > > + * mbuf, are not fragmented and can be safely modified. >=20 > What happens otherwise? >=20 There are requirements for this helper function. For Tx preparation callbac= k we check this requirement and if it fails, -NOTSUP errno is returned. > > + * > > + * @param m > > + * The packet mbuf to be fixed. > > + * @return > > + * 0 if checksum is initialized properly > > + */ > > +static inline int > > +rte_phdr_cksum_fix(struct rte_mbuf *m) >=20 > Could we find a better name for this function? > - About the prefix, rte_ip_ ? > - About the scope, where this phdr_cksum is specified? > Isn't it an intel_phdr_cksum to match what hardware expects? > - About the verb, is it really fixing something broken? > Or just writing into a mbuf? > I would suggest rte_ip_intel_cksum_prepare. Fixes in the meaning of requirements for offloads, which states e.g. that t= o use specific Tx offload we should to fill checksums in a proper way, if n= ot, thee settings are not valid and should be fixed. But you're right, prepare is better word. About the function name, maybe rte_net_intel_chksum_prepare will be better = while it prepares also tcp/udp headers and is placed in rte_net.h? Tomasz