From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
To: Trevor Tao <taozj888@163.com>, dev@dpdk.org
Subject: Re: [PATCH v2 2/3] examples/l3fwd: relax the Offload requirement
Date: Tue, 17 Oct 2023 19:13:48 +0100 [thread overview]
Message-ID: <c4e012bd-f94f-484a-990b-0e3199098484@yandex.ru> (raw)
In-Reply-To: <20231013042722.429592-3-taozj888@163.com>
13.10.2023 05:27, Trevor Tao пишет:
> Now the port Rx offload mode is set to RTE_ETH_RX_OFFLOAD_CHECKSUM
> by default, but some hw and/or virtual interface does not support
> the offload mode presupposed, e.g., some virtio interfaces in
> the cloud may only partly support RTE_ETH_RX_OFFLOAD_UDP_CKSUM/
> RTE_ETH_RX_OFFLOAD_TCP_CKSUM,
> but not RTE_ETH_RX_OFFLOAD_IPV4_CKSUM, and the error msg here:
>
> Ethdev port_id=0 requested Rx offloads 0xe does not match Rx offloads
> capabilities 0x201d in rte_eth_dev_configure()
>
> So to enable the l3fwd running in that environment, the Rx mode requirement
> can be relaxed to reflect the hardware feature reality here, and the l3fwd
> can run smoothly then.
> A warning msg would be provided to user in case it happens here.
>
> On the other side, enabling the software cksum check in case missing the
> hw support.
>
> The relax action for rx cksum offload is just enabled when relax_rx_mode is
> true which is false by default.
>
> Signed-off-by: Trevor Tao <taozj888@163.com>
> ---
> examples/l3fwd/l3fwd.h | 12 ++++++++++--
> examples/l3fwd/l3fwd_em.h | 2 +-
> examples/l3fwd/l3fwd_lpm.h | 2 +-
> examples/l3fwd/main.c | 14 ++++++++++++++
> 4 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
> index b55855c932..fd98ad3373 100644
> --- a/examples/l3fwd/l3fwd.h
> +++ b/examples/l3fwd/l3fwd.h
> @@ -159,7 +159,7 @@ send_single_packet(struct lcore_conf *qconf,
>
> #ifdef DO_RFC_1812_CHECKS
> static inline int
> -is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
> +is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags)
> {
> /* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
> /*
> @@ -170,7 +170,15 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
> return -1;
>
> /* 2. The IP checksum must be correct. */
> - /* this is checked in H/W */
> + /* if this is not checked in H/W, check it. */
> + if ((ol_flags & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
That looks like a wrong flag, I think it should be:
if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_NONE)
Which makes me wonder was that piece of code ever tested properly?
> + uint16_t actual_cksum, expected_cksum;
> + actual_cksum = pkt->hdr_checksum;
> + pkt->hdr_checksum = 0;
> + expected_cksum = rte_ipv4_cksum(pkt);
> + if (actual_cksum != expected_cksum)
> + return -2;
> + }
Actually, while looking at it another thing stroke me, when HW ip cksum
is enabled, shouldn't we check that it is a valid one?
I.E:
if (ol_flags & RTE_MBUF_F_RX_L4_CKSUM_MASK) == RTE_MBUF_F_RX_L4_CKSUM_BAD)
return -2;
>
> /*
> * 3. The IP version number must be 4. If the version number is not 4
> diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
> index 7d051fc076..1fee2e2e6c 100644
> --- a/examples/l3fwd/l3fwd_em.h
> +++ b/examples/l3fwd/l3fwd_em.h
> @@ -20,7 +20,7 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
>
> #ifdef DO_RFC_1812_CHECKS
> /* Check to make sure the packet is valid (RFC1812) */
> - if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
> + if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
> rte_pktmbuf_free(m);
> return BAD_PORT;
> }
> diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
> index c61b969584..4ee61e8d88 100644
> --- a/examples/l3fwd/l3fwd_lpm.h
> +++ b/examples/l3fwd/l3fwd_lpm.h
> @@ -22,7 +22,7 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
>
> #ifdef DO_RFC_1812_CHECKS
> /* Check to make sure the packet is valid (RFC1812) */
> - if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
> + if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
> rte_pktmbuf_free(m);
> return;
> }
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 89ad546a5e..2b815375a9 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -1285,6 +1285,20 @@ l3fwd_poll_resource_setup(void)
> local_port_conf.rx_adv_conf.rss_conf.rss_hf);
> }
>
> + /* relax the rx offload requirement */
> + if ((local_port_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
> + local_port_conf.rxmode.offloads) {
> + printf("Port %u requested Rx offloads 0x%"PRIx64" does not"
> + " match Rx offloads capabilities 0x%"PRIx64"\n",
> + portid, local_port_conf.rxmode.offloads,
> + dev_info.rx_offload_capa);
> + if (relax_rx_mode) {
> + local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> + printf("warning: modified the rx offload to 0x%"PRIx64" based on device"
> + " capability\n", local_port_conf.rxmode.offloads);
> + }
> + }
> +
Still not sure it is a good thing to disable L4 cksum offload.
We definetly don't need it for LPM fwd, for EM/ACL - it is an open
question, as we do lookup on L4 ports too...
If we don't need it completely, why to request it after all?
> ret = rte_eth_dev_configure(portid, nb_rx_queue,
> (uint16_t)n_tx_queue, &local_port_conf);
> if (ret < 0)
next prev parent reply other threads:[~2023-10-17 18:13 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-13 4:27 [PATCH v2 0/3] example/l3fwd: relax l3fwd rx RSS/Offload if needed Trevor Tao
2023-10-13 4:27 ` [PATCH v2 1/3] examples/l3fwd: relax RSS requirement with option Trevor Tao
2023-10-17 18:06 ` Konstantin Ananyev
2023-10-13 4:27 ` [PATCH v2 2/3] examples/l3fwd: relax the Offload requirement Trevor Tao
2023-10-17 18:13 ` Konstantin Ananyev [this message]
2023-10-13 4:27 ` [PATCH v2 3/3] doc: add a relax rx mode requirement option Trevor Tao
2023-10-31 20:08 ` Stephen Hemminger
2023-10-13 7:07 ` [PATCH v2 0/3] example/l3fwd: relax l3fwd rx RSS/Offload if needed David Marchand
2023-10-15 14:06 ` taozj888
-- strict thread matches above, loose matches on Subject: below --
2023-10-13 3:27 Trevor Tao
2023-10-13 3:27 ` [PATCH v2 2/3] examples/l3fwd: relax the Offload requirement Trevor Tao
2023-10-13 3:08 [PATCH v2 0/3] example/l3fwd: relax l3fwd rx RSS/Offload if needed Trevor Tao
2023-10-13 3:08 ` [PATCH v2 2/3] examples/l3fwd: relax the Offload requirement Trevor Tao
2023-10-12 16:03 [PATCH v2 1/3] examples/l3fwd: relax RSS requirement with option Trevor Tao
2023-10-12 16:03 ` [PATCH v2 2/3] examples/l3fwd: relax the Offload requirement Trevor Tao
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=c4e012bd-f94f-484a-990b-0e3199098484@yandex.ru \
--to=konstantin.v.ananyev@yandex.ru \
--cc=dev@dpdk.org \
--cc=taozj888@163.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).