From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v12 1/6] ethdev: add Tx preparation Date: Fri, 02 Dec 2016 00:50:31 +0100 Message-ID: <4969291.OX96oIJoy2@xps13> References: <1477486575-25148-1-git-send-email-tomaszx.kulasek@intel.com> <2505996.o0gdCe9Hsd@xps13> <3042915272161B4EB253DA4D77EB373A14F57CDE@IRSMSX102.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, "Ananyev, Konstantin" , olivier.matz@6wind.com, "Richardson, Bruce" To: "Kulasek, TomaszX" Return-path: Received: from mail-wj0-f176.google.com (mail-wj0-f176.google.com [209.85.210.176]) by dpdk.org (Postfix) with ESMTP id 6ED694A59 for ; Fri, 2 Dec 2016 00:50:33 +0100 (CET) Received: by mail-wj0-f176.google.com with SMTP id xy5so218354437wjc.0 for ; Thu, 01 Dec 2016 15:50:33 -0800 (PST) In-Reply-To: <3042915272161B4EB253DA4D77EB373A14F57CDE@IRSMSX102.ger.corp.intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2016-12-01 22:31, Kulasek, TomaszX: > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > 2016-12-01 19:20, Kulasek, TomaszX: > > > Hi Thomas, > > > > > > Sorry, I have answered for this question in another thread and I missed > > about this one. Detailed answer is below. > > > > Yes you already gave this answer. > > And I will continue asking the question until you understand it. > > > > > > 2016-11-28 11:54, Thomas Monjalon: > > > > > Hi, > > > > > > > > > > 2016-11-23 18:36, Tomasz Kulasek: > > > > > > --- a/config/common_base > > > > > > +++ b/config/common_base > > > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024 > > > > > > CONFIG_RTE_LIBRTE_IEEE1588=n > > > > > > CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16 > > > > > > CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y > > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y > > > > > > > > > > Please, remind me why is there a configuration here. > > > > > It should be the responsibility of the application to call > > > > > tx_prepare or not. If the application choose to use this new API > > > > > but it is disabled, then the packets won't be prepared and there is > > no error code: > > > > > > > > > > > +#else > > > > > > + > > > > > > +static inline uint16_t > > > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused > > > > uint16_t queue_id, > > > > > > + __rte_unused struct rte_mbuf **tx_pkts, uint16_t > > > > > > +nb_pkts) { > > > > > > + return nb_pkts; > > > > > > +} > > > > > > + > > > > > > +#endif > > > > > > > > > > So the application is not aware of the issue and it will not use > > > > > any fallback. > > > > > > tx_prepare mechanism can be turned off by compilation flag (as discussed > > with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real > > NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory > > dereference and check can have significant impact on performance). > > > > > > Jerin observed that on some architectures (e.g. low-end ARM with > > embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause > > significant performance drop, so he proposed to introduce this > > configuration flag to provide real NOOP when tx_prepare functionality is > > not required, and can be turned on based on the _target_ configuration. > > > > > > For other cases, when this flag is turned on (by default), and > > tx_prepare is not implemented, functional NOOP is used based on comparison > > (dev->tx_pkt_prepare == NULL). > > > > So if the application call this function and if it is disabled, it simply > > won't work. Packets won't be prepared, checksum won't be computed. > > > > I give up, I just NACK. > > It is not to be turned on/off whatever someone wants, but only and only for the case, when platform developer knows, that his platform doesn't need this callback, so, he may turn off it and then save some performance (this option is per target). How may he know? There is no comment in the config file, no documentation. > For this case, the behavior of tx_prepare will be exactly the same when it is turned on or off. If is not the same, there's no sense to turn it off. There were long topic, where we've tried to convince you, that it should be turned on for all devices. Really? You tried to convince me to turn it on? No you were trying to convince Jerin. I think it is a wrong idea to allow disabling this function. I didn't comment in first discussion because Jerin told it was really important for small hardware with fixed NIC, and I thought it would be implemented in a way the application cannot be misleaded. The only solution I see here is to add some comments in the configuration file, below the #else and in the doc. Have you checked doc/guides/prog_guide/poll_mode_drv.rst?