From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0FA664318F; Tue, 17 Oct 2023 20:13:54 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CB30640A8B; Tue, 17 Oct 2023 20:13:53 +0200 (CEST) Received: from forward501a.mail.yandex.net (forward501a.mail.yandex.net [178.154.239.81]) by mails.dpdk.org (Postfix) with ESMTP id EAF4C40A8B for ; Tue, 17 Oct 2023 20:13:52 +0200 (CEST) Received: from mail-nwsmtp-smtp-production-main-73.iva.yp-c.yandex.net (mail-nwsmtp-smtp-production-main-73.iva.yp-c.yandex.net [IPv6:2a02:6b8:c0c:2ea5:0:640:a7e7:0]) by forward501a.mail.yandex.net (Yandex) with ESMTP id 60B97612D7; Tue, 17 Oct 2023 21:13:52 +0300 (MSK) Received: by mail-nwsmtp-smtp-production-main-73.iva.yp-c.yandex.net (smtp/Yandex) with ESMTPSA id oDpO2i4DY4Y0-pm6GrGZz; Tue, 17 Oct 2023 21:13:51 +0300 X-Yandex-Fwd: 1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yandex.ru; s=mail; t=1697566431; bh=+ZHvzUH53csxzcBv7v4WdIW9mDGYMn8CiF5oQlFc8EM=; h=In-Reply-To:From:Date:References:To:Subject:Message-ID; b=GjtKmF+DiboYg8E/eHis5grAYectA7pFuNMV7dnQd431/caeOO4TuDzLN4exMbha0 v8rmSA6xMgkPVzvycdAyiouFLSkow4vprBcN4XpT5k3cnVRJPq+rj8sgnQGZEb+7em pB75mb9kWKZXRrKUpXqOtaf299ZDTDBjDZ4hJopY= Authentication-Results: mail-nwsmtp-smtp-production-main-73.iva.yp-c.yandex.net; dkim=pass header.i=@yandex.ru Message-ID: Date: Tue, 17 Oct 2023 19:13:48 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/3] examples/l3fwd: relax the Offload requirement Content-Language: en-US, ru-RU To: Trevor Tao , dev@dpdk.org References: <20231013042722.429592-1-taozj888@163.com> <20231013042722.429592-3-taozj888@163.com> From: Konstantin Ananyev In-Reply-To: <20231013042722.429592-3-taozj888@163.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > --- > 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)