[dpdk-dev,v3,2/2] app/testpmd: only config supported RSS hash types
Checks
Commit Message
"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 <xuemingl@mellanox.com>
---
app/test-pmd/cmdline.c | 7 +++++++
1 file changed, 7 insertions(+)
Comments
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 <xuemingl@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.
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
>
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 9:26 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@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 <xuemingl@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
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 9:26 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@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 <xuemingl@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.
Keeping it "all" as it is will result in error for PMDs not completely support "all" RSS types and this
always confuse people.
How about adding a check, if flow_type_rss_offloads not set, ignore and set default RSS "all" types?
>
> 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.
>
> 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
On Wed, Apr 18, 2018 at 01:54:20PM +0000, Xueming(Steven) Li wrote:
>
>
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 18, 2018 9:26 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> > <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@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 <xuemingl@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
I did, however this is does not address my previous comment.
In this patch you're preventing testpmd users from requesting something. If
they want to enable "all" RSS types and some of them happen to be
unsupported on some ports, it means they want to get errors. Otherwise they
might think their request succeeded while it was in fact interpreted in an
undocumented manner.
The point is if users want to enable only "supported" RSS types, they
wouldn't write "all" on the command line right? A new parameter is needed
for that.
>
> >
> > 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
+Olivier
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 10:17 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
>
> On Wed, Apr 18, 2018 at 01:54:20PM +0000, Xueming(Steven) Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Wednesday, April 18, 2018 9:26 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> > > <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> > > Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > > <thomas@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 <xuemingl@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:
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> > dpdk.org%2Fml%2Farchives%2Fdev%2F2018-April%2F097849.html&data=02%7C01
> > %7Cxuemingl%40mellanox.com%7C497583093d324a6861f708d5a5370a8f%7Ca65297
> > 1c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636596578186582002&sdata=uNVw2egf
> > 0rcTHazCuoMelNrY44lZVfd6Ado1bJfzg8s%3D&reserved=0
>
> I did, however this is does not address my previous comment.
In first patch, if flow_type_rss_offloads not set, any RSS will not be allowed.
We need to clarify definition of empty flow_type_rss_offloads, not set or no rss support.
>
> In this patch you're preventing testpmd users from requesting something. If they want to enable "all"
> RSS types and some of them happen to be unsupported on some ports, it means they want to get errors.
> Otherwise they might think their request succeeded while it was in fact interpreted in an undocumented
> manner.
>
> The point is if users want to enable only "supported" RSS types, they wouldn't write "all" on the
> command line right? A new parameter is needed for that.
>
> >
> > >
> > > 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
>
> --
> Adrien Mazarguil
> 6WIND
On Wed, Apr 18, 2018 at 02:10:45PM +0000, Xueming(Steven) Li wrote:
>
>
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 18, 2018 9:26 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> > <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@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 <xuemingl@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.
>
> Keeping it "all" as it is will result in error for PMDs not completely support "all" RSS types and this
> always confuse people.
You're redefining the commonly accepted meaning of "all". The first instance
of "all" in the "port config all rss all" command means "all ports", not
"only ports that happen to implement the RSS configuration callback". Same
for all other testpmd commands accepting "all" as a parameter anywhere.
Those that don't will return errors, it's fine.
> How about adding a check, if flow_type_rss_offloads not set, ignore and set default RSS "all" types?
Results would be even less predictable. Keep it simple. Testpmd is a testing
tool, if PMD developers want to check error-spitting abilities of their
PMDs, they must be able to. Adding a dedicated parameter is one way to
satisfy everyone, dropping this patch is another.
> > 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.
> >
> > 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
On Wed, Apr 18, 2018 at 02:26:30PM +0000, Xueming(Steven) Li wrote:
> +Olivier
>
> > -----Original Message-----
> > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > Sent: Wednesday, April 18, 2018 10:17 PM
> > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> > <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> > dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
> >
> > On Wed, Apr 18, 2018 at 01:54:20PM +0000, Xueming(Steven) Li wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > > Sent: Wednesday, April 18, 2018 9:26 PM
> > > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> > > > <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> > > > Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > > > <thomas@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 <xuemingl@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:
> > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> > > dpdk.org%2Fml%2Farchives%2Fdev%2F2018-April%2F097849.html&data=02%7C01
> > > %7Cxuemingl%40mellanox.com%7C497583093d324a6861f708d5a5370a8f%7Ca65297
> > > 1c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636596578186582002&sdata=uNVw2egf
> > > 0rcTHazCuoMelNrY44lZVfd6Ado1bJfzg8s%3D&reserved=0
> >
> > I did, however this is does not address my previous comment.
>
> In first patch, if flow_type_rss_offloads not set, any RSS will not be allowed.
> We need to clarify definition of empty flow_type_rss_offloads, not set or no rss support.
It's fine, mlx4 does not implement rte_eth_dev_rss_hash_update() either
(legacy RSS configuration is not implemented at all).
With mlx4, the command:
port config all rss (all|anything else)
Always returns an error. It's a good thing because it's not implemented and
users must be made aware of that fact. No need to lie about it.
> > In this patch you're preventing testpmd users from requesting something. If they want to enable "all"
> > RSS types and some of them happen to be unsupported on some ports, it means they want to get errors.
> > Otherwise they might think their request succeeded while it was in fact interpreted in an undocumented
> > manner.
> >
> > The point is if users want to enable only "supported" RSS types, they wouldn't write "all" on the
> > command line right? A new parameter is needed for that.
> >
> > >
> > > >
> > > > 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
> >
> > --
> > Adrien Mazarguil
> > 6WIND
> -----Original Message-----
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Sent: Wednesday, April 18, 2018 10:30 PM
> To: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro <notifications@github.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon <thomas@monjalon.net>;
> dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] app/testpmd: only config supported RSS hash types
>
> On Wed, Apr 18, 2018 at 02:10:45PM +0000, Xueming(Steven) Li wrote:
> >
> >
> > > -----Original Message-----
> > > From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > Sent: Wednesday, April 18, 2018 9:26 PM
> > > To: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Shahaf Shuler <shahafs@mellanox.com>; Nelio Laranjeiro
> > > <notifications@github.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> > > Jingjing Wu <jingjing.wu@intel.com>; Thomas Monjalon
> > > <thomas@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 <xuemingl@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.
> >
> > Keeping it "all" as it is will result in error for PMDs not completely
> > support "all" RSS types and this always confuse people.
>
> You're redefining the commonly accepted meaning of "all". The first instance of "all" in the "port
> config all rss all" command means "all ports", not "only ports that happen to implement the RSS
> configuration callback". Same for all other testpmd commands accepting "all" as a parameter anywhere.
>
> Those that don't will return errors, it's fine.
>
> > How about adding a check, if flow_type_rss_offloads not set, ignore and set default RSS "all" types?
>
> Results would be even less predictable. Keep it simple. Testpmd is a testing tool, if PMD developers
> want to check error-spitting abilities of their PMDs, they must be able to. Adding a dedicated
> parameter is one way to satisfy everyone, dropping this patch is another.
Okay, v4 version sent by adding "default" parameter, thanks.
>
> > > 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.
> > >
> > > 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
>
> --
> Adrien Mazarguil
> 6WIND
@@ -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};
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 "