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 1FD99A0548; Tue, 31 May 2022 18:35:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 70D6E4111B; Tue, 31 May 2022 18:35:24 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E17A54111B; Tue, 31 May 2022 18:35:23 +0200 (CEST) Received: from [192.168.1.38] (unknown [188.170.81.145]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 1F45688; Tue, 31 May 2022 19:35:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 1F45688 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1654014923; bh=qffoyAyJ+a6EVBQMXYCV7KUI0FS3lDqokTsUPp2nDwI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=gfAu2WoA2uh8K/F82bTeWksjHBtXqX+JG/yHfTFwaycdtShMoW5SR2q9qoz41H039 1M+iVu8lO2joxXSHko6UXtn2zh6ABmMmQmvg5WAWjvEgnzKoZnXRuD1+38TDSk8SJf Pf6wf0+6pUmRKeP7u0GYHdMoFyeb87inYs8TOxuc= Message-ID: Date: Tue, 31 May 2022 19:35:22 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 Subject: Re: [PATCH 1/3] app/testpmd: fix displaying RSS info Content-Language: en-US To: "lihuisong (C)" , Ferruh Yigit , Xiaoyun Li , Aman Singh , Yuying Zhang , Helin Zhang , Jingjing Wu , Thomas Monjalon Cc: dev@dpdk.org, Thomas Monjalon , stable@dpdk.org References: <20220525173736.3394787-1-ferruh.yigit@xilinx.com> <3110523f-72e6-20be-4510-be579722ffbd@huawei.com> <242f8a69-81d9-1795-fcf3-94358dae590d@xilinx.com> From: Andrew Rybchenko In-Reply-To: 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 On 5/31/22 05:07, lihuisong (C) wrote: > > 在 2022/5/30 21:02, Ferruh Yigit 写道: >> On 5/30/2022 1:32 PM, 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/5/30 18:43, Ferruh Yigit 写道: >>>> On 5/27/2022 3:30 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/5/26 1:37, Ferruh Yigit 写道: >>>>>> When supported RSS offload flow types are printed via 'show port >>>>>> info #' >>>>>> command, flow names are get from flow type array which is wrong and >>>>>> causing some RSS flow types not being displayed. >>>>>> >>>>>> Instead RSS flow type array should be used. Also helper functions >>>>>> added >>>>>> and existing code updated to use helpers. >>>>>> >>>>>> Fixes: b12964f621dc ("ethdev: unification of RSS offload types") >>>>>> Cc: stable@dpdk.org >>>>>> >>>>>> Signed-off-by: Ferruh Yigit >>>>>> --- >>>>>> Cc: helin.zhang@intel.com >>>>>> >>>>>> Note: >>>>>> In ethdev, flow type macros 'RTE_ETH_FLOW_*' and RSS type macros >>>>>> 'RTE_ETH_RSS_*' are related, buy they seems diverged a little, may >>>>>> need >>>>>> to check that too. >>>>>> --- >>>>>>   app/test-pmd/config.c | 92 >>>>>> ++++++++++++++++++++++++++++++------------- >>>>>>   1 file changed, 64 insertions(+), 28 deletions(-) >>>>>> >>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >>>>>> index 72d2606d19d5..c353d224ef06 100644 >>>>>> --- a/app/test-pmd/config.c >>>>>> +++ b/app/test-pmd/config.c >>>>>> @@ -147,6 +147,32 @@ const struct rss_type_info rss_type_table[] = { >>>>>>       { NULL, 0 }, >>>>>>   }; >>>>>> >>>>>> +static const char * >>>>>> +rsstype_to_str(uint64_t rss_type) >>>>>> +{ >>>>>> +     int i; >>>>>> + >>>>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) { >>>>>> +             if (rss_type_table[i].rss_type == rss_type) >>>>>> +                     return rss_type_table[i].str; >>>>>> +     } >>>>>> + >>>>>> +     return NULL; >>>>>> +} >>>>>> + >>>>>> +static uint64_t >>>>>> +str_to_rsstype(const char *str) >>>>>> +{ >>>>>> +     int i; >>>>>> + >>>>>> +     for (i = 0; rss_type_table[i].str != NULL; i++) { >>>>>> +             if (!strcmp(rss_type_table[i].str, str)) >>>>>> +                     return rss_type_table[i].rss_type; >>>>>> +     } >>>>>> + >>>>>> +     return 0; >>>>>> +} >>>>>> + >>>>>>   static const struct { >>>>>>       enum rte_eth_fec_mode mode; >>>>>>       const char *name; >>>>>> @@ -779,19 +805,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_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; rss_types != 0; i++) { >>>>>> +                     if (rss_types & 1) { >>>>>> +                             uint64_t rss_type = 1ULL << i; >>>>>> +                             const char *p = >>>>>> rsstype_to_str(rss_type); >>>>>> + >>>>>> +                             if (p) >>>>>> +                                     printf("  %s\n", p); >>>>>> +                             else >>>>>> +                                     printf("  user defined >>>>>> 0x%"PRIx64"\n", rss_type); >>> Maybe we need to add a mapping table between RSS offload and name for >>> 'flow_type_rss_offloads'. >>>>>> +                     } >>>>>> +                     rss_types >>= 1; >>>>>>               } >>>>>>       } >>>>>> >>>>>> @@ -1547,6 +1575,7 @@ port_flow_complain(struct rte_flow_error >>>>>> *error) >>>>>>   static void >>>>>>   rss_config_display(struct rte_flow_action_rss *rss_conf) >>>>>>   { >>>>>> +     uint64_t rss_types; >>>>>>       uint8_t i; >>>>>> >>>>>>       if (rss_conf == NULL) { >>>>>> @@ -1582,16 +1611,23 @@ rss_config_display(struct >>>>>> rte_flow_action_rss *rss_conf) >>>>>>       } >>>>>> >>>>>>       printf(" types:\n"); >>>>>> -     if (rss_conf->types == 0) { >>>>>> +     rss_types = rss_conf->types; >>>>>> +     if (rss_types == 0) { >>>>>>               printf("  none\n"); >>>>>>               return; >>>>>>       } >>>>>> -     for (i = 0; rss_type_table[i].str; i++) { >>>>>> -             if ((rss_conf->types & >>>>>> -                 rss_type_table[i].rss_type) == >>>>>> -                 rss_type_table[i].rss_type && >>>>>> -                 rss_type_table[i].rss_type != 0) >>>>>> -                     printf("  %s\n", rss_type_table[i].str); >>>>>> + >>>>>> +     for (i = 0; rss_types != 0; i++) { >>>>>> +             if (rss_types & 1) { >>>>>> +                     uint64_t rss_type = 1ULL << i; >>>>>> +                     const char *p = rsstype_to_str(rss_type); >>>>> It seems that we can't use one bit to get rss type name. >>>>> Because part of name in rss_type_table[] consist of multiple bits. >>>>> Maybe it's better to use the original method to display RSS type name. >>>>> >>>> >>>> Right, it doesn't cover 'all' case, but thinking twice how useful >>>> 'all' case is, it doesn't really cover all options, and for user it is >>>> hard to know which functions are set when it displays 'all'. >>>> What about to remove 'all' item from 'rss_type_table[]'? >>> It seems that remove 'all' item isn't the way to resolve this issue. >>> There are other iterms in 'rss_type_table[], such as, 'ip', 'udp', >>> 'tcp' and so on. The 'rss_type_table[]' should primarily serve >>> the "show port rss xxx" and "port confg rss xxx" commands. >>> They all need these. >> > >> >> You are right, it is not just 'all'. >> >>>> >>>>> On the other hand, RSS type name are printed in many places as this >>>>> patch modified. >>>>> It is recommended that you encapsulate a funcion to display RSS type. >>>> >>>> Yes but each are formatting slightly different, so to let various >>>> formatting possible, common function returns string instead of >>>> printing them. As 3/3 of this set does some formatting. >>> >>> We can unify this display if we add the mapping table mentioned above. >>> >> >> If it can be unified, agree that this simplifies the code [2]. > Ack. >> >> Both above two changes already exists in your set [1], if you don't >> mind to adapt 2/3 & 3/3 of this set into yours, we can continue with >> your set, is this OK for you? > ok. It's my pleasure to work with you. >> >> >> >> [1] >> https://patches.dpdk.org/project/dpdk/list/?series=22735 >> >> [2] >> In your 'show_rss_types()', it can get additional parameter for >> formatting, like after how many items to break to next line, this can >> cover existing formatting options, but not sure how elegant a solution >> it is. So, I'm waiting for a new version from lihuisong.