Re: [dpdk-dev] [RFC] ethdev: remove all offload API

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 06/09/2018 01:41 AM, Ferruh Yigit wrote:
> Cc: Shahaf Shuler <shahafs@mellanox.com>
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---

<...>

> diff --git a/app/test-eventdev/test_perf_common.c b/app/test-eventdev/test_perf_common.c
> index d00f91802..9fe042ffe 100644
> --- a/app/test-eventdev/test_perf_common.c
> +++ b/app/test-eventdev/test_perf_common.c
> @@ -680,13 +680,7 @@ perf_ethdev_setup(struct evt_test *test, struct evt_options *opt)
>   			.mq_mode = ETH_MQ_RX_RSS,
>   			.max_rx_pkt_len = ETHER_MAX_LEN,
>   			.split_hdr_size = 0,
> -			.header_split   = 0,
> -			.hw_ip_checksum = 0,
> -			.hw_vlan_filter = 0,
> -			.hw_vlan_strip  = 0,
> -			.hw_vlan_extend = 0,
>   			.jumbo_frame    = 0,
> -			.hw_strip_crc   = 1,

I have 2 questions here:
  1. Why is jumbo_frame kept? There is DEV_RX_OFFLOAD_JUMBO_FRAME.
  2. Why is hw_strip_crc=1 discarded? Yes, I remember plans to make it
      default behaviour and introduce flag to keep CRC, but right now the
      patch looks like mixture of two changes which is not good.

There are more cases in the patch where hw_strip_crc=1 is simply discarded.

<...>

> diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index 1b6499f85..ee8ae5b9f 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -1089,7 +1089,6 @@ sfc_tx_queue_info_get(struct rte_eth_dev *dev, uint16_t tx_queue_id,
>   
>   	memset(qinfo, 0, sizeof(*qinfo));
>   
> -	qinfo->conf.txq_flags = txq_info->txq->flags;
>   	qinfo->conf.offloads = txq_info->txq->offloads;
>   	qinfo->conf.tx_free_thresh = txq_info->txq->free_thresh;
>   	qinfo->conf.tx_deferred_start = txq_info->deferred_start;
> diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
> index cc76a5b15..58a0df583 100644
> --- a/drivers/net/sfc/sfc_rx.c
> +++ b/drivers/net/sfc/sfc_rx.c
> @@ -1446,7 +1446,6 @@ sfc_rx_check_mode(struct sfc_adapter *sa, struct rte_eth_rxmode *rxmode)
>   	if (~rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
>   		sfc_warn(sa, "FCS stripping cannot be disabled - always on");
>   		rxmode->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> -		rxmode->hw_strip_crc = 1;
>   	}
>   
>   	return rc;
> diff --git a/drivers/net/sfc/sfc_tx.c b/drivers/net/sfc/sfc_tx.c
> index 1bcc2c697..6d42a1a65 100644
> --- a/drivers/net/sfc/sfc_tx.c
> +++ b/drivers/net/sfc/sfc_tx.c
> @@ -171,7 +171,6 @@ sfc_tx_qinit(struct sfc_adapter *sa, unsigned int sw_index,
>   	txq->free_thresh =
>   		(tx_conf->tx_free_thresh) ? tx_conf->tx_free_thresh :
>   		SFC_TX_DEFAULT_FREE_THRESH;
> -	txq->flags = tx_conf->txq_flags;
>   	txq->offloads = offloads;
>   
>   	rc = sfc_dma_alloc(sa, "txq", sw_index, EFX_TXQ_SIZE(txq_info->entries),
> @@ -182,7 +181,6 @@ sfc_tx_qinit(struct sfc_adapter *sa, unsigned int sw_index,
>   	memset(&info, 0, sizeof(info));
>   	info.max_fill_level = txq_max_fill_level;
>   	info.free_thresh = txq->free_thresh;
> -	info.flags = tx_conf->txq_flags;
>   	info.offloads = offloads;
>   	info.txq_entries = txq_info->entries;
>   	info.dma_desc_size_max = encp->enc_tx_dma_desc_size_max;
> @@ -431,18 +429,10 @@ sfc_tx_qstart(struct sfc_adapter *sa, unsigned int sw_index)
>   	if (rc != 0)
>   		goto fail_ev_qstart;
>   
> -	/*
> -	 * The absence of ETH_TXQ_FLAGS_IGNORE is associated with a legacy
> -	 * application which expects that IPv4 checksum offload is enabled
> -	 * all the time as there is no legacy flag to turn off the offload.
> -	 */
> -	if ((txq->offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) ||
> -	    (~txq->flags & ETH_TXQ_FLAGS_IGNORE))
> +	if (txq->offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
>   		flags |= EFX_TXQ_CKSUM_IPV4;
>   
> -	if ((txq->offloads & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM) ||
> -	    ((~txq->flags & ETH_TXQ_FLAGS_IGNORE) &&
> -	     (offloads_supported & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)))
> +	if (txq->offloads & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)
>   		flags |= EFX_TXQ_CKSUM_INNER_IPV4;
>   
>   	if ((txq->offloads & DEV_TX_OFFLOAD_TCP_CKSUM) ||
> @@ -453,16 +443,7 @@ sfc_tx_qstart(struct sfc_adapter *sa, unsigned int sw_index)
>   			flags |= EFX_TXQ_CKSUM_INNER_TCPUDP;
>   	}
>   
> -	/*
> -	 * The absence of ETH_TXQ_FLAGS_IGNORE is associated with a legacy
> -	 * application. In turn, the absence of ETH_TXQ_FLAGS_NOXSUMTCP is
> -	 * associated specifically with a legacy application which expects
> -	 * both TCP checksum offload and TSO to be enabled because the legacy
> -	 * API does not provide a dedicated mechanism to control TSO.
> -	 */
> -	if ((txq->offloads & DEV_TX_OFFLOAD_TCP_TSO) ||
> -	    ((~txq->flags & ETH_TXQ_FLAGS_IGNORE) &&
> -	     (~txq->flags & ETH_TXQ_FLAGS_NOXSUMTCP)))
> +	if (txq->offloads & DEV_TX_OFFLOAD_TCP_TSO)
>   		flags |= EFX_TXQ_FATSOV2;
>   
>   	rc = efx_tx_qcreate(sa->nic, sw_index, 0, &txq->mem,

net/sfc changes looks good.
Plus 'struct sfc_txq -> flags' (drivers/net/sfc/sfc_tx.h) and
'struct sfc_dp_tx_qcreate_info -> flags' (drivers/net/sfc/sfc_dp_tx.h) 
should be
removed since there are not used now.

If finally rxmode.jumbo_frame is removed, it should removed from
net/sfc as well (but compiler will help to find it in any case).

After applying the patch:
$ git grep ETH_TXQ_FLAGS
drivers/net/fm10k/fm10k.h:#define FM10K_SIMPLE_TX_FLAG 
((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
drivers/net/fm10k/fm10k.h: ETH_TXQ_FLAGS_NOOFFLOADS)

In general I think that we should do it ASAP. Also it will guarantee
that new PMDs do not use corresponding structure members etc.