> -----Original Message----- > From: Adrien Mazarguil <adrien.mazarg...@6wind.com> > Sent: Wednesday, April 18, 2018 9:26 PM > To: Xueming(Steven) Li <xuemi...@mellanox.com> > Cc: Shahaf Shuler <shah...@mellanox.com>; Nelio Laranjeiro > <notificati...@github.com>; Wenzhuo Lu > <wenzhuo...@intel.com>; Jingjing Wu <jingjing...@intel.com>; Thomas Monjalon > <tho...@monjalon.net>; > dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS > hash types > > Hi Xueming, > > On Wed, Apr 18, 2018 at 07:06:48PM +0800, Xueming Li wrote: > > "port config all rss all" command will fail on PMD that not support > > any of hard coding RSS hash types. This patch changed hard coding hash > > types to supported types retrieved from device info. > > > > Signed-off-by: Xueming Li <xuemi...@mellanox.com> > > Problem is that this patch removes the ability to request "all" RSS types > regardless of PMD support. > > Users will expect "all" to behave as documented, however doing so will only > result in the limited set > of types reported by PMDs. For instance at least > mlx4 doesn't update the flow_type_rss_offloads field since it has never > implemented configuration > support for the legacy RSS API. > > You should add an argument with a different name (e.g. "supported" or > "default") for that and keep the original meaning for "all". > > Testpmd documentation has to be updated accordingly.
You must want to have a look at first part of this patchset: http://www.dpdk.org/ml/archives/dev/2018-April/097849.html > > One more nit below. > > > --- > > app/test-pmd/cmdline.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index > > 512e3b55e..d357de7e6 100644 > > --- a/app/test-pmd/cmdline.c > > +++ b/app/test-pmd/cmdline.c > > @@ -1998,6 +1998,7 @@ cmd_config_rss_parsed(void *parsed_result, { > > struct cmd_config_rss *res = parsed_result; > > struct rte_eth_rss_conf rss_conf = { .rss_key_len = 0, }; > > + struct rte_eth_dev_info dev_info = {0}; > > This could be declared in the new block where it's used. Also note that "{0}" > is nonstandard syntax, > use memset() or initialize a field like rss_key_len above. > > > int diag; > > uint8_t i; > > > > @@ -2034,6 +2035,12 @@ cmd_config_rss_parsed(void *parsed_result, > > } > > rss_conf.rss_key = NULL; > > for (i = 0; i < rte_eth_dev_count(); i++) { > > + if (!strcmp(res->value, "all")) { > > + rte_eth_dev_info_get(i, &dev_info); > > + if (dev_info.flow_type_rss_offloads) > > + rss_conf.rss_hf = > > + dev_info.flow_type_rss_offloads; > > + } > > diag = rte_eth_dev_rss_hash_update(i, &rss_conf); > > if (diag < 0) > > printf("Configuration of RSS hash at ethernet port %d " > > -- > > 2.13.3 > > > > > -- > Adrien Mazarguil > 6WIND
