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 C9553432FA; Sat, 11 Nov 2023 05:27:44 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7C0D9402A6; Sat, 11 Nov 2023 05:27:44 +0100 (CET) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 614984025C for ; Sat, 11 Nov 2023 05:27:42 +0100 (CET) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.56]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4SS2cd1vwjzPp45; Sat, 11 Nov 2023 12:23:29 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm000004.china.huawei.com (7.193.23.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Sat, 11 Nov 2023 12:27:39 +0800 Message-ID: <999b1089-57b9-898d-dc8b-e8ee47946c6c@huawei.com> Date: Sat, 11 Nov 2023 12:27:38 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v1 2/3] app/testpmd: add the explicit check for tunnel TSO offload To: Ferruh Yigit , , Aman Singh , Yuying Zhang , Wenzhuo Lu , Shahaf Shuler CC: , References: <20231110081925.14142-1-lihuisong@huawei.com> <20231110081925.14142-3-lihuisong@huawei.com> From: "lihuisong (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To kwepemm000004.china.huawei.com (7.193.23.18) X-CFilter-Loop: Reflected 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 Hi Ferruh, Thanks for you review so fast. 在 2023/11/11 11:30, Ferruh Yigit 写道: > On 11/10/2023 8:19 AM, Huisong Li wrote: >> If port don't support TSO of tunnel packets, tell user in advance and >> no need to change other configuration of this port in case of fault spread. >> >> In addition, if some tunnel offloads don't support, which is not an error >> case, the log about this shouldn't be to stderr. >> > Overall patch looks good to me, please check a minor comment below. > >> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check") >> Cc: stable@dpdk.org >> >> Signed-off-by: Huisong Li >> --- >> app/test-pmd/cmdline.c | 51 +++++++++++++++++++----------------------- >> 1 file changed, 23 insertions(+), 28 deletions(-) >> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c >> index 25462bdbfc..d3243d016b 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -5039,28 +5039,22 @@ static void >> check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa) >> { >> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO)) >> - fprintf(stderr, >> - "Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n", >> + printf("Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n", >> port_id); >> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO)) >> - fprintf(stderr, >> - "Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n", >> + printf("Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n", >> port_id); >> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO)) >> - fprintf(stderr, >> - "Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n", >> + printf("Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n", >> port_id); >> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO)) >> - fprintf(stderr, >> - "Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n", >> + printf("Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n", >> port_id); >> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO)) >> - fprintf(stderr, >> - "Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n", >> + printf("Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n", >> port_id); >> if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO)) >> - fprintf(stderr, >> - "Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n", >> + printf("Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n", >> port_id); >> } >> >> @@ -5071,6 +5065,12 @@ cmd_tunnel_tso_set_parsed(void *parsed_result, >> { >> struct cmd_tunnel_tso_set_result *res = parsed_result; >> struct rte_eth_dev_info dev_info; >> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO; >> int ret; >> >> if (port_id_is_invalid(res->port_id, ENABLED_WARN)) >> @@ -5087,26 +5087,21 @@ cmd_tunnel_tso_set_parsed(void *parsed_result, >> if (ret != 0) >> return; >> >> - check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa); >> + if (ports[res->port_id].tunnel_tso_segsz != 0) { >> + if ((all_tunnel_tso & dev_info.tx_offload_capa) == 0) { >> + fprintf(stderr, "Error: port=%u don't support tunnel TSO offloads.\n", >> + res->port_id); >> + return; >> + } >> + check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa); >> + } >> + > Instead of having a separate if block, else leg of below if block does > same check [1], what do you think to move above block to there? ok, will fixed in next version. > >> if (ports[res->port_id].tunnel_tso_segsz == 0) { >> - ports[res->port_id].dev_conf.txmode.offloads &= >> - ~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_IP_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO); >> + ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso; >> printf("TSO for tunneled packets is disabled\n"); >> } else { >> - uint64_t tso_offloads = (RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_IP_TNL_TSO | >> - RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO); >> - > [1] ---> here has same logical condition > >> ports[res->port_id].dev_conf.txmode.offloads |= >> - (tso_offloads & dev_info.tx_offload_capa); >> + (all_tunnel_tso & dev_info.tx_offload_capa); >> printf("TSO segment size for tunneled packets is %d\n", >> ports[res->port_id].tunnel_tso_segsz); >> > .