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 E663C56A2 for ; Fri, 28 Oct 2016 13:29:29 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 28 Oct 2016 04:29:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,557,1473145200"; d="scan'208";a="895060959" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by orsmga003.jf.intel.com with ESMTP; 28 Oct 2016 04:29:28 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by irsmsx110.ger.corp.intel.com (163.33.3.25) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 28 Oct 2016 12:29:27 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.177]) by IRSMSX156.ger.corp.intel.com ([169.254.3.66]) with mapi id 14.03.0248.002; Fri, 28 Oct 2016 12:29:27 +0100 From: "Ananyev, Konstantin" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation Thread-Index: AQHSL4iZ5WiAYkWG4EqB+e9ej9GU7qC8ViAAgAAd2uD///MYgIAAEtTQ///3j4CAAUaGcA== Date: Fri, 28 Oct 2016 11:29:26 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583F0CEC55@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> In-Reply-To: <2500924.jYNDaNt7Th@xps13> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v11 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, 28 Oct 2016 11:29:30 -0000 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 com= ments. > > > > > > > > > > 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 a= s 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 drive= rs.. > > My bad, missed that part completely. > > Yes, then I suppose for now we'll need to support both (with and withou= t) code paths for testpmd. > > Probably a new fwd mode or just extra parameter for the existing one? > > Any other suggestions? >=20 > 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. 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 prepara= tion itself (as it has to do it now). All existing apps would keep working as expected without using that fun= ction. Though if the app developer knows that for all HW models he plans to ru= n on tx_prep is implemented - he is free to use it. 2) It would be difficult for Tomasz (and other Intel guys) to implement= 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 kn= owledge of it. So we depend here on the good will of other PMD mainaners/developers to= implement tx_prep() for these devices.=20 From other side, if it will be disabled by default, then, I think, PMD developers just wouldn't be motivated to implement it.=20 So it will be left untested and unused forever. =20 >=20 > > > > > > struct rte_eth_dev { > > > > > > eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive func= tion. */ > > > > > > eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit fun= ction. */ > > > > > > + eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepa= re 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 dat= apath_dev_ops? > > > > > > > > That's probably a good idea, but I suppose it is out of scope for t= hat 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 introduc= ed 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. >=20 > Yes it could be a separate patch in the same patchset. 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 function= ality affected. Konstantin