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 D2723A0544; Tue, 21 Jun 2022 04:17:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 744914069C; Tue, 21 Jun 2022 04:17:10 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id DC57840151 for ; Tue, 21 Jun 2022 04:17:08 +0200 (CEST) Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.57]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4LRqpr5Vcwz1KC5w; Tue, 21 Jun 2022 10:15:00 +0800 (CST) Received: from kwepemm600004.china.huawei.com (7.193.23.242) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 21 Jun 2022 10:17:06 +0800 Received: from [10.67.103.231] (10.67.103.231) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Tue, 21 Jun 2022 10:17:05 +0800 Message-ID: Date: Tue, 21 Jun 2022 10:17:04 +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 V3 app/testpmd 1/4] app/testpmd: fix supported RSS offload display To: Ferruh Yigit , , , , CC: , , References: <20220429102445.23711-1-lihuisong@huawei.com> <20220607083246.12259-1-lihuisong@huawei.com> <20220607083246.12259-2-lihuisong@huawei.com> <04f8806c-0dfe-67a3-0027-c21465b29a77@xilinx.com> From: "lihuisong (C)" In-Reply-To: <04f8806c-0dfe-67a3-0027-c21465b29a77@xilinx.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemm600004.china.huawei.com (7.193.23.242) 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 在 2022/6/20 20:35, Ferruh Yigit 写道: > On 6/9/2022 4:25 AM, lihuisong (C) wrote: >> CAUTION: This message has originated from an External Source. Please >> use proper judgment and caution when opening attachments, clicking >> links, or responding to this email. >> >> >> 在 2022/6/7 23:45, Ferruh Yigit 写道: >>> On 6/7/2022 9:32 AM, Huisong Li wrote: >>> >>>> >>>> And rte_eth_dev_info.flow_type_rss_offloads is populated in terms of >>>> RTE_ETH_RSS_* bits. If PMD sets RTE_ETH_RSS_L3_SRC_ONLY to >>>> dev_info->flow_type_rss_offloads. testpmd will display "user >>>> defined 63" >>>> when run 'show port info 0'. Because testpmd use flowtype_to_str() >>>> to display the supported RSS offload of PMD. In fact, the function is >>>> used to display flow type in FDIR commands. This patch uses the >>>> RTE_ETH_RSS_* bits to display supported RSS offload of PMD. >>>> >>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Huisong Li >>>> Signed-off-by: Ferruh Yigit >>>> --- >>>>   app/test-pmd/config.c | 85 >>>> ++++++++++++++++++++++++++++++++++++++----- >>>>   1 file changed, 75 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >>>> index 72d2606d19..d290ca3a06 100644 >>>> --- a/app/test-pmd/config.c >>>> +++ b/app/test-pmd/config.c >>>> @@ -86,6 +86,56 @@ static const struct { >>>>          }, >>>>   }; >>>> >>>> +const struct rss_type_info rss_offload_table[] = { >>>> +       {"ipv4", RTE_ETH_RSS_IPV4}, >>>> +       {"ipv4-frag", RTE_ETH_RSS_FRAG_IPV4}, >>>> +       {"ipv4-tcp", RTE_ETH_RSS_NONFRAG_IPV4_TCP}, >>>> +       {"ipv4-udp", RTE_ETH_RSS_NONFRAG_IPV4_UDP}, >>>> +       {"ipv4-sctp", RTE_ETH_RSS_NONFRAG_IPV4_SCTP}, >>>> +       {"ipv4-other", RTE_ETH_RSS_NONFRAG_IPV4_OTHER}, >>>> +       {"ipv6", RTE_ETH_RSS_IPV6}, >>>> +       {"ipv6-frag", RTE_ETH_RSS_FRAG_IPV6}, >>>> +       {"ipv6-tcp", RTE_ETH_RSS_NONFRAG_IPV6_TCP}, >>>> +       {"ipv6-udp", RTE_ETH_RSS_NONFRAG_IPV6_UDP}, >>>> +       {"ipv6-sctp", RTE_ETH_RSS_NONFRAG_IPV6_SCTP}, >>>> +       {"ipv6-other", RTE_ETH_RSS_NONFRAG_IPV6_OTHER}, >>>> +       {"l2_payload", RTE_ETH_RSS_L2_PAYLOAD}, >>>> +       {"ipv6-ex", RTE_ETH_RSS_IPV6_EX}, >>>> +       {"ipv6-tcp-ex", RTE_ETH_RSS_IPV6_TCP_EX}, >>>> +       {"ipv6-udp-ex", RTE_ETH_RSS_IPV6_UDP_EX}, >>>> +       {"port", RTE_ETH_RSS_PORT}, >>>> +       {"vxlan", RTE_ETH_RSS_VXLAN}, >>>> +       {"geneve", RTE_ETH_RSS_GENEVE}, >>>> +       {"nvgre", RTE_ETH_RSS_NVGRE}, >>>> +       {"gtpu", RTE_ETH_RSS_GTPU}, >>>> +       {"eth", RTE_ETH_RSS_ETH}, >>>> +       {"s-vlan", RTE_ETH_RSS_S_VLAN}, >>>> +       {"c-vlan", RTE_ETH_RSS_C_VLAN}, >>>> +       {"esp", RTE_ETH_RSS_ESP}, >>>> +       {"ah", RTE_ETH_RSS_AH}, >>>> +       {"l2tpv3", RTE_ETH_RSS_L2TPV3}, >>>> +       {"pfcp", RTE_ETH_RSS_PFCP}, >>>> +       {"pppoe", RTE_ETH_RSS_PPPOE}, >>>> +       {"ecpri", RTE_ETH_RSS_ECPRI}, >>>> +       {"mpls", RTE_ETH_RSS_MPLS}, >>>> +       {"ipv4-chksum", RTE_ETH_RSS_IPV4_CHKSUM}, >>>> +       {"l4-chksum", RTE_ETH_RSS_L4_CHKSUM}, >>>> +       {"l2tpv2", RTE_ETH_RSS_L2TPV2}, >>>> +       {"l3-pre96", RTE_ETH_RSS_L3_PRE96}, >>>> +       {"l3-pre64", RTE_ETH_RSS_L3_PRE64}, >>>> +       {"l3-pre56", RTE_ETH_RSS_L3_PRE56}, >>>> +       {"l3-pre48", RTE_ETH_RSS_L3_PRE48}, >>>> +       {"l3-pre40", RTE_ETH_RSS_L3_PRE40}, >>>> +       {"l3-pre32", RTE_ETH_RSS_L3_PRE32}, >>>> +       {"l2-dst-only", RTE_ETH_RSS_L2_DST_ONLY}, >>>> +       {"l2-src-only", RTE_ETH_RSS_L2_SRC_ONLY}, >>>> +       {"l4-dst-only", RTE_ETH_RSS_L4_DST_ONLY}, >>>> +       {"l4-src-only", RTE_ETH_RSS_L4_SRC_ONLY}, >>>> +       {"l3-dst-only", RTE_ETH_RSS_L3_DST_ONLY}, >>>> +       {"l3-src-only", RTE_ETH_RSS_L3_SRC_ONLY}, >>>> +       {NULL, 0}, >>>> +}; >>>> + >>> >>> Hi Huisong, >>> >>> Why not reusing existing 'rss_type_table[]', but adding a new one? >>> Is it to have each individual RSS type instead of grouping >>> 'rss_type_table[]' has? If so, since command "port config all rss ..." >>> using the grouping, I think it makes sense to have grouping. >>> >> The 'rss_offload_table[]' includes all defined RSS offloads in ethdev >> layer, every iterm has one bit, >> and is part of 'rss_type_table[]'. Some iterms in 'rss_type_table[]' >> consist of multiple bits. If we >> want to display all RSS offloads PMD supported and resolve undefined >> offload, we have to check >> 'flow_type_rss_offloads' bit by bit. However, "port config all rss ..." >> and "show port 0 rss-hash" >> commands supports the configuration and display of multiple offload bits >> at a time. So it is necessary >> to introduce this 'rss_offload_table[]' to display >> 'flow_type_rss_offloads'. That's what I think. > > 'rss_type_table[]' has both group and individual types, so show > functions displays both. > > A sample of grouping is UDP, a few UDP flags compined under UDP, > #define RTE_ETH_RSS_UDP ( \ >         RTE_ETH_RSS_NONFRAG_IPV4_UDP | \ >         RTE_ETH_RSS_NONFRAG_IPV6_UDP | \ >         RTE_ETH_RSS_IPV6_UDP_EX) > > Above mentioned commands use 'RTE_ETH_RSS_UDP' to set "udp" type RSS, > and when displying instead of displaying each above type separately, I > think displaying as "udp" makes sense. > > And again I believe having two different arrays is prone to error for > future. Agreed. > >>> Another thing is having two different array with very similar content >>> is easy to confuse and can cause diversion on arrays and generate bugs. >>> >> Add some notes about the use of the 'rss_offload_table[]'? >>> >>> As mentioned from "port config all rss ..." command, it seems it is >>> also not using 'rss_type_table[]', but all string to RSS type matching >>> done within the function ('cmd_config_rss_parsed()') duplicating what >>> we have in 'rss_type_table[]'. Again this has concern to diverge >>> between set and show functions. >>> If you have time for it, can you make an additional patch to update >>> 'cmd_config_rss_parsed()' to use new 'str_to_rsstypes()' function? >> All right. Let's fix it. > > Thanks, and if you want it can be a separate patch on top of this one, > it doesn't have to be part of this set if you don't have time for it. > I think it's better to do it in this patch set. So I will do it. >>> >>> Thanks, >>> ferruh >>> >>>>   const struct rss_type_info rss_type_table[] = { >>>>          { "all", RTE_ETH_RSS_ETH | RTE_ETH_RSS_VLAN | RTE_ETH_RSS_IP >>>> | RTE_ETH_RSS_TCP | >>>>                  RTE_ETH_RSS_UDP | RTE_ETH_RSS_SCTP | >>>> RTE_ETH_RSS_L2_PAYLOAD | >>>> @@ -675,6 +725,19 @@ print_dev_capabilities(uint64_t capabilities) >>>>          } >>>>   } >>>> >>>> +static const char * >>>> +rss_offload_to_str(uint64_t rss_offload) >>>> +{ >>>> +       uint16_t i; >>>> + >>>> +       for (i = 0; rss_offload_table[i].str != NULL; i++) { >>>> +               if (rss_offload_table[i].rss_type == rss_offload) >>>> +                       return rss_offload_table[i].str; >>>> +       } >>>> + >>>> +       return NULL; >>>> +} >>>> + >>>>   void >>>>   port_infos_display(portid_t port_id) >>>>   { >>>> @@ -779,19 +842,21 @@ port_infos_display(portid_t port_id) >>>>          if (!dev_info.flow_type_rss_offloads) >>>>                  printf("No RSS offload flow type is supported.\n"); >>>>          else { >>>> +               uint64_t rss_offload_types = >>>> dev_info.flow_type_rss_offloads; >>>>                  uint16_t i; >>>> -               char *p; >>>> >>>>                  printf("Supported RSS offload flow types:\n"); >>>> -               for (i = RTE_ETH_FLOW_UNKNOWN + 1; >>>> -                    i < sizeof(dev_info.flow_type_rss_offloads) * >>>> CHAR_BIT; i++) { >>>> -                       if (!(dev_info.flow_type_rss_offloads & (1ULL >>>> << i))) >>>> -                               continue; >>>> -                       p = flowtype_to_str(i); >>>> -                       if (p) >>>> -                               printf("  %s\n", p); >>>> -                       else >>>> -                               printf("  user defined %d\n", i); >>>> +               for (i = 0; i < sizeof(rss_offload_types) * CHAR_BIT; >>>> i++) { >>>> +                       uint64_t rss_offload = RTE_BIT64(i); >>>> +                       if ((rss_offload_types & rss_offload) != 0) { >>>> +                               const char *p = >>>> + rss_offload_to_str(rss_offload); >>>> +                               if (p) >>>> +                                       printf("  %s\n", p); >>>> +                               else >>>> +                                       printf("  user defined >>>> 0x%"PRIx64"\n", >>>> +                                              rss_offload); >>>> +                       } >>> >>> If you go with 'rss_type_table[]', you need to change above logic as >>> you did in your v2. >>> >>>>                  } >>>>          } >>>> >>>> -- >>>> 2.33.0 >>>> >>> >>> . > > .