From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
Jerin Jacob <jerin.jacob@caviumnetworks.com>,
Wenzhuo Lu <wenzhuo.lu@intel.com>,
"Jingjing Wu" <jingjing.wu@intel.com>,
Bernard Iremonger <bernard.iremonger@intel.com>,
John McNamara <john.mcnamara@intel.com>,
Marko Kovacevic <marko.kovacevic@intel.com>,
Neil Horman <nhorman@tuxdriver.com>,
Remy Horton <remy.horton@intel.com>, Ori Kam <orika@mellanox.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Pablo de Lara <pablo.de.lara.guarch@intel.com>,
Radu Nicolau <radu.nicolau@intel.com>,
Akhil Goyal <akhil.goyal@nxp.com>,
Tomasz Kantecki <tomasz.kantecki@intel.com>,
Harry van Haaren <harry.van.haaren@intel.com>,
Jijiang Liu <jijiang.liu@intel.com>,
Ravi Kumar <ravi1.kumar@amd.com>,
Konstantin Ananyev <konstantin.ananyev@intel.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
Tiwei Bie <tiwei.bie@intel.com>, Yong Wang <yongwang@vmware.com>,
Amr Mokhtar <amr.mokhtar@intel.com>,
Declan Doherty <declan.doherty@intel.com>,
"Chas Williams" <chas3@att.com>,
David Hunt <david.hunt@intel.com>,
"Cristian Dumitrescu" <cristian.dumitrescu@intel.com>,
Anatoly Burakov <anatoly.burakov@intel.com>,
Reshma Pattan <reshma.pattan@intel.com>,
"Byron Marohn" <byron.marohn@intel.com>,
Thomas Monjalon <thomas@monjalon.net>
Cc: <dev@dpdk.org>, Shahaf Shuler <shahafs@mellanox.com>
Subject: Re: [dpdk-dev] [RFC] ethdev: remove all offload API
Date: Sat, 9 Jun 2018 11:04:24 +0300 [thread overview]
Message-ID: <553fb3e1-3ae4-d682-17cc-f7b894b0c285@solarflare.com> (raw)
In-Reply-To: <20180608224141.42730-1-ferruh.yigit@intel.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.
next prev parent reply other threads:[~2018-06-09 8:04 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-08 22:41 Ferruh Yigit
2018-06-08 21:52 ` Ferruh Yigit
2018-06-09 8:04 ` Andrew Rybchenko [this message]
2018-06-11 9:09 ` Ferruh Yigit
2018-06-11 11:00 ` Shahaf Shuler
2018-06-11 11:18 ` Ferruh Yigit
2018-06-11 11:26 ` Ananyev, Konstantin
2018-06-11 11:35 ` Ferruh Yigit
2018-06-11 11:35 ` Shahaf Shuler
2018-06-11 12:12 ` Ferruh Yigit
2018-06-29 1:11 ` Thomas Monjalon
2018-07-02 21:27 ` [dpdk-dev] [PATCH v2 0/5] remove old ethdev " Thomas Monjalon
2018-07-02 21:27 ` [dpdk-dev] [PATCH v2 1/5] doc: remove code from KNI example guide Thomas Monjalon
2018-07-02 21:27 ` [dpdk-dev] [PATCH v2 2/5] test: remove unused configuration for bonding Thomas Monjalon
2018-07-02 21:27 ` [dpdk-dev] [PATCH v2 3/5] ethdev: convert remaining apps to new offload API Thomas Monjalon
2018-07-04 11:16 ` Andrew Rybchenko
2018-07-04 12:26 ` Thomas Monjalon
2018-07-04 12:52 ` Andrew Rybchenko
2018-07-02 21:27 ` [dpdk-dev] [PATCH v2 4/5] net/fm10k: remove unused constant Thomas Monjalon
2018-07-02 21:27 ` [dpdk-dev] [PATCH v2 5/5] ethdev: remove old offload API Thomas Monjalon
2018-07-03 12:28 ` Shahaf Shuler
2018-07-04 11:31 ` Andrew Rybchenko
2018-07-03 18:37 ` [dpdk-dev] [PATCH v2 0/5] remove old ethdev " Ferruh Yigit
2018-07-04 18:56 ` Ferruh Yigit
2018-07-02 21:34 ` [dpdk-dev] [RFC] ethdev: remove all " Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=553fb3e1-3ae4-d682-17cc-f7b894b0c285@solarflare.com \
--to=arybchenko@solarflare.com \
--cc=akhil.goyal@nxp.com \
--cc=amr.mokhtar@intel.com \
--cc=anatoly.burakov@intel.com \
--cc=bernard.iremonger@intel.com \
--cc=bruce.richardson@intel.com \
--cc=byron.marohn@intel.com \
--cc=chas3@att.com \
--cc=cristian.dumitrescu@intel.com \
--cc=david.hunt@intel.com \
--cc=declan.doherty@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=harry.van.haaren@intel.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=jijiang.liu@intel.com \
--cc=jingjing.wu@intel.com \
--cc=john.mcnamara@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=marko.kovacevic@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=nhorman@tuxdriver.com \
--cc=orika@mellanox.com \
--cc=pablo.de.lara.guarch@intel.com \
--cc=radu.nicolau@intel.com \
--cc=ravi1.kumar@amd.com \
--cc=remy.horton@intel.com \
--cc=reshma.pattan@intel.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
--cc=tiwei.bie@intel.com \
--cc=tomasz.kantecki@intel.com \
--cc=wenzhuo.lu@intel.com \
--cc=yongwang@vmware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).