Re: [dpdk-dev] [PATCH v2 3/5] ethdev: convert remaining apps to new offload API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 07/03/2018 12:27 AM, Thomas Monjalon wrote:
> Some test applications and examples were not converted
> to the new offload API introduced in 17.11.
>
> For reference, see "Hardware Offload" in
> 	doc/guides/prog_guide/poll_mode_drv.rst
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---

<...>

> diff --git a/doc/guides/sample_app_ug/link_status_intr.rst b/doc/guides/sample_app_ug/link_status_intr.rst
> index 8c11ba4ae..c7665fe5c 100644
> --- a/doc/guides/sample_app_ug/link_status_intr.rst
> +++ b/doc/guides/sample_app_ug/link_status_intr.rst
> @@ -137,10 +137,7 @@ The global configuration is stored in a static structure:
>       static const struct rte_eth_conf port_conf = {
>           .rxmode = {
>               .split_hdr_size = 0,
> -            .header_split = 0,   /**< Header Split disabled */
> -            .hw_ip_checksum = 0, /**< IP checksum offload disabled */
> -            .hw_vlan_filter = 0, /**< VLAN filtering disabled */
> -            .hw_strip_crc= 0,    /**< CRC stripped by hardware */
> +            .offloads = DEV_RX_OFFLOAD_CRC_STRIP,

Is it intended that CRC strip was disabled before and now it is becoming 
enabled?

>           },
>           .txmode = {},
>           .intr_conf = {

<...>

> diff --git a/examples/bbdev_app/main.c b/examples/bbdev_app/main.c
> index 254cc0676..045a190b9 100644
> --- a/examples/bbdev_app/main.c
> +++ b/examples/bbdev_app/main.c
> @@ -64,11 +64,7 @@ static const struct rte_eth_conf port_conf = {
>   		.mq_mode = ETH_MQ_RX_NONE,
>   		.max_rx_pkt_len = ETHER_MAX_LEN,
>   		.split_hdr_size = 0,
> -		.header_split = 0, /**< Header Split disabled */
> -		.hw_ip_checksum = 0, /**< IP checksum offload disabled */
> -		.hw_vlan_filter = 0, /**< VLAN filtering disabled */
> -		.jumbo_frame = 0, /**< Jumbo Frame Support disabled */
> -		.hw_strip_crc = 0, /**< CRC stripped by hardware */
> +		.offloads = DEV_RX_OFFLOAD_CRC_STRIP,

Is it intended that CRC strip was disabled before and now it is becoming 
enabled?


>   	},
>   	.txmode = {
>   		.mq_mode = ETH_MQ_TX_NONE,

<...>

> diff --git a/test/test/test_pmd_perf.c b/test/test/test_pmd_perf.c
> index 54bc4f6b0..4549965fc 100644
> --- a/test/test/test_pmd_perf.c
> +++ b/test/test/test_pmd_perf.c
> @@ -65,14 +65,7 @@ static struct rte_eth_conf port_conf = {
>   		.mq_mode = ETH_MQ_RX_NONE,
>   		.max_rx_pkt_len = ETHER_MAX_LEN,
>   		.split_hdr_size = 0,
> -		.header_split   = 0, /**< Header Split disabled */
> -		.hw_ip_checksum = 0, /**< IP checksum offload enabled */
> -		.hw_vlan_filter = 0, /**< VLAN filtering disabled */
> -		.hw_vlan_strip  = 0, /**< VLAN strip enabled. */
> -		.hw_vlan_extend = 0, /**< Extended VLAN disabled. */
> -		.jumbo_frame    = 0, /**< Jumbo Frame Support disabled */
> -		.hw_strip_crc   = 1, /**< CRC stripped by hardware */
> -		.enable_scatter = 0, /**< scatter rx disabled */
> +		.offloads = DEV_RX_OFFLOAD_CRC_STRIP,
>   	},
>   	.txmode = {
>   		.mq_mode = ETH_MQ_TX_NONE,
> @@ -97,11 +90,6 @@ static struct rte_eth_txconf tx_conf = {
>   	},
>   	.tx_free_thresh = 32, /* Use PMD default values */
>   	.tx_rs_thresh = 32, /* Use PMD default values */
> -	.txq_flags = (ETH_TXQ_FLAGS_NOMULTSEGS |
> -		      ETH_TXQ_FLAGS_NOVLANOFFL |
> -		      ETH_TXQ_FLAGS_NOXSUMSCTP |
> -		      ETH_TXQ_FLAGS_NOXSUMUDP |
> -		      ETH_TXQ_FLAGS_NOXSUMTCP)
>   };
>   
>   enum {
> @@ -808,38 +796,29 @@ test_set_rxtx_conf(cmdline_fixed_string_t mode)
>   
>   	if (!strcmp(mode, "vector")) {
>   		/* vector rx, tx */
> -		tx_conf.txq_flags = 0xf01;

I'd say that 100% correct equivalent would be:
tx_conf.offloads &= ~(DEV_TX_OFFLOAD_VLAN_INSERT |
           DEV_TX_OFFLOAD_IPV4_CKSUM | DEV_TX_OFFLOAD_UDP_CKSUM |
           DEV_TX_OFFLOAD_TCP_CKSUM | DEV_TX_OFFLOAD_SCTP_CKSUM |
           DEV_TX_OFFLOAD_MULTI_SEGS);

I guess the function may be called few times with different mode set.
If so, similar fixes should be applied below as well.

>   		tx_conf.tx_rs_thresh = 32;
>   		tx_conf.tx_free_thresh = 32;
> -		port_conf.rxmode.hw_ip_checksum = 0;
> -		port_conf.rxmode.enable_scatter = 0;

port_conf.rxmode.offloads &= ~(DEV_RX_OFFLOAD_CHECKSUM | 
DEV_RX_OFFLOAD_SCATTER);

>   		return 0;
>   	} else if (!strcmp(mode, "scalar")) {
>   		/* bulk alloc rx, full-featured tx */
> -		tx_conf.txq_flags = 0;

I think here we should enable offloads listed above to have 
full-featured Tx:
tx_conf.offloads |=  ...

>   		tx_conf.tx_rs_thresh = 32;
>   		tx_conf.tx_free_thresh = 32;
> -		port_conf.rxmode.hw_ip_checksum = 1;
> -		port_conf.rxmode.enable_scatter = 0;
> +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;

port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_SCATTER;

>   		return 0;
>   	} else if (!strcmp(mode, "hybrid")) {
>   		/* bulk alloc rx, vector tx
>   		 * when vec macro not define,
>   		 * using the same rx/tx as scalar
>   		 */
> -		tx_conf.txq_flags = 0xf01;

As in similar case above.

>   		tx_conf.tx_rs_thresh = 32;
>   		tx_conf.tx_free_thresh = 32;
> -		port_conf.rxmode.hw_ip_checksum = 1;
> -		port_conf.rxmode.enable_scatter = 0;
> +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_CHECKSUM;

As in similar case above

>   		return 0;
>   	} else if (!strcmp(mode, "full")) {
>   		/* full feature rx,tx pair */
> -		tx_conf.txq_flags = 0x0;   /* must condition */

As in similar case above.

>   		tx_conf.tx_rs_thresh = 32;
>   		tx_conf.tx_free_thresh = 32;
> -		port_conf.rxmode.hw_ip_checksum = 0;
> -		port_conf.rxmode.enable_scatter = 1; /* must condition */
> +		port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_SCATTER;

port_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_CHECKSUM;

>   		return 0;
>   	}
>   

In general I think that it would be really good to avoid changes in
behaviour when technical changes are done.