From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 724643772 for ; Mon, 15 Jan 2018 18:45:09 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Jan 2018 09:45:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,364,1511856000"; d="scan'208";a="10508812" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by fmsmga008.fm.intel.com with ESMTP; 15 Jan 2018 09:45:07 -0800 To: Maciej Czekaj , dev@dpdk.org Cc: shahafs@mellanox.com, thomas@monjalon.net References: <1514985137-2653-1-git-send-email-maciej.czekaj@caviumnetworks.com> <5795a502-77cf-2b33-a723-bb7b43ec6d82@intel.com> From: Ferruh Yigit Message-ID: Date: Mon, 15 Jan 2018 17:45:06 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] net/thunderx: Convert ThunderX VNIC PMD to new offload API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Jan 2018 17:45:09 -0000 On 1/15/2018 2:49 PM, Maciej Czekaj wrote: > > > > > -- Oryginal message -- >> On 1/3/2018 1:12 PM, maciej.czekaj@caviumnetworks.com wrote: >>> From: Maciej Czekaj >>> >>> This patch removes all references to old-style offload API >>> replacing them with new offload flags. >>> >>> Signed-off-by: Maciej Czekaj >> <...> >> >>> >>> dev_info->default_txconf = (struct rte_eth_txconf) { >>> .tx_free_thresh = NICVF_DEFAULT_TX_FREE_THRESH, >>> - .txq_flags = >>> - ETH_TXQ_FLAGS_NOMULTSEGS | >>> - ETH_TXQ_FLAGS_NOREFCOUNT | >>> - ETH_TXQ_FLAGS_NOMULTMEMP | >>> - ETH_TXQ_FLAGS_NOVLANOFFL | >>> - ETH_TXQ_FLAGS_NOXSUMSCTP, >>> + .txq_flags = ETH_TXQ_FLAGS_IGNORE, >> I am not sure about this, Shahafs may comment better, shouldn't application >> decide to set "c" or not, instead of having this in default >> configuration? > I think of it as a safeguard against a legacy application that is using > old-style txq_flags. > > There is an assymetry in API, since >  - rte_eth_tx_queue setup() and rte_eth_rx_queue_setup() convert flags >  - rte_eth_dev_info_get() - does not convert them. > > The scenario leading to issues is as follows: > > 1.Application reads default txq_flags from the rte_eth_dev_info_get(). >  - dev_info.txconf.offloads contains flags but it is ignored by legacy code >  - dev_info.txq_flags may be: >       a) txq_flags == 0 >       b) txq_flags ==  ETH_TXQ_FLAGS_IGNORE >       c) txq_flags == new-style flags converted to old-style flags > > > 2. Application uses default txq_flags field to rte_eth_tx_queue_setup(). > Now, depending on txq_flags: > >   a) txq_flags == 0, ethdev layer __clears__ all offloads, see * >   b) txq_flags ==  ETH_TXQ_FLAGS_IGNORE, ethdev layer converts offloads > to txq_flags, but leaves orignal offloads, so PMD can still use them >   c) txq_flags == old-style flags, ethdev layer converts txq_flags to > offloads, destroying default offloads > > > * relevant code snippet from rte_eth_tx_queue_setup() with comments: > >     if (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) { >         // ==> converts offloads to txq_flags but LEAVES offloads, too >         rte_eth_convert_txq_offloads(tx_conf->offloads, >                          &local_conf.txq_flags); >         /* Keep the ignore flag. */ >         local_conf.txq_flags |= ETH_TXQ_FLAGS_IGNORE; >     } else { >         // ==> converts txq_flags to offloads but DESTROYS original > offloads > rte_eth_convert_txq_flags(tx_conf->txq_flags, >                       &local_conf.offloads); >     } > > > So, out of 3 options: > > a) does not work for legacy code > b) will work for legacy code > c) will work but defeats the purpose of removing old-style flags, since > dev_info() callback has to setup both old-style and new-style default flags > > I chose b) as the simplest way to work-around the issue. I could post a > patch to ethdev API if you think it is important. What I understand is txq_flags should be supported because of legacy apps. That is why ethdev layer converts old txq_flags to offloads when new ETH_TXQ_FLAGS_IGNORE flag is missing. So that PMD can only use "offloads" variable for both legacy and new applications. To support the case application uses defaults from PMD, the one you mentioned above, I think we should go with option c). For implementation you can use only new "offloads", so it is not both old-style and new-style, just new-style, but as default yes should keep both. And new applications will set ETH_TXQ_FLAGS_IGNORE to say "offloads" has valid information. Currently this also converted to the txq_flags but this will go away when all PMDs support new method. Think about a case where a legacy application gets defaults from PMD and sets a few more values in txq_flags and configure the device. When you set ETH_TXQ_FLAGS_IGNORE as part of defaults, ethdev layer will think only "offloads" is valid and it will overwrite the txq_flags value with offloads one and the updates to the txq_flags will be lost. At one point ETH_TXQ_FLAGS_IGNORE will also go away and applications also will need to support new method. When it is removed than we can get rid of the txq_flags defaults from PMDs until than I guess we need to live with them. > >> >> <...> >> >>> + if ((conf_tx_offloads & tx_offload_capa) != conf_tx_offloads) { >>> + PMD_INIT_LOG(ERR, "Some Tx offloads are not supported " >>> + "requested 0x%lx supported 0x%lx\n", >>> + conf_tx_offloads, tx_offload_capa); >> This is broken for 32bits, using PRIx64 instead of "lx" makes your code more >> portable. > Will fix in v2. > > >