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

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


And is this update tested with PMDs both support new offload method and old
offload method?

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 ?

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

<...>