From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 432EB293B for ; Wed, 6 Dec 2017 23:57:33 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Dec 2017 14:57:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,369,1508828400"; d="scan'208";a="9725728" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.226.140]) ([10.241.226.140]) by FMSMGA003.fm.intel.com with ESMTP; 06 Dec 2017 14:57:32 -0800 To: Shahaf Shuler , "jingjing.wu@intel.com" Cc: "dev@dpdk.org" , "Ananyev, Konstantin" References: <20171123120804.143897-1-shahafs@mellanox.com> <20171123120804.143897-2-shahafs@mellanox.com> From: Ferruh Yigit Message-ID: <1be9e697-8e23-2212-ddf1-e04336435f87@intel.com> Date: Wed, 6 Dec 2017 14:57:31 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 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 1/5] app/testpmd: convert to new Ethdev offloads 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: Wed, 06 Dec 2017 22:57:34 -0000 On 12/4/2017 10:39 PM, Shahaf Shuler wrote: > Tuesday, December 5, 2017 12:31 AM, Ferruh Yigit: >> On 11/23/2017 4:08 AM, Shahaf Shuler wrote: >>> Ethdev Rx/Tx offloads API has changed since: >>> >>> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") commit >>> cba7f53b717d ("ethdev: introduce Tx queue offloads API") >>> >>> Convert the application to use the new API. >> >> Hi Shahaf, >> >> As far as I can see patch does a few things: >> 1- Convert rxmode bitfields usage to rxmode offloads usage. >> 2- Apply some config options to port config and add some port config checks. >> 3- Adding device advertised capability checks for some offloads. >> >> Would you mind separate 2 and 3 to their own patches, with that 1 should be >> straightforward and 2 & 3 will be easy to review. > > See below comments. #2 on your list is actually needed for the conversion patch. Please see below comment [1] for it. > I can split the extra cap check if you fill it needs to be in a separate patch. I find it hard to review this patch, splitting is to make easier to understand the changes, there is no extra need. > >> >> >> And is this update tested with PMDs both support new offload method and >> old offload method? > > It was tested with mlx5 and mlx4 PMD after the conversion to the new APIs. > I put this series early so everyone can try, test and report if something is broken. > I will try to do more testing with the old mlx PMD. > > BTW - we agreed that we set DD for all PMDs to move to the new API by 18.02. I still haven't see patches for that from the rest.> >> >> Thanks, >> ferruh >> >>> >>> Signed-off-by: Shahaf Shuler >> >> <...> >> >>> } else if (!strcmp(res->name, "hw-vlan-extend")) { >>> if (!strcmp(res->value, "on")) >>> - rx_mode.hw_vlan_extend = 1; >>> + rx_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND; >> >> Not related to this patch, but since you are touching these, what is the >> difference between DEV_RX_OFFLOAD_VLAN_EXTEND and >> DEV_RX_OFFLOAD_QINQ_STRIP ? > > Good question, I could not figure it out either. > I guess those are identical. In the old API the hw_vlan_extend was the offload and the DEV_RX_OFFLOAD_QINQ_STRIP was the cap. > Now that we merged them we have duplication. > > From one side, using DEV_RX_OFFLOAD_VLAN_EXTEND is more intuitive for application which previously used hw_vlan_extend to set the offload, > For the other side QINQ_STRIP is more explicit with respect to what the feature does, and it is the flag which is currently being used for PMDs. > > So when we will change it, I guess I am in favor of the QINQ flag. +1 to QINQ. > >> >>> @@ -3434,7 +3439,14 @@ cmd_tx_vlan_set_parsed(void *parsed_result, >> { >>> struct cmd_tx_vlan_set_result *res = parsed_result; >>> >>> + if (!all_ports_stopped()) { >>> + printf("Please stop all ports first\n"); >>> + return; >>> + } >> >> rte_eth_rxmode bitfields to "offloads" conversion is mostly straightforward, >> but is above kind of modifications part of this conversion or are you adding >> missing checks? > > It is part of the conversion and this is related to testpmd design. > > Previously in the old API the tx offloads were set even after the port is started. For this specific example the vlan insert just changed a flag (TESTPMD_TX_OFFLOAD_INSERT_VLAN) on the application port struct to use PKT_TX_VLAN_PKT in the mbuf ol_flags. The application didn't update the PMD in anyway. In fact, it was possible to put txq_flag which says no vlan insertion and then to set vlan insertion through the CLI. As you said, so I am a little concerned if missing something, because otherwise how this was working previously? > This was OK back then because all of the Tx offloads were set by default and we assumed users know what they are doing when setting the txq_flags. > > Now all the tx offloads are disabled by default. Every Tx offload being used should update the PMD (as it may need to switch tx burst function). This is why the Tx offloads configuration must be done when the port is stopped, and be followed with reconfiguration of the device and the queues. [1] Not agree on this part. Indeed you are fixing a behavior of the testpmd, not just converting to new method. Technically you can provide same old config with new flags, like enable all tx offloads, so behavior should be same. Later can fix testpmd in another patch, this gives a better separation. And for example I remember Konstatin mentioned some Intel NICs can accept vlan related configuration updates without stopping the forwarding, we can discuss these kind of things in this fix patch. Just suggesting removing straightforward bitfield to offloads bitwise changes out of way, and focus on what logic is changing. > >> >> I would prefer making only conversion related changes in this patch, and >> extra improvements in other patch. >> >>> + >>> tx_vlan_set(res->port_id, res->vlan_id); >>> + >>> + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1); >> >> Is this required for converting bitfield to offloads usage? > > Yes, see above. btw, why RTE_PORT_ALL, just providing a port_id also should be OK. > >> >>> } >>> >>> cmdline_parse_token_string_t cmd_tx_vlan_set_tx_vlan = @@ -3481,7 >>> +3493,14 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result, { >>> struct cmd_tx_vlan_set_qinq_result *res = parsed_result; >>> >>> + if (!all_ports_stopped()) { >>> + printf("Please stop all ports first\n"); >>> + return; >>> + } >> >> Same for all occurrence of these updates. >> >> <...> >> >>> @@ -3693,22 +3724,34 @@ cmd_csum_parsed(void *parsed_result, >>> >>> if (!strcmp(res->proto, "ip")) { >>> mask = TESTPMD_TX_OFFLOAD_IP_CKSUM; >>> + csum_offloads |= DEV_TX_OFFLOAD_IPV4_CKSUM; >>> } else if (!strcmp(res->proto, "udp")) { >>> mask = TESTPMD_TX_OFFLOAD_UDP_CKSUM; >>> + csum_offloads |= DEV_TX_OFFLOAD_UDP_CKSUM; >>> } else if (!strcmp(res->proto, "tcp")) { >>> mask = TESTPMD_TX_OFFLOAD_TCP_CKSUM; >>> + csum_offloads |= DEV_TX_OFFLOAD_TCP_CKSUM; >>> } else if (!strcmp(res->proto, "sctp")) { >>> mask = TESTPMD_TX_OFFLOAD_SCTP_CKSUM; >>> + csum_offloads |= DEV_TX_OFFLOAD_SCTP_CKSUM; >>> } else if (!strcmp(res->proto, "outer-ip")) { >>> mask = TESTPMD_TX_OFFLOAD_OUTER_IP_CKSUM; >>> + csum_offloads |= >> DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM; >>> } >>> >>> - if (hw) >>> + if (hw) { >>> ports[res->port_id].tx_ol_flags |= mask; >>> - else >>> + ports[res->port_id].dev_conf.txmode.offloads |= >>> + csum_offloads; >> >> So you are updating port config as well as testpmd internal configuration, >> again I guess this is not related to conversion to offloads usage. > > It is. See above. > The port config will be used for the reconfiguration of the device and queues. This is a must for the Tx offloads . > >> >> <...> >> >>> @@ -13017,6 +13093,13 @@ cmd_set_macsec_offload_on_parsed( >>> >>> switch (ret) { >>> case 0: >>> + rte_eth_dev_info_get(port_id, &dev_info); >>> + if ((dev_info.tx_offload_capa & >>> + DEV_TX_OFFLOAD_MACSEC_INSERT) == 0) { >>> + printf("Warning: macsec insert enabled but not " >>> + "supported by port %d\n", port_id); >>> + } >> >> This also adding another layer of check if device advertise requested >> capability, this is an improvement independent from conversion, can you >> please separate into its own patch? > > Yes we can do it. > >> >> <...> >> >>> @@ -606,8 +616,8 @@ port_offload_cap_display(portid_t port_id) >>> >>> if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_VLAN_INSERT) { >>> printf("VLAN insert: "); >>> - if (ports[port_id].tx_ol_flags & >>> - TESTPMD_TX_OFFLOAD_INSERT_VLAN) >> >> This is removing testpmd local config check, just to double check if all places >> that updates this local config covered to update device config variable? > > I hope so. If you find something I missed let me know :). If you are not sure, and if you are not removing tx_ol_flags, what is the benefit of replacing? Whoever does the work removing tx_ol_flags can do replacing, this also ensures all instances updated, no? > >> >> And do we still need testpmd tx_ol_flags after these changes? > > Currently those flags are being used. One can prepare another patch to remove those and use the port config flags instead. > This is not related to the conversion and could be a nice cleanup. > >> >> <...> >> >>> @@ -1658,7 +1679,8 @@ rxtx_config_display(void) >>> printf(" %s packet forwarding%s - CRC stripping %s - " >>> "packets/burst=%d\n", cur_fwd_eng->fwd_mode_name, >>> retry_enabled == 0 ? "" : " with retry", >>> - rx_mode.hw_strip_crc ? "enabled" : "disabled", >>> + (ports[0].dev_conf.rxmode.offloads & >> DEV_RX_OFFLOAD_CRC_STRIP) ? >>> + "enabled" : "disabled", >> >> There is a global config option in testpmd, for all ports. Previous log was print >> based on that config option, but now you are printing the value of first port. > > Not exactly (there are multiple wrong issues with this function). For example the next lines are: > > if (cur_fwd_eng == &tx_only_engine || cur_fwd_eng == &flow_gen_engine) > printf(" packet len=%u - nb packet segments=%d\n", > (unsigned)tx_pkt_length, (int) tx_pkt_nb_segs); > > struct rte_eth_rxconf *rx_conf = &ports[0].rx_conf; > struct rte_eth_txconf *tx_conf = &ports[0].tx_conf; > > the last were added by commit f2c5125a686a ("app/testpmd: use default Rx/Tx port configuration"). > > As you can see port[0] is the one being used for the rest of the configuration print. > >> >> I believe it is wrong to display only first port values, either log can be updated >> to say testpmd default configs, or remove completely, or print for all ports, >> what do you think? > > IMO it is the best to print for all ports. +1, can this fix be part of this patchset although it is not directly related to the offload conversion? > >> >> <...> >> >>> @@ -897,34 +900,31 @@ launch_args_parse(int argc, char** argv) >>> } >>> #endif >>> if (!strcmp(lgopts[opt_idx].name, "disable-crc- >> strip")) >>> - rx_mode.hw_strip_crc = 0; >>> + rx_offloads &= >> ~DEV_RX_OFFLOAD_CRC_STRIP; >>> if (!strcmp(lgopts[opt_idx].name, "enable-lro")) >>> - rx_mode.enable_lro = 1; >>> - if (!strcmp(lgopts[opt_idx].name, "enable-scatter")) >>> - rx_mode.enable_scatter = 1; >>> + rx_offloads |= DEV_RX_OFFLOAD_TCP_LRO; >>> + if (!strcmp(lgopts[opt_idx].name, "enable-scatter")) >> { >>> + rx_offloads |= DEV_RX_OFFLOAD_SCATTER; >>> + } >> >> Can drop "{}" > > Yes. > >> >> <...> >> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index >>> c3ab44849..e3a7c26b8 100644 >>> --- a/app/test-pmd/testpmd.c >>> +++ b/app/test-pmd/testpmd.c >>> @@ -338,15 +338,10 @@ lcoreid_t latencystats_lcore_id = -1; >>> */ >>> struct rte_eth_rxmode rx_mode = { >>> .max_rx_pkt_len = ETHER_MAX_LEN, /**< Default maximum frame >> length. */ >>> - .split_hdr_size = 0, >>> - .header_split = 0, /**< Header Split disabled. */ >>> - .hw_ip_checksum = 0, /**< IP checksum offload disabled. */ >>> - .hw_vlan_filter = 1, /**< VLAN filtering enabled. */ >>> - .hw_vlan_strip = 1, /**< VLAN strip enabled. */ >>> - .hw_vlan_extend = 0, /**< Extended VLAN disabled. */ >>> - .jumbo_frame = 0, /**< Jumbo Frame Support disabled. */ >>> - .hw_strip_crc = 1, /**< CRC stripping by hardware enabled. */ >>> - .hw_timestamp = 0, /**< HW timestamp enabled. */ >>> + .offloads = (DEV_RX_OFFLOAD_VLAN_FILTER | >>> + DEV_RX_OFFLOAD_VLAN_STRIP | >>> + DEV_RX_OFFLOAD_CRC_STRIP), >>> + .ignore_offload_bitfield = 1, /**< Use rte_eth_rxq_conf offloads API >>> +*/ >> >> Is comment correct? > > No I should remove it. > >> Flag has two meaning I guess, >> 1) Ignore bitfield values for port based offload configuration. > > It is only this meaning. > >> 2) For rxq, use rx_conf.offloads field. >> >> testpmd is still using port based offload (rte_eth_rxmode) but "offloads" >> variable instead of bitfields, right? > > Right. > > queue specific ones are copy of port >> configs. >> >> <...> > @@ -1495,6 +1490,10 @@ start_port(portid_t pid) > } > if (port->need_reconfig_queues > 0) { > port->need_reconfig_queues = 0; > + /* Use rte_eth_txq_conf offloads API */ > + port->tx_conf.txq_flags = ETH_TXQ_FLAGS_IGNORE; Also I just catch this part, during app start txq_flags set via PMD default values (rxtx_port_config), if you overwrite this flag without converting to offloads, for PMDs supporting old method you are loosing configuration. > + /* Apply Tx offloads configuration */ > + port->tx_conf.offloads = port->dev_conf.txmode.offloads; > /* setup tx queues */ > for (qi = 0; qi < nb_txq; qi++) { > if ((numa_support) && > @@ -1521,6 +1520,8 @@ start_port(portid_t pid) > port->need_reconfig_queues = 1; > return -1; > } > + /* Apply Rx offloads configuration */ > + port->rx_conf.offloads = port->dev_conf.rxmode.offloads; > /* setup rx queues */ > for (qi = 0; qi < nb_rxq; qi++) { > if ((numa_support) && >