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

[Shahaf Shuler]

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.
I can split the extra cap check if you fill it needs to be in a separate patch. 

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

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

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

> 
> >  }
> >
> >  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 :). 

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

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