From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Thomas Monjalon <thomas@monjalon.net>, <ferruh.yigit@intel.com>,
<shahafs@mellanox.com>
Cc: <dev@dpdk.org>, <ravi1.kumar@amd.com>, <rasesh.mody@cavium.com>,
<maxime.coquelin@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v2 3/5] ethdev: convert remaining apps to new offload API
Date: Wed, 4 Jul 2018 14:16:21 +0300 [thread overview]
Message-ID: <1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com> (raw)
In-Reply-To: <20180702212750.16758-4-thomas@monjalon.net>
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.
next prev parent reply other threads:[~2018-07-04 11:16 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-08 22:41 [dpdk-dev] [RFC] ethdev: remove all " Ferruh Yigit
2018-06-08 21:52 ` Ferruh Yigit
2018-06-09 8:04 ` Andrew Rybchenko
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 [this message]
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=1d842244-297a-1e0d-c86c-989a99b6c3c8@solarflare.com \
--to=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=rasesh.mody@cavium.com \
--cc=ravi1.kumar@amd.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
/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).