From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id C5FD4201 for ; Mon, 4 Dec 2017 23:31:03 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Dec 2017 14:31:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,361,1508828400"; d="scan'208";a="9036910" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.178]) ([10.241.225.178]) by FMSMGA003.fm.intel.com with ESMTP; 04 Dec 2017 14:31:01 -0800 To: Shahaf Shuler , jingjing.wu@intel.com Cc: dev@dpdk.org References: <20171123120804.143897-1-shahafs@mellanox.com> <20171123120804.143897-2-shahafs@mellanox.com> From: Ferruh Yigit Message-ID: Date: Mon, 4 Dec 2017 14:31:00 -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: <20171123120804.143897-2-shahafs@mellanox.com> 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: Mon, 04 Dec 2017 22:31:04 -0000 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. And is this update tested with PMDs both support new offload method and old offload method? 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 ? > @@ -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? 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? > } > > 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. <...> > @@ -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? <...> > @@ -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? And do we still need testpmd tx_ol_flags after these changes? <...> > @@ -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. 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? <...> > @@ -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 "{}" <...> > 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? Flag has two meaning I guess, 1) Ignore bitfield values for port based offload configuration. 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? queue specific ones are copy of port configs. <...>