[dpdk-dev] [PATCH 1/5] app/testpmd: convert to new Ethdev offloads API

[Shahaf Shuler]

Thursday, December 7, 2017 12:58 AM, Ferruh Yigit:
> 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.

OK will have them splitted on v2.  

> 
> >
> >>
> >>
> >> 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 <shahafs at mellanox.com>
> >>
> >> <...>
> >>
> >>>  	} 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?

Again - because all of the tx offload where enabled by default. This means that the PMDs which supports those offload were using the tx/rx_burst functions which includes the functionally (unless the application disable those offloads using txq_flags). There was no need to futher update the PMD on the Tx offloads being used. 

As for the conflicts between CLI commands and txq flags - small bug in PMD which wasn't discovered probably because no one did some strange configuration. 

> 
> > 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.

The on the flight VLAN configuration was converted as well and supported. I will make it more explicit on v2 with a separate 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.

Yeah,  I will switch to port_id. 
> 
> >
> >>
> >>>  }
> >>>
> >>>  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?

Will provide another patch to remove the ol_flags. 

> 
> >
> >>
> >> 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?

It can. 

> 
> >
> >>
> >> <...>
> >>
> >>> @@ -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.

Am disagree. 
The txq_flags are to disable offloads. From application which uses the new API perspective they are all disabled. With the new offloads API, the application is the one to choose the offloads being used, not the PMD therefore there is no point with converting the PMD txq_flags.
If the underlying PMD is not supporting the new API, the offloads flags selected by the application will be converted into txq_flags. For example 0 will be converted to all of the TXQ_FLAGS. 

It is true that before the changes we could still have some offloads enabled (like TSO) when we use the PMD default txq_flags. However I see it as an improvement and not downside. The new application is better because it don't have offloads enabled for nothing.

> 
> > +			/* 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) &&
> >