On 6/11/2018 12:00 PM, Shahaf Shuler wrote:

Hi Ferruh,

Thanks for this patch. 

Monday, June 11, 2018 12:10 PM, Ferruh Yigit:

quoted

Subject: Re: [RFC] ethdev: remove all offload API

On 6/9/2018 9:04 AM, Andrew Rybchenko wrote:

quoted

On 06/09/2018 01:41 AM, Ferruh Yigit wrote:

quoted

Cc: Shahaf Shuler <redacted>

Signed-off-by: Ferruh Yigit <redacted>
---

<...>

quoted

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)

quoted

quoted

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

Hi Andrew,

quoted

I have 2 questions here:
 1. Why is jumbo_frame kept? There is

DEV_RX_OFFLOAD_JUMBO_FRAME.

Because there are still some usage of this flag in PMDs, they need to be
clarified before removing the flag. I am for removing the flag in final version,
but for this RFC I am not able to find enough time to work on PMDs for it.

Can you elaborate?
Is this something more than just a replace of the jumbo_frame bit with its corresponding flag? 

That was my concern that copy paste won't be enough because some drivers not
just use the jumbo_frame but set it based on max_rx_pkt_len etc.., that is why
left out .jumbo_frame in the RFC.

quoted

quoted

 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.

Yes it is wrong, app should replace "".hw_strip_crc=1 with KEEP_CRC offload.
Since both are RFC, kind of hard to maintain, but I think good to create a
dependency from this patch to KEEP_CRC one.

quoted

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

discarded.

quoted

<...>

quoted

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,

quoted

quoted

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)

quoted

quoted

 	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,

quoted

quoted

 	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,

quoted

quoted

 	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

quoted

quoted

+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

quoted

quoted

-	 * 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)))

quoted

quoted

+	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

quoted

quoted

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

quoted

quoted

-	 * application. In turn, the absence of ETH_TXQ_FLAGS_NOXSUMTCP

is

quoted

quoted

-	 * associated specifically with a legacy application which expects
-	 * both TCP checksum offload and TSO to be enabled because the

legacy

quoted

quoted

-	 * 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)

Thanks, will remove this too.

quoted

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

+1, +1