Re: [RFC] ethdev: remove all offload API

From: Shahaf Shuler <hidden>
Date: 2018-06-11 11:35:58

Monday, June 11, 2018 2:26 PM, Ananyev, Konstantin:

Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [RFC] ethdev: remove all offload API

quoted

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

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

quoted

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

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

quoted

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

corresponding flag?

quoted

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.

I understand the max_rx_pkt_len consideration, but in the end there is some logic which decide if to set/not the jumbo_frame bit. I would expect it just to replace it with the JUMBO_FRAME flag w/o touching the rest of the logic.

Can you refer to some specific file which you encounter it?

But max_rx_pkt_len need to be remained (and properly processed anyway)
no?
BTW, I always wonder is there any reason to have jumbo_frame flag at all (as
we do have max_rx_pkt_len anyway)?

I share the same thought, and it was expressed in multiple threads. There is more - {MTU, SCATTER, JUMBO_FRAME}, all are connected and depended only on max_rx_pkt_len and mbuf size. 
However I think it is too much work to include into this RFC and completely orthogonal.

Konstantin

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.

quoted

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

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

quoted

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

 	if (~rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
 		sfc_warn(sa, "FCS stripping cannot be disabled -

always on");

quoted

 		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

 	txq->free_thresh =
 		(tx_conf->tx_free_thresh) ? tx_conf-

tx_free_thresh :

quoted

 		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

 	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;

quoted

@@ -431,18

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

quoted

legacy

quoted

-	 * application which expects that IPv4 checksum offload is

enabled

quoted

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

quoted

-	    ((~txq->flags & ETH_TXQ_FLAGS_IGNORE) &&
-	     (offloads_supported &

DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)))

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

quoted

+443,7

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

quoted

quoted
quoted
legacy
quoted
quoted
-	 * application. In turn, the absence of

ETH_TXQ_FLAGS_NOXSUMTCP

quoted

is

quoted

-	 * associated specifically with a legacy application which

expects

quoted

quoted
quoted
quoted
quoted
-	 * both TCP checksum offload and TSO to be enabled

because the

quoted

legacy

quoted

-	 * API does not provide a dedicated mechanism to control

TSO.

quoted

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

quoted

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.

quoted

quoted
quoted
+1, +1

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help