DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration
@ 2020-09-10  1:51 Lijun Ou
  2020-09-22  9:51 ` Phil Yang
  2020-09-24 13:45 ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Lijun Ou
  0 siblings, 2 replies; 44+ messages in thread
From: Lijun Ou @ 2020-09-10  1:51 UTC (permalink / raw)
  To: wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit; +Cc: dev, linuxarm

When a user runs a flow create cmd to configure an RSS rule
with specifying the empty rss actions in testpmd, this mean
that the flow gets the default RSS functions from the valid
NIC default RSS hash key. However, the testpmd is not set
the default RSS key incorrectly when RSS key is not specified
in flow create cmd. the cmdline as flows:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
 all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
20C6A42B73BBEAC01FA

2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end

3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
 all ipv4-udp udp
RSS key:
74657374706D6427732064656661756C74205253532068617368206B65792C206F
76657272696465

Now, it uses rte_eth_dev_rss_hash_conf_get to correctly the
default rss key. the cmdline and result as flows:
testpmd> show port 0 rss-hash key
RSS functions:
 all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C
6A42B73BBEAC01FA
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end
testpmd> show port 0 rss-hash key
RSS functions:
 all ipv4-udp udp
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
20C6A42B73BBEAC01FA

Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
Cc: stable@dpdk.org

Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V2->V3:
-fix checkpatch warning.

V1->V2:
-fix the commit.
---
 app/test-pmd/cmdline_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 6263d30..e6648da 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->queue[i] = i;
 	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
 	    ctx->port != (portid_t)RTE_PORT_ALL) {
+		struct rte_eth_rss_conf rss_conf = {0};
 		struct rte_eth_dev_info info;
 		int ret2;
 
@@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->conf.key_len =
 			RTE_MIN(sizeof(action_rss_data->key),
 				info.hash_key_size);
+
+		rss_conf.rss_key_len = sizeof(action_rss_data->key);
+		rss_conf.rss_key = action_rss_data->key;
+		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
+		if (ret2 != 0)
+			return ret2;
+		action_rss_data->conf.key = rss_conf.rss_key;
 	}
 	action->conf = &action_rss_data->conf;
 	return ret;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration
  2020-09-10  1:51 [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration Lijun Ou
@ 2020-09-22  9:51 ` Phil Yang
  2020-09-22 13:50   ` oulijun
  2020-09-24 13:45 ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Lijun Ou
  1 sibling, 1 reply; 44+ messages in thread
From: Phil Yang @ 2020-09-22  9:51 UTC (permalink / raw)
  To: Lijun Ou, wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit
  Cc: dev, linuxarm, nd, nd

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
> Sent: Thursday, September 10, 2020 9:51 AM
> To: wenzhuo.lu@intel.com; beilei.xing@intel.com;
> adrien.mazarguil@6wind.com; ferruh.yigit@intel.com
> Cc: dev@dpdk.org; linuxarm@huawei.com
> Subject: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key
> configuration

Hi Lijun,

Please fix the coding style issues.

"Must be a reply to the first patch (--in-reply-to)."


> 
> When a user runs a flow create cmd to configure an RSS rule
> with specifying the empty rss actions in testpmd, this mean
                                                                                                    ^^^ means
> that the flow gets the default RSS functions from the valid
> NIC default RSS hash key. However, the testpmd is not set
                                                                                          ^^^ is set xxx incorrectly    
> the default RSS key incorrectly when RSS key is not specified
> in flow create cmd. 

Use the NIC valid default RSS key instead of the testpmd dummy RSS key in the flow configuration when the RSS key is not specified in the flow rule. If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the default key.

Is that good to put it in this way? Because I think it is not a bug, your patch offers an approach to update the default testpmd RSS key.

I would also suggest making the commit message shorter and move the test result into the cover letter.
Because the checkepatch tool is not happy with the hex dumped below.
http://mails.dpdk.org/archives/test-report/2020-September/151395.html


Thanks,
Phil

> the cmdline as flows:
> 1. first, startup testpmd:
> testpmd> show port 0 rss-hash key
> RSS functions:
>  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> 20C6A42B73BBEAC01FA
> 
> 2. create a rss rule
> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> types ipv4-udp end queues end / end
> 
> 3. show rss-hash key
> testpmd> show port 0 rss-hash key
> RSS functions:
>  all ipv4-udp udp
> RSS key:
> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
> 76657272696465
> 
> Now, it uses rte_eth_dev_rss_hash_conf_get to correctly the
> default rss key. the cmdline and result as flows:
> testpmd> show port 0 rss-hash key
> RSS functions:
>  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F2
> 0C
> 6A42B73BBEAC01FA
> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> types ipv4-udp end queues end / end
> testpmd> show port 0 rss-hash key
> RSS functions:
>  all ipv4-udp udp
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> 20C6A42B73BBEAC01FA
> 
> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V2->V3:
> -fix checkpatch warning.
> 
> V1->V2:
> -fix the commit.
> ---
>  app/test-pmd/cmdline_flow.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 6263d30..e6648da 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const
> struct token *token,
>  		action_rss_data->queue[i] = i;
>  	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>  	    ctx->port != (portid_t)RTE_PORT_ALL) {
> +		struct rte_eth_rss_conf rss_conf = {0};
>  		struct rte_eth_dev_info info;
>  		int ret2;
> 
> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const
> struct token *token,
>  		action_rss_data->conf.key_len =
>  			RTE_MIN(sizeof(action_rss_data->key),
>  				info.hash_key_size);
> +
> +		rss_conf.rss_key_len = sizeof(action_rss_data->key);
> +		rss_conf.rss_key = action_rss_data->key;
> +		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port,
> &rss_conf);
> +		if (ret2 != 0)
> +			return ret2;
> +		action_rss_data->conf.key = rss_conf.rss_key;
>  	}
>  	action->conf = &action_rss_data->conf;
>  	return ret;
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration
  2020-09-22  9:51 ` Phil Yang
@ 2020-09-22 13:50   ` oulijun
  2020-09-22 15:44     ` Phil Yang
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-09-22 13:50 UTC (permalink / raw)
  To: Phil Yang, wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit
  Cc: dev, linuxarm, nd



ÔÚ 2020/9/22 17:51, Phil Yang дµÀ:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
>> Sent: Thursday, September 10, 2020 9:51 AM
>> To: wenzhuo.lu@intel.com; beilei.xing@intel.com;
>> adrien.mazarguil@6wind.com; ferruh.yigit@intel.com
>> Cc: dev@dpdk.org; linuxarm@huawei.com
>> Subject: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key
>> configuration
> 
> Hi Lijun,
> 
> Please fix the coding style issues.
> 
> "Must be a reply to the first patch (--in-reply-to)."
> 
> 
>>
>> When a user runs a flow create cmd to configure an RSS rule
>> with specifying the empty rss actions in testpmd, this mean
>                                                                                                      ^^^ means
>> that the flow gets the default RSS functions from the valid
>> NIC default RSS hash key. However, the testpmd is not set
>                                                                                            ^^^ is set xxx incorrectly
>> the default RSS key incorrectly when RSS key is not specified
>> in flow create cmd.
> 
> Use the NIC valid default RSS key instead of the testpmd dummy RSS key in the flow configuration when the RSS key is not specified in the flow rule. If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the default key.
> 
> Is that good to put it in this way? Because I think it is not a bug, your patch offers an approach to update the default testpmd RSS key.
> 
Do you have any better advice?  or don't use my approach? I think the 
previous methods are easy to misunderstand.Can we use the NUL KEY 
solution and fix the problem that the length is 0 and the RSS key is not 
NULL ?

Thanks
Lijun Ou
> I would also suggest making the commit message shorter and move the test result into the cover letter.
> Because the checkepatch tool is not happy with the hex dumped below.
> http://mails.dpdk.org/archives/test-report/2020-September/151395.html
> 
> 
> Thanks,
> Phil
> 
>> the cmdline as flows:
>> 1. first, startup testpmd:
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>> 20C6A42B73BBEAC01FA
>>
>> 2. create a rss rule
>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
>> types ipv4-udp end queues end / end
>>
>> 3. show rss-hash key
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-udp udp
>> RSS key:
>> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
>> 76657272696465
>>
>> Now, it uses rte_eth_dev_rss_hash_conf_get to correctly the
>> default rss key. the cmdline and result as flows:
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F2
>> 0C
>> 6A42B73BBEAC01FA
>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
>> types ipv4-udp end queues end / end
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-udp udp
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>> 20C6A42B73BBEAC01FA
>>
>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V2->V3:
>> -fix checkpatch warning.
>>
>> V1->V2:
>> -fix the commit.
>> ---
>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index 6263d30..e6648da 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const
>> struct token *token,
>>   		action_rss_data->queue[i] = i;
>>   	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>>   	    ctx->port != (portid_t)RTE_PORT_ALL) {
>> +		struct rte_eth_rss_conf rss_conf = {0};
>>   		struct rte_eth_dev_info info;
>>   		int ret2;
>>
>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const
>> struct token *token,
>>   		action_rss_data->conf.key_len =
>>   			RTE_MIN(sizeof(action_rss_data->key),
>>   				info.hash_key_size);
>> +
>> +		rss_conf.rss_key_len = sizeof(action_rss_data->key);
>> +		rss_conf.rss_key = action_rss_data->key;
>> +		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port,
>> &rss_conf);
>> +		if (ret2 != 0)
>> +			return ret2;
>> +		action_rss_data->conf.key = rss_conf.rss_key;
>>   	}
>>   	action->conf = &action_rss_data->conf;
>>   	return ret;
>> --
>> 2.7.4
> 
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration
  2020-09-22 13:50   ` oulijun
@ 2020-09-22 15:44     ` Phil Yang
  0 siblings, 0 replies; 44+ messages in thread
From: Phil Yang @ 2020-09-22 15:44 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit
  Cc: dev, linuxarm, nd, nd

dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou writes:


> >> Subject: [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key
> >> configuration
> >
> > Hi Lijun,
> >
> > Please fix the coding style issues.
> >
> > "Must be a reply to the first patch (--in-reply-to)."
> >
> >
> >>
> >> When a user runs a flow create cmd to configure an RSS rule
> >> with specifying the empty rss actions in testpmd, this mean
> >                                                                                                      ^^^ means
> >> that the flow gets the default RSS functions from the valid
> >> NIC default RSS hash key. However, the testpmd is not set
> >                                                                                            ^^^ is set xxx incorrectly
> >> the default RSS key incorrectly when RSS key is not specified
> >> in flow create cmd.
> >
> > Use the NIC valid default RSS key instead of the testpmd dummy RSS key in
> the flow configuration when the RSS key is not specified in the flow rule. If
> the NIC RSS key is invalid, it will use testpmd dummy RSS key as the default
> key.
> >
> > Is that good to put it in this way? Because I think it is not a bug, your patch
> offers an approach to update the default testpmd RSS key.
> >
> Do you have any better advice?  or don't use my approach? I think the


No, I think you misunderstood me. I agree with your proposal and your patch looks good to me.
My suggestion is to reword the commit message to highlight that you mare making testpmd use the valid NIC RSS key as the default flow RSS key in this patch. 
In my perspective, if you don't specify any RSS key in your flow rule, it should allow any available RSS key work as the default key. 
So use a dummy RSS key is correct as well.


> previous methods are easy to misunderstand.Can we use the NUL KEY
> solution and fix the problem that the length is 0 and the RSS key is not
> NULL ?
> 
> Thanks
> Lijun Ou
> > I would also suggest making the commit message shorter and move the
> test result into the cover letter.
> > Because the checkepatch tool is not happy with the hex dumped below.
> > http://mails.dpdk.org/archives/test-report/2020-September/151395.html
> >
> >
> > Thanks,
> > Phil
> >
> >> the cmdline as flows:
> >> 1. first, startup testpmd:
> >> testpmd> show port 0 rss-hash key
> >> RSS functions:
> >>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> >> RSS key:
> >>
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> >> 20C6A42B73BBEAC01FA
> >>
> >> 2. create a rss rule
> >> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> >> types ipv4-udp end queues end / end
> >>
> >> 3. show rss-hash key
> >> testpmd> show port 0 rss-hash key
> >> RSS functions:
> >>   all ipv4-udp udp
> >> RSS key:
> >>
> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
> >> 76657272696465
> >>
> >> Now, it uses rte_eth_dev_rss_hash_conf_get to correctly the
> >> default rss key. the cmdline and result as flows:
> >> testpmd> show port 0 rss-hash key
> >> RSS functions:
> >>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> >> RSS key:
> >>
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F2
> >> 0C
> >> 6A42B73BBEAC01FA
> >> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> >> types ipv4-udp end queues end / end
> >> testpmd> show port 0 rss-hash key
> >> RSS functions:
> >>   all ipv4-udp udp
> >> RSS key:
> >>
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> >> 20C6A42B73BBEAC01FA
> >>
> >> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >> ---
> >> V2->V3:
> >> -fix checkpatch warning.
> >>
> >> V1->V2:
> >> -fix the commit.
> >> ---
> >>   app/test-pmd/cmdline_flow.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
> pmd/cmdline_flow.c
> >> index 6263d30..e6648da 100644
> >> --- a/app/test-pmd/cmdline_flow.c
> >> +++ b/app/test-pmd/cmdline_flow.c
> >> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const
> >> struct token *token,
> >>   		action_rss_data->queue[i] = i;
> >>   	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
> >>   	    ctx->port != (portid_t)RTE_PORT_ALL) {
> >> +		struct rte_eth_rss_conf rss_conf = {0};
> >>   		struct rte_eth_dev_info info;
> >>   		int ret2;
> >>
> >> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const
> >> struct token *token,
> >>   		action_rss_data->conf.key_len =
> >>   			RTE_MIN(sizeof(action_rss_data->key),
> >>   				info.hash_key_size);
> >> +
> >> +		rss_conf.rss_key_len = sizeof(action_rss_data->key);
> >> +		rss_conf.rss_key = action_rss_data->key;
> >> +		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port,
> >> &rss_conf);
> >> +		if (ret2 != 0)
> >> +			return ret2;
> >> +		action_rss_data->conf.key = rss_conf.rss_key;
> >>   	}
> >>   	action->conf = &action_rss_data->conf;
> >>   	return ret;
> >> --
> >> 2.7.4
> >
> > .
> >

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-09-10  1:51 [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration Lijun Ou
  2020-09-22  9:51 ` Phil Yang
@ 2020-09-24 13:45 ` Lijun Ou
  2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
  2020-09-30 13:17   ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Ferruh Yigit
  1 sibling, 2 replies; 44+ messages in thread
From: Lijun Ou @ 2020-09-24 13:45 UTC (permalink / raw)
  To: wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit; +Cc: dev, linuxarm

Consider the follow usage with testpmd:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
 all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end

3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
 all ipv4-udp udp
RSS key:
74657374706D6427732064656661756C74205253532068617368206B65792C206F
76657272696465

As a result, the step 3 with RSS key and the step 1 RSS key
is not the same. The patch[1] to solve the above problems.

Lijun Ou (1):
  app/testpmd: fix the default RSS key configuration

 app/test-pmd/cmdline_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-09-24 13:45 ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Lijun Ou
@ 2020-09-24 13:45   ` Lijun Ou
  2020-09-29 15:35     ` Phil Yang
                       ` (3 more replies)
  2020-09-30 13:17   ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Ferruh Yigit
  1 sibling, 4 replies; 44+ messages in thread
From: Lijun Ou @ 2020-09-24 13:45 UTC (permalink / raw)
  To: wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit; +Cc: dev, linuxarm

It use the NIC valid default RSS key instead of the testpmd
dummy RSS key in the flow configuration when the RSS key is
not specified in the flow rule. If the NIC RSS key is
invalid, it will use testpmd dummy RSS key as the default
key.

Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
Cc: stable@dpdk.org

Signed-off-by: Lijun Ou <oulijun@huawei.com>
---
V3->V4:
-fix checkpatch warning and shorter commit content.

V2->V3:
-fix checkpatch warning.

V1->V2:
-fix the commit.
---
 app/test-pmd/cmdline_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 6263d30..e6648da 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->queue[i] = i;
 	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
 	    ctx->port != (portid_t)RTE_PORT_ALL) {
+		struct rte_eth_rss_conf rss_conf = {0};
 		struct rte_eth_dev_info info;
 		int ret2;
 
@@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->conf.key_len =
 			RTE_MIN(sizeof(action_rss_data->key),
 				info.hash_key_size);
+
+		rss_conf.rss_key_len = sizeof(action_rss_data->key);
+		rss_conf.rss_key = action_rss_data->key;
+		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
+		if (ret2 != 0)
+			return ret2;
+		action_rss_data->conf.key = rss_conf.rss_key;
 	}
 	action->conf = &action_rss_data->conf;
 	return ret;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
@ 2020-09-29 15:35     ` Phil Yang
  2020-09-30 12:57     ` Ferruh Yigit
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 44+ messages in thread
From: Phil Yang @ 2020-09-29 15:35 UTC (permalink / raw)
  To: Lijun Ou, wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit
  Cc: dev, linuxarm, nd, nd

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Lijun Ou
> Sent: Thursday, September 24, 2020 9:45 PM
> To: wenzhuo.lu@intel.com; beilei.xing@intel.com;
> adrien.mazarguil@6wind.com; ferruh.yigit@intel.com
> Cc: dev@dpdk.org; linuxarm@huawei.com
> Subject: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key
> configuration
> 
> It use the NIC valid default RSS key instead of the testpmd
> dummy RSS key in the flow configuration when the RSS key is
> not specified in the flow rule. If the NIC RSS key is
> invalid, it will use testpmd dummy RSS key as the default
> key.
> 
> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lijun Ou <oulijun@huawei.com>

It looks good to me. Thanks.

Reviewed-by: Phil Yang <phil.yang@arm.com>


> ---
> V3->V4:
> -fix checkpatch warning and shorter commit content.
> 
> V2->V3:
> -fix checkpatch warning.
> 
> V1->V2:
> -fix the commit.
> ---
>  app/test-pmd/cmdline_flow.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 6263d30..e6648da 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const
> struct token *token,
>  		action_rss_data->queue[i] = i;
>  	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>  	    ctx->port != (portid_t)RTE_PORT_ALL) {
> +		struct rte_eth_rss_conf rss_conf = {0};
>  		struct rte_eth_dev_info info;
>  		int ret2;
> 
> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const
> struct token *token,
>  		action_rss_data->conf.key_len =
>  			RTE_MIN(sizeof(action_rss_data->key),
>  				info.hash_key_size);
> +
> +		rss_conf.rss_key_len = sizeof(action_rss_data->key);
> +		rss_conf.rss_key = action_rss_data->key;
> +		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port,
> &rss_conf);
> +		if (ret2 != 0)
> +			return ret2;
> +		action_rss_data->conf.key = rss_conf.rss_key;
>  	}
>  	action->conf = &action_rss_data->conf;
>  	return ret;
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
  2020-09-29 15:35     ` Phil Yang
@ 2020-09-30 12:57     ` Ferruh Yigit
  2020-10-09 11:55       ` oulijun
  2020-09-30 13:36     ` Ferruh Yigit
  2020-10-15 12:41     ` [dpdk-dev] [PATCH v5] " Lijun Ou
  3 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-09-30 12:57 UTC (permalink / raw)
  To: Lijun Ou, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm

On 9/24/2020 2:45 PM, Lijun Ou wrote:
> It use the NIC valid default RSS key instead of the testpmd
> dummy RSS key in the flow configuration when the RSS key is
> not specified in the flow rule. If the NIC RSS key is
> invalid, it will use testpmd dummy RSS key as the default
> key.

Can you please describe the impact, what fails without this fix?

> 
> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V3->V4:
> -fix checkpatch warning and shorter commit content.
> 
> V2->V3:
> -fix checkpatch warning.
> 
> V1->V2:
> -fix the commit.
> ---
>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 6263d30..e6648da 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
>   		action_rss_data->queue[i] = i;
>   	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>   	    ctx->port != (portid_t)RTE_PORT_ALL) {
> +		struct rte_eth_rss_conf rss_conf = {0};
>   		struct rte_eth_dev_info info;
>   		int ret2;
>   
> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
>   		action_rss_data->conf.key_len =
>   			RTE_MIN(sizeof(action_rss_data->key),
>   				info.hash_key_size);
> +
> +		rss_conf.rss_key_len = sizeof(action_rss_data->key);
> +		rss_conf.rss_key = action_rss_data->key;

'rss_conf.rss_key_len' is the input parameter, it looks like it has been used as 
the size of the 'rss_key', but as far as I can see it is not.

Because of this if 'info.hash_key_size' is bigger than 
'sizeof(action_rss_data->key)', won't PMD overwrite the stack for the remaining 
bytes?

Can you please check?

> +		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
> +		if (ret2 != 0)
> +			return ret2;

If PMD not implemented the 'rss_hash_conf_get' dev_ops, should it fail the RSS 
action?

> +		action_rss_data->conf.key = rss_conf.rss_key;
>   	}
>   	action->conf = &action_rss_data->conf;
>   	return ret;
> 


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-09-24 13:45 ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Lijun Ou
  2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
@ 2020-09-30 13:17   ` Ferruh Yigit
  2020-10-09 12:09     ` oulijun
  1 sibling, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-09-30 13:17 UTC (permalink / raw)
  To: Lijun Ou, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm

On 9/24/2020 2:45 PM, Lijun Ou wrote:
> Consider the follow usage with testpmd:
> 1. first, startup testpmd:
> testpmd> show port 0 rss-hash key
> RSS functions:
>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> 20C6A42B73BBEAC01FA
> 2. create a rss rule
> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> types ipv4-udp end queues end / end
> 
> 3. show rss-hash key
> testpmd> show port 0 rss-hash key
> RSS functions:
>   all ipv4-udp udp
> RSS key:
> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
> 76657272696465
> 
> As a result, the step 3 with RSS key and the step 1 RSS key
> is not the same. The patch[1] to solve the above problems.
> 

This is interesting, can you please provide above information in the commit log too?

Also can you please provide the details on why this happens, callstack can help?

Thanks,
ferruh


> Lijun Ou (1):
>    app/testpmd: fix the default RSS key configuration
> 
>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
  2020-09-29 15:35     ` Phil Yang
  2020-09-30 12:57     ` Ferruh Yigit
@ 2020-09-30 13:36     ` Ferruh Yigit
  2020-10-09 11:59       ` oulijun
  2020-10-15 12:41     ` [dpdk-dev] [PATCH v5] " Lijun Ou
  3 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-09-30 13:36 UTC (permalink / raw)
  To: Lijun Ou, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm

On 9/24/2020 2:45 PM, Lijun Ou wrote:
> It use the NIC valid default RSS key instead of the testpmd
> dummy RSS key in the flow configuration when the RSS key is
> not specified in the flow rule. If the NIC RSS key is
> invalid, it will use testpmd dummy RSS key as the default
> key.
> 
> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> ---
> V3->V4:
> -fix checkpatch warning and shorter commit content.
> 
> V2->V3:
> -fix checkpatch warning.
> 
> V1->V2:
> -fix the commit.
> ---
>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 6263d30..e6648da 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
>   		action_rss_data->queue[i] = i;
>   	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>   	    ctx->port != (portid_t)RTE_PORT_ALL) {
> +		struct rte_eth_rss_conf rss_conf = {0};
>   		struct rte_eth_dev_info info;
>   		int ret2;
>   
> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
>   		action_rss_data->conf.key_len =
>   			RTE_MIN(sizeof(action_rss_data->key),
>   				info.hash_key_size);
> +
> +		rss_conf.rss_key_len = sizeof(action_rss_data->key);
> +		rss_conf.rss_key = action_rss_data->key;
> +		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
> +		if (ret2 != 0)
> +			return ret2;
> +		action_rss_data->conf.key = rss_conf.rss_key;

Do you need this last assignment at all?
'rss_conf.rss_key' point to 'action_rss_data->key'
'action_rss_data->conf.key' already point to 'action_rss_data->key'
so it just assigns same value back, no?


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-09-30 12:57     ` Ferruh Yigit
@ 2020-10-09 11:55       ` oulijun
  2020-10-09 18:27         ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-09 11:55 UTC (permalink / raw)
  To: Ferruh Yigit, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm



在 2020/9/30 20:57, Ferruh Yigit 写道:
> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>> It use the NIC valid default RSS key instead of the testpmd
>> dummy RSS key in the flow configuration when the RSS key is
>> not specified in the flow rule. If the NIC RSS key is
>> invalid, it will use testpmd dummy RSS key as the default
>> key.
> 
> Can you please describe the impact, what fails without this fix?
> 
Hi, Ferruh
   According to Phil Yang's suggestion, I've put the impact description 
in the cover letter.
    When a user runs a flow create cmd to configure an RSS rule
with specifying the empty rss actions in testpmd, this mean
that the flow gets the default RSS functions from the valid
NIC default RSS hash key. However, the testpmd is not set
the default RSS key incorrectly when RSS key is not specified
in flow create cmd.
    After the testpmd is started, run the flow create cmd without 
specifying the RSS hash key. Ensure that RSS hash key queried after
the RSS flow is configured is the same as the default key after the
testpmd is start.

>>
>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V3->V4:
>> -fix checkpatch warning and shorter commit content.
>>
>> V2->V3:
>> -fix checkpatch warning.
>>
>> V1->V2:
>> -fix the commit.
>> ---
>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index 6263d30..e6648da 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const 
>> struct token *token,
>>           action_rss_data->queue[i] = i;
>>       if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>>           ctx->port != (portid_t)RTE_PORT_ALL) {
>> +        struct rte_eth_rss_conf rss_conf = {0};
>>           struct rte_eth_dev_info info;
>>           int ret2;
>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const 
>> struct token *token,
>>           action_rss_data->conf.key_len =
>>               RTE_MIN(sizeof(action_rss_data->key),
>>                   info.hash_key_size);
>> +
>> +        rss_conf.rss_key_len = sizeof(action_rss_data->key);
>> +        rss_conf.rss_key = action_rss_data->key;
> 
> 'rss_conf.rss_key_len' is the input parameter, it looks like it has been 
> used as the size of the 'rss_key', but as far as I can see it is not.
> 
Yes, rss_key_len and rss_key is configured independently. In fact, when 
the user configures the rss_key, we should know rss_key_len.
> Because of this if 'info.hash_key_size' is bigger than 
> 'sizeof(action_rss_data->key)', won't PMD overwrite the stack for the 
> remaining bytes?
if info.hash_key_size and action_rss_data->key is set used RSS flow cmd 
when info.hash_key_size is bigger than sizeof(action_rss_data->key), the 
driver will return an error.
> 
> Can you please check?
> 
>> +        ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
>> +        if (ret2 != 0)
>> +            return ret2;
> 
> If PMD not implemented the 'rss_hash_conf_get' dev_ops, should it fail 
> the RSS action?
Yes, When the RSS hash key is not specified in the RSS flow, ensure that 
the RSS hash key remains unchanged and is the same as that configured in 
the hardware when the testpmd is started.
> 
>> +        action_rss_data->conf.key = rss_conf.rss_key;
>>       }
>>       action->conf = &action_rss_data->conf;
>>       return ret;
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-09-30 13:36     ` Ferruh Yigit
@ 2020-10-09 11:59       ` oulijun
  2020-10-09 18:36         ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-09 11:59 UTC (permalink / raw)
  To: Ferruh Yigit, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm



在 2020/9/30 21:36, Ferruh Yigit 写道:
> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>> It use the NIC valid default RSS key instead of the testpmd
>> dummy RSS key in the flow configuration when the RSS key is
>> not specified in the flow rule. If the NIC RSS key is
>> invalid, it will use testpmd dummy RSS key as the default
>> key.
>>
>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> ---
>> V3->V4:
>> -fix checkpatch warning and shorter commit content.
>>
>> V2->V3:
>> -fix checkpatch warning.
>>
>> V1->V2:
>> -fix the commit.
>> ---
>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>> index 6263d30..e6648da 100644
>> --- a/app/test-pmd/cmdline_flow.c
>> +++ b/app/test-pmd/cmdline_flow.c
>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const 
>> struct token *token,
>>           action_rss_data->queue[i] = i;
>>       if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>>           ctx->port != (portid_t)RTE_PORT_ALL) {
>> +        struct rte_eth_rss_conf rss_conf = {0};
>>           struct rte_eth_dev_info info;
>>           int ret2;
>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const 
>> struct token *token,
>>           action_rss_data->conf.key_len =
>>               RTE_MIN(sizeof(action_rss_data->key),
>>                   info.hash_key_size);
>> +
>> +        rss_conf.rss_key_len = sizeof(action_rss_data->key);
>> +        rss_conf.rss_key = action_rss_data->key;
>> +        ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
>> +        if (ret2 != 0)
>> +            return ret2;
>> +        action_rss_data->conf.key = rss_conf.rss_key;
> 
> Do you need this last assignment at all?
> 'rss_conf.rss_key' point to 'action_rss_data->key'
> 'action_rss_data->conf.key' already point to 'action_rss_data->key'
> so it just assigns same value back, no?
> 
You need to call rte_eth_dev_rss_hash_conf_get to obtain the RSS hash 
key configured in the hardware as the default key.
Currently, the testpmd uses a key that is different from that configured 
in the hardware after the testpmd is started as the default key when 
users crearte a RSS flow rule without specifying a RSS hash key.
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-09-30 13:17   ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Ferruh Yigit
@ 2020-10-09 12:09     ` oulijun
  2020-10-09 18:52       ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-09 12:09 UTC (permalink / raw)
  To: Ferruh Yigit, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm



在 2020/9/30 21:17, Ferruh Yigit 写道:
> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>> Consider the follow usage with testpmd:
>> 1. first, startup testpmd:
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>> 20C6A42B73BBEAC01FA
>> 2. create a rss rule
>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions 
>> rss \
>> types ipv4-udp end queues end / end
>>
>> 3. show rss-hash key
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-udp udp
>> RSS key:
>> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
>> 76657272696465
>>
>> As a result, the step 3 with RSS key and the step 1 RSS key
>> is not the same. The patch[1] to solve the above problems.
>>
> 
> This is interesting, can you please provide above information in the 
> commit log too?
> 
Yes, I submitted detailed operation information in patch v3 as a commit, 
and Yang suggested that the operation information be included in the 
cover letter.
> Also can you please provide the details on why this happens, callstack 
> can help?
> 
When you start the testpmd, the pmd driver initializes the RSS 
configuration. Generally, the recommended RSS hash key is used as the 
default key in the driver. In addition, the default key is different 
from the default RSS flow in testpmd without specifying RSS hash key.
So, if you do not specify the RSS key when creating an RSS rule, the 
testpmd uses the default key as the default RSS key of the RSS rule.As a 
result, you may mistakenly consider that the RSS key in use is the valid 
default key of the NIC, actually, the key and the valid default key of 
the NIC are two values.
> Thanks,
> ferruh
> 
> 
>> Lijun Ou (1):
>>    app/testpmd: fix the default RSS key configuration
>>
>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-10-09 11:55       ` oulijun
@ 2020-10-09 18:27         ` Ferruh Yigit
  2020-10-09 18:54           ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-09 18:27 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm

On 10/9/2020 12:55 PM, oulijun wrote:
> 
> 
> 在 2020/9/30 20:57, Ferruh Yigit 写道:
>> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>>> It use the NIC valid default RSS key instead of the testpmd
>>> dummy RSS key in the flow configuration when the RSS key is
>>> not specified in the flow rule. If the NIC RSS key is
>>> invalid, it will use testpmd dummy RSS key as the default
>>> key.
>>
>> Can you please describe the impact, what fails without this fix?
>>
> Hi, Ferruh
>    According to Phil Yang's suggestion, I've put the impact description in the 
> cover letter.
>     When a user runs a flow create cmd to configure an RSS rule
> with specifying the empty rss actions in testpmd, this mean
> that the flow gets the default RSS functions from the valid
> NIC default RSS hash key. However, the testpmd is not set
> the default RSS key incorrectly when RSS key is not specified
> in flow create cmd.
>     After the testpmd is started, run the flow create cmd without specifying the 
> RSS hash key. Ensure that RSS hash key queried after
> the RSS flow is configured is the same as the default key after the
> testpmd is start.
> 
>>>
>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> ---
>>> V3->V4:
>>> -fix checkpatch warning and shorter commit content.
>>>
>>> V2->V3:
>>> -fix checkpatch warning.
>>>
>>> V1->V2:
>>> -fix the commit.
>>> ---
>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>> index 6263d30..e6648da 100644
>>> --- a/app/test-pmd/cmdline_flow.c
>>> +++ b/app/test-pmd/cmdline_flow.c
>>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct 
>>> token *token,
>>>           action_rss_data->queue[i] = i;
>>>       if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>>>           ctx->port != (portid_t)RTE_PORT_ALL) {
>>> +        struct rte_eth_rss_conf rss_conf = {0};
>>>           struct rte_eth_dev_info info;
>>>           int ret2;
>>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct 
>>> token *token,
>>>           action_rss_data->conf.key_len =
>>>               RTE_MIN(sizeof(action_rss_data->key),
>>>                   info.hash_key_size);
>>> +
>>> +        rss_conf.rss_key_len = sizeof(action_rss_data->key);
>>> +        rss_conf.rss_key = action_rss_data->key;
>>
>> 'rss_conf.rss_key_len' is the input parameter, it looks like it has been used 
>> as the size of the 'rss_key', but as far as I can see it is not.
>>
> Yes, rss_key_len and rss_key is configured independently. In fact, when the user 
> configures the rss_key, we should know rss_key_len.
>> Because of this if 'info.hash_key_size' is bigger than 
>> 'sizeof(action_rss_data->key)', won't PMD overwrite the stack for the 
>> remaining bytes?
> if info.hash_key_size and action_rss_data->key is set used RSS flow cmd when 
> info.hash_key_size is bigger than sizeof(action_rss_data->key), the driver will 
> return an error.
 >

How can driver return an error?
Both 'rss_conf.rss_key_len' & 'rss_conf.rss_key' filled by driver, and driver 
expect 'rss_conf.rss_key' is big enough to hold its key.
Here 'rss_conf.rss_key' points to fixed size 'action_rss_data->key' array 
without checking the driver key length.

Where this driver check happens, am I missing it?

>>
>> Can you please check?
>>
>>> +        ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
>>> +        if (ret2 != 0)
>>> +            return ret2;
>>
>> If PMD not implemented the 'rss_hash_conf_get' dev_ops, should it fail the RSS 
>> action?
> Yes, When the RSS hash key is not specified in the RSS flow, ensure that the RSS 
> hash key remains unchanged and is the same as that configured in the hardware 
> when the testpmd is started.

OK, but this means any PMD not implemented dev_ops to get RSS key won't able to 
use rte flow RSS action via testpmd, not sure if this is right.

What this patch does is read the device configured RSS key, and feed it back to 
rte flow RSS action, to be sure device key is not changed.

Instead, can we update the rte flow RSS action to not change the key if it is 
not provided, and don't provide one in the testpmd by default.

What do you think?

>>
>>> +        action_rss_data->conf.key = rss_conf.rss_key;
>>>       }
>>>       action->conf = &action_rss_data->conf;
>>>       return ret;
>>>
>>
>> .
>>


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-10-09 11:59       ` oulijun
@ 2020-10-09 18:36         ` Ferruh Yigit
  0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-09 18:36 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm

On 10/9/2020 12:59 PM, oulijun wrote:
> 
> 
> 在 2020/9/30 21:36, Ferruh Yigit 写道:
>> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>>> It use the NIC valid default RSS key instead of the testpmd
>>> dummy RSS key in the flow configuration when the RSS key is
>>> not specified in the flow rule. If the NIC RSS key is
>>> invalid, it will use testpmd dummy RSS key as the default
>>> key.
>>>
>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> ---
>>> V3->V4:
>>> -fix checkpatch warning and shorter commit content.
>>>
>>> V2->V3:
>>> -fix checkpatch warning.
>>>
>>> V1->V2:
>>> -fix the commit.
>>> ---
>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>> index 6263d30..e6648da 100644
>>> --- a/app/test-pmd/cmdline_flow.c
>>> +++ b/app/test-pmd/cmdline_flow.c
>>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct 
>>> token *token,
>>>           action_rss_data->queue[i] = i;
>>>       if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>>>           ctx->port != (portid_t)RTE_PORT_ALL) {
>>> +        struct rte_eth_rss_conf rss_conf = {0};
>>>           struct rte_eth_dev_info info;
>>>           int ret2;
>>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct 
>>> token *token,
>>>           action_rss_data->conf.key_len =
>>>               RTE_MIN(sizeof(action_rss_data->key),
>>>                   info.hash_key_size);
>>> +
>>> +        rss_conf.rss_key_len = sizeof(action_rss_data->key);
>>> +        rss_conf.rss_key = action_rss_data->key;
>>> +        ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
>>> +        if (ret2 != 0)
>>> +            return ret2;
>>> +        action_rss_data->conf.key = rss_conf.rss_key;
>>
>> Do you need this last assignment at all?
>> 'rss_conf.rss_key' point to 'action_rss_data->key'
>> 'action_rss_data->conf.key' already point to 'action_rss_data->key'
>> so it just assigns same value back, no?
>>
> You need to call rte_eth_dev_rss_hash_conf_get to obtain the RSS hash key 
> configured in the hardware as the default key.
> Currently, the testpmd uses a key that is different from that configured in the 
> hardware after the testpmd is started as the default key when users crearte a 
> RSS flow rule without specifying a RSS hash key.
>

I got this part, I am saying why you have the following assignment:
"action_rss_data->conf.key = rss_conf.rss_key;"

'action_rss_data->conf.key' already points to 'action_rss_data->key' [1]

later 'rss_conf.rss_key' pointer points 'action_rss_data->key' [2]
This is used to fill the RSS key.

At this stage both 'action_rss_data->conf.key' & 'rss_conf.rss_key' are point to 
exact same address.
So what is the point of assigning 'action_rss_data->conf.key' to 'rss_conf.rss_key'?


[1]
  *action_rss_data = (struct action_rss_data){
    conf = (struct rte_flow_action_rss){
      ...
      .key = action_rss_data->key,
      ...
   }
   ...
  }


[2]
rss_conf.rss_key = action_rss_data->key;

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-10-09 12:09     ` oulijun
@ 2020-10-09 18:52       ` Ferruh Yigit
  2020-10-10  3:07         ` Phil Yang
  2020-10-14  6:15         ` oulijun
  0 siblings, 2 replies; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-09 18:52 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil, Phil Yang
  Cc: dev, linuxarm

On 10/9/2020 1:09 PM, oulijun wrote:
> 
> 
> 在 2020/9/30 21:17, Ferruh Yigit 写道:
>> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>>> Consider the follow usage with testpmd:
>>> 1. first, startup testpmd:
>>> testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>> RSS key:
>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>>> 20C6A42B73BBEAC01FA
>>> 2. create a rss rule
>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
>>> types ipv4-udp end queues end / end
>>>
>>> 3. show rss-hash key
>>> testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>   all ipv4-udp udp
>>> RSS key:
>>> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
>>> 76657272696465
>>>
>>> As a result, the step 3 with RSS key and the step 1 RSS key
>>> is not the same. The patch[1] to solve the above problems.
>>>
>>
>> This is interesting, can you please provide above information in the commit 
>> log too?
>>
> Yes, I submitted detailed operation information in patch v3 as a commit, and 
> Yang suggested that the operation information be included in the cover letter.
 >

OK, understood.
Only, commands helped me to understand the problem, it is easy to grasp the 
issue with samples, so I thought it may help others later in if it is in the 
commit log, since cover letter won't be visible in the git repo.

@Phil, will you be OK to have them in the commit log if the checkpatch warnings 
fixed?

>> Also can you please provide the details on why this happens, callstack can help?
>>
> When you start the testpmd, the pmd driver initializes the RSS configuration. 
> Generally, the recommended RSS hash key is used as the default key in the 
> driver. In addition, the default key is different from the default RSS flow in 
> testpmd without specifying RSS hash key.
> So, if you do not specify the RSS key when creating an RSS rule, the testpmd 
> uses the default key as the default RSS key of the RSS rule.As a result, you may 
> mistakenly consider that the RSS key in use is the valid default key of the NIC, 
> actually, the key and the valid default key of the NIC are two values.

Above description looks good, can you include this to the commit log please?

>> Thanks,
>> ferruh
>>
>>
>>> Lijun Ou (1):
>>>    app/testpmd: fix the default RSS key configuration
>>>
>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>
>> .
>>


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration
  2020-10-09 18:27         ` Ferruh Yigit
@ 2020-10-09 18:54           ` Ferruh Yigit
  0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-09 18:54 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil; +Cc: dev, linuxarm, Ori Kam

On 10/9/2020 7:27 PM, Ferruh Yigit wrote:
> On 10/9/2020 12:55 PM, oulijun wrote:
>>
>>
>> 在 2020/9/30 20:57, Ferruh Yigit 写道:
>>> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>>>> It use the NIC valid default RSS key instead of the testpmd
>>>> dummy RSS key in the flow configuration when the RSS key is
>>>> not specified in the flow rule. If the NIC RSS key is
>>>> invalid, it will use testpmd dummy RSS key as the default
>>>> key.
>>>
>>> Can you please describe the impact, what fails without this fix?
>>>
>> Hi, Ferruh
>>    According to Phil Yang's suggestion, I've put the impact description in the 
>> cover letter.
>>     When a user runs a flow create cmd to configure an RSS rule
>> with specifying the empty rss actions in testpmd, this mean
>> that the flow gets the default RSS functions from the valid
>> NIC default RSS hash key. However, the testpmd is not set
>> the default RSS key incorrectly when RSS key is not specified
>> in flow create cmd.
>>     After the testpmd is started, run the flow create cmd without specifying 
>> the RSS hash key. Ensure that RSS hash key queried after
>> the RSS flow is configured is the same as the default key after the
>> testpmd is start.
>>
>>>>
>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>> V3->V4:
>>>> -fix checkpatch warning and shorter commit content.
>>>>
>>>> V2->V3:
>>>> -fix checkpatch warning.
>>>>
>>>> V1->V2:
>>>> -fix the commit.
>>>> ---
>>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
>>>> index 6263d30..e6648da 100644
>>>> --- a/app/test-pmd/cmdline_flow.c
>>>> +++ b/app/test-pmd/cmdline_flow.c
>>>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct 
>>>> token *token,
>>>>           action_rss_data->queue[i] = i;
>>>>       if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
>>>>           ctx->port != (portid_t)RTE_PORT_ALL) {
>>>> +        struct rte_eth_rss_conf rss_conf = {0};
>>>>           struct rte_eth_dev_info info;
>>>>           int ret2;
>>>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct 
>>>> token *token,
>>>>           action_rss_data->conf.key_len =
>>>>               RTE_MIN(sizeof(action_rss_data->key),
>>>>                   info.hash_key_size);
>>>> +
>>>> +        rss_conf.rss_key_len = sizeof(action_rss_data->key);
>>>> +        rss_conf.rss_key = action_rss_data->key;
>>>
>>> 'rss_conf.rss_key_len' is the input parameter, it looks like it has been used 
>>> as the size of the 'rss_key', but as far as I can see it is not.
>>>
>> Yes, rss_key_len and rss_key is configured independently. In fact, when the 
>> user configures the rss_key, we should know rss_key_len.
>>> Because of this if 'info.hash_key_size' is bigger than 
>>> 'sizeof(action_rss_data->key)', won't PMD overwrite the stack for the 
>>> remaining bytes?
>> if info.hash_key_size and action_rss_data->key is set used RSS flow cmd when 
>> info.hash_key_size is bigger than sizeof(action_rss_data->key), the driver 
>> will return an error.
>  >
> 
> How can driver return an error?
> Both 'rss_conf.rss_key_len' & 'rss_conf.rss_key' filled by driver, and driver 
> expect 'rss_conf.rss_key' is big enough to hold its key.
> Here 'rss_conf.rss_key' points to fixed size 'action_rss_data->key' array 
> without checking the driver key length.
> 
> Where this driver check happens, am I missing it?
> 
>>>
>>> Can you please check?
>>>
>>>> +        ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
>>>> +        if (ret2 != 0)
>>>> +            return ret2;
>>>
>>> If PMD not implemented the 'rss_hash_conf_get' dev_ops, should it fail the 
>>> RSS action?
>> Yes, When the RSS hash key is not specified in the RSS flow, ensure that the 
>> RSS hash key remains unchanged and is the same as that configured in the 
>> hardware when the testpmd is started.
> 
> OK, but this means any PMD not implemented dev_ops to get RSS key won't able to 
> use rte flow RSS action via testpmd, not sure if this is right.
> 
> What this patch does is read the device configured RSS key, and feed it back to 
> rte flow RSS action, to be sure device key is not changed.
> 
> Instead, can we update the rte flow RSS action to not change the key if it is 
> not provided, and don't provide one in the testpmd by default.
> 
> What do you think?
> 

cc'ed Ori for the rte flow related change suggestion.
@Ori, can you please check the problem this patch address and the suggestion above?


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-10-09 18:52       ` Ferruh Yigit
@ 2020-10-10  3:07         ` Phil Yang
  2020-10-14  6:15         ` oulijun
  1 sibling, 0 replies; 44+ messages in thread
From: Phil Yang @ 2020-10-10  3:07 UTC (permalink / raw)
  To: Ferruh Yigit, oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil
  Cc: dev, linuxarm, nd, nd

Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> Sent: Saturday, October 10, 2020 2:53 AM
> To: oulijun <oulijun@huawei.com>; wenzhuo.lu@intel.com;
> beilei.xing@intel.com; adrien.mazarguil@6wind.com; Phil Yang
> <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; linuxarm@huawei.com
> Subject: Re: [PATCH v4] RSS key use with testpmd
> 
> On 10/9/2020 1:09 PM, oulijun wrote:
> >
> >
> > 在 2020/9/30 21:17, Ferruh Yigit 写道:
> >> On 9/24/2020 2:45 PM, Lijun Ou wrote:
> >>> Consider the follow usage with testpmd:
> >>> 1. first, startup testpmd:
> >>> testpmd> show port 0 rss-hash key
> >>> RSS functions:
> >>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> >>> RSS key:
> >>>
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> >>> 20C6A42B73BBEAC01FA
> >>> 2. create a rss rule
> >>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss
> \
> >>> types ipv4-udp end queues end / end
> >>>
> >>> 3. show rss-hash key
> >>> testpmd> show port 0 rss-hash key
> >>> RSS functions:
> >>>   all ipv4-udp udp
> >>> RSS key:
> >>>
> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
> >>> 76657272696465
> >>>
> >>> As a result, the step 3 with RSS key and the step 1 RSS key
> >>> is not the same. The patch[1] to solve the above problems.
> >>>
> >>
> >> This is interesting, can you please provide above information in the
> commit
> >> log too?
> >>
> > Yes, I submitted detailed operation information in patch v3 as a commit,
> and
> > Yang suggested that the operation information be included in the cover
> letter.
>  >
> 
> OK, understood.
> Only, commands helped me to understand the problem, it is easy to grasp
> the
> issue with samples, so I thought it may help others later in if it is in the
> commit log, since cover letter won't be visible in the git repo.
> 
> @Phil, will you be OK to have them in the commit log if the checkpatch
> warnings
> fixed?

Yeah. It is OK. 


> 
> >> Also can you please provide the details on why this happens, callstack can
> help?
> >>
> > When you start the testpmd, the pmd driver initializes the RSS
> configuration.
> > Generally, the recommended RSS hash key is used as the default key in the
> > driver. In addition, the default key is different from the default RSS flow in
> > testpmd without specifying RSS hash key.
> > So, if you do not specify the RSS key when creating an RSS rule, the
> testpmd
> > uses the default key as the default RSS key of the RSS rule.As a result, you
> may
> > mistakenly consider that the RSS key in use is the valid default key of the
> NIC,
> > actually, the key and the valid default key of the NIC are two values.
> 
> Above description looks good, can you include this to the commit log please?
> 
> >> Thanks,
> >> ferruh
> >>
> >>
> >>> Lijun Ou (1):
> >>>    app/testpmd: fix the default RSS key configuration
> >>>
> >>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
> >>>   1 file changed, 8 insertions(+)
> >>>
> >>
> >> .
> >>


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-10-09 18:52       ` Ferruh Yigit
  2020-10-10  3:07         ` Phil Yang
@ 2020-10-14  6:15         ` oulijun
  2020-10-14  8:41           ` Ferruh Yigit
  1 sibling, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-14  6:15 UTC (permalink / raw)
  To: Ferruh Yigit, wenzhuo.lu, beilei.xing, adrien.mazarguil, Phil Yang
  Cc: dev, linuxarm



在 2020/10/10 2:52, Ferruh Yigit 写道:
> On 10/9/2020 1:09 PM, oulijun wrote:
>>
>>
>> 在 2020/9/30 21:17, Ferruh Yigit 写道:
>>> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>>>> Consider the follow usage with testpmd:
>>>> 1. first, startup testpmd:
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>>>> 20C6A42B73BBEAC01FA
>>>> 2. create a rss rule
>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end 
>>>> actions rss \
>>>> types ipv4-udp end queues end / end
>>>>
>>>> 3. show rss-hash key
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-udp udp
>>>> RSS key:
>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
>>>> 76657272696465
>>>>
>>>> As a result, the step 3 with RSS key and the step 1 RSS key
>>>> is not the same. The patch[1] to solve the above problems.
>>>>
>>>
>>> This is interesting, can you please provide above information in the 
>>> commit log too?
>>>
>> Yes, I submitted detailed operation information in patch v3 as a 
>> commit, and Yang suggested that the operation information be included 
>> in the cover letter.
>  >
> 
> OK, understood.
> Only, commands helped me to understand the problem, it is easy to grasp 
> the issue with samples, so I thought it may help others later in if it 
> is in the commit log, since cover letter won't be visible in the git repo.
> 
> @Phil, will you be OK to have them in the commit log if the checkpatch 
> warnings fixed?
> 
>>> Also can you please provide the details on why this happens, 
>>> callstack can help?
>>>
>> When you start the testpmd, the pmd driver initializes the RSS 
>> configuration. Generally, the recommended RSS hash key is used as the 
>> default key in the driver. In addition, the default key is different 
>> from the default RSS flow in testpmd without specifying RSS hash key.
>> So, if you do not specify the RSS key when creating an RSS rule, the 
>> testpmd uses the default key as the default RSS key of the RSS rule.As 
>> a result, you may mistakenly consider that the RSS key in use is the 
>> valid default key of the NIC, actually, the key and the valid default 
>> key of the NIC are two values.
> 
> Above description looks good, can you include this to the commit log 
> please?
> 
Hi,Ferruh
   Your opinion is to put the following comit log in app/testpmd: fix 
the default RSS key configuration?
   Consider the follow usage with testpmd:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end

3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
   all ipv4-udp udp
RSS key:
74657374706D6427732064656661756C74205253532068617368206B65792C206F
76657272696465

>>> Thanks,
>>> ferruh
>>>
>>>
>>>> Lijun Ou (1):
>>>>    app/testpmd: fix the default RSS key configuration
>>>>
>>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>
>>> .
>>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v4] RSS key use with testpmd
  2020-10-14  6:15         ` oulijun
@ 2020-10-14  8:41           ` Ferruh Yigit
  0 siblings, 0 replies; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-14  8:41 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil, Phil Yang
  Cc: dev, linuxarm

On 10/14/2020 7:15 AM, oulijun wrote:
> 
> 
> 在 2020/10/10 2:52, Ferruh Yigit 写道:
>> On 10/9/2020 1:09 PM, oulijun wrote:
>>>
>>>
>>> 在 2020/9/30 21:17, Ferruh Yigit 写道:
>>>> On 9/24/2020 2:45 PM, Lijun Ou wrote:
>>>>> Consider the follow usage with testpmd:
>>>>> 1. first, startup testpmd:
>>>>> testpmd> show port 0 rss-hash key
>>>>> RSS functions:
>>>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>>> RSS key:
>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>>>>> 20C6A42B73BBEAC01FA
>>>>> 2. create a rss rule
>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
>>>>> types ipv4-udp end queues end / end
>>>>>
>>>>> 3. show rss-hash key
>>>>> testpmd> show port 0 rss-hash key
>>>>> RSS functions:
>>>>>   all ipv4-udp udp
>>>>> RSS key:
>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
>>>>> 76657272696465
>>>>>
>>>>> As a result, the step 3 with RSS key and the step 1 RSS key
>>>>> is not the same. The patch[1] to solve the above problems.
>>>>>
>>>>
>>>> This is interesting, can you please provide above information in the commit 
>>>> log too?
>>>>
>>> Yes, I submitted detailed operation information in patch v3 as a commit, and 
>>> Yang suggested that the operation information be included in the cover letter.
>>  >
>>
>> OK, understood.
>> Only, commands helped me to understand the problem, it is easy to grasp the 
>> issue with samples, so I thought it may help others later in if it is in the 
>> commit log, since cover letter won't be visible in the git repo.
>>
>> @Phil, will you be OK to have them in the commit log if the checkpatch 
>> warnings fixed?
>>
>>>> Also can you please provide the details on why this happens, callstack can 
>>>> help?
>>>>
>>> When you start the testpmd, the pmd driver initializes the RSS configuration. 
>>> Generally, the recommended RSS hash key is used as the default key in the 
>>> driver. In addition, the default key is different from the default RSS flow 
>>> in testpmd without specifying RSS hash key.
>>> So, if you do not specify the RSS key when creating an RSS rule, the testpmd 
>>> uses the default key as the default RSS key of the RSS rule.As a result, you 
>>> may mistakenly consider that the RSS key in use is the valid default key of 
>>> the NIC, actually, the key and the valid default key of the NIC are two values.
>>
>> Above description looks good, can you include this to the commit log please?
>>
> Hi,Ferruh
>    Your opinion is to put the following comit log in app/testpmd: fix the 
> default RSS key configuration?
>    Consider the follow usage with testpmd:
> 1. first, startup testpmd:
> testpmd> show port 0 rss-hash key
> RSS functions:
>    all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> 20C6A42B73BBEAC01FA
> 2. create a rss rule
> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> types ipv4-udp end queues end / end
> 
> 3. show rss-hash key
> testpmd> show port 0 rss-hash key
> RSS functions:
>    all ipv4-udp udp
> RSS key:
> 74657374706D6427732064656661756C74205253532068617368206B65792C206F
> 76657272696465
> 

Above description and this sample please? Thanks.

>>>> Thanks,
>>>> ferruh
>>>>
>>>>
>>>>> Lijun Ou (1):
>>>>>    app/testpmd: fix the default RSS key configuration
>>>>>
>>>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++
>>>>>   1 file changed, 8 insertions(+)
>>>>>
>>>>
>>>> .
>>>>
>>
>> .
>>


^ permalink raw reply	[flat|nested] 44+ messages in thread

* [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
                       ` (2 preceding siblings ...)
  2020-09-30 13:36     ` Ferruh Yigit
@ 2020-10-15 12:41     ` " Lijun Ou
  2020-10-15 13:52       ` Ferruh Yigit
       [not found]       ` <20201015195637.26476-1-robot@bytheb.org>
  3 siblings, 2 replies; 44+ messages in thread
From: Lijun Ou @ 2020-10-15 12:41 UTC (permalink / raw)
  To: wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit; +Cc: dev, linuxarm

When start the testpmd, the pmd driver initializes the RSS
configuration. Generally, the recommended RSS hash key is
used as the default key in the driver. In addition, the
default key is different from the default RSS flow in testpmd
without specifying RSS hash key. So. if you do not specify
the RSS key when creating an RSS rule, the testpmd uses the
default key as the default RSS key of the RSS rule. As a result,
you may mistakenly consider that the RSS key in use is the valid
default key of the NIC, actually, the key and the valid default
key of the NIC are two values.

Consider the follow usage with testpmd:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
-6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
-20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end

3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-udp udp
RSS key:
-74657374706D6427732064656661756C74205253532068617368206B65792C206F
-76657272696465

In order to solve the above problems, it use the NIC valid default
RSS key instead of the testpmd dummy RSS key in the flow configuration
when the RSS key is not specified in the flow rule. If the NIC RSS key
is invalid, it will use testpmd dummy RSS key as the default key.

Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
Cc: stable@dpdk.org

Signed-off-by: Lijun Ou <oulijun@huawei.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
V4->V5:
-rewrite the commit log
-add reviewed-by

V3->V4:
-fix checkpatch warning and shorter commit content.

V2->V3:
-fix checkpatch warning.

V1->V2:
-fix the commit.
---
 app/test-pmd/cmdline_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 84bba0f..578555e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4735,6 +4735,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->queue[i] = i;
 	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
 	    ctx->port != (portid_t)RTE_PORT_ALL) {
+		struct rte_eth_rss_conf rss_conf = {0};
 		struct rte_eth_dev_info info;
 		int ret2;
 
@@ -4745,6 +4746,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->conf.key_len =
 			RTE_MIN(sizeof(action_rss_data->key),
 				info.hash_key_size);
+
+		rss_conf.rss_key_len = sizeof(action_rss_data->key);
+		rss_conf.rss_key = action_rss_data->key;
+		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
+		if (ret2 != 0)
+			return ret2;
+		action_rss_data->conf.key = rss_conf.rss_key;
 	}
 	action->conf = &action_rss_data->conf;
 	return ret;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 12:41     ` [dpdk-dev] [PATCH v5] " Lijun Ou
@ 2020-10-15 13:52       ` Ferruh Yigit
  2020-10-15 14:04         ` oulijun
       [not found]       ` <20201015195637.26476-1-robot@bytheb.org>
  1 sibling, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-15 13:52 UTC (permalink / raw)
  To: Lijun Ou, wenzhuo.lu, beilei.xing, adrien.mazarguil
  Cc: dev, linuxarm, Ophir Munk, Ori Kam

On 10/15/2020 1:41 PM, Lijun Ou wrote:
> When start the testpmd, the pmd driver initializes the RSS
> configuration. Generally, the recommended RSS hash key is
> used as the default key in the driver. In addition, the
> default key is different from the default RSS flow in testpmd
> without specifying RSS hash key. So. if you do not specify
> the RSS key when creating an RSS rule, the testpmd uses the
> default key as the default RSS key of the RSS rule. As a result,
> you may mistakenly consider that the RSS key in use is the valid
> default key of the NIC, actually, the key and the valid default
> key of the NIC are two values.
> 
> Consider the follow usage with testpmd:
> 1. first, startup testpmd:
> testpmd> show port 0 rss-hash key
> RSS functions:
>    all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> -6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
> -20C6A42B73BBEAC01FA
> 2. create a rss rule
> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
> types ipv4-udp end queues end / end
> 
> 3. show rss-hash key
> testpmd> show port 0 rss-hash key
> RSS functions:
>    all ipv4-udp udp
> RSS key:
> -74657374706D6427732064656661756C74205253532068617368206B65792C206F
> -76657272696465
> 
> In order to solve the above problems, it use the NIC valid default
> RSS key instead of the testpmd dummy RSS key in the flow configuration
> when the RSS key is not specified in the flow rule. If the NIC RSS key
> is invalid, it will use testpmd dummy RSS key as the default key.
> 
> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> ---
> V4->V5:
> -rewrite the commit log
> -add reviewed-by

Hi Lijun,

There were multiple other comments, it seems they are not addressed but only 
updated the commit log, can you please check comments to prev versions.

Before going into the details, my question was what happens if default key not 
provided at all?
It seems this has been already tried by Ophir [1], later reverted back [2] 
bringing the initial issue back.

According commit, the reason of revert is to support following command:
"flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"

@Ophir, @Lijun,
Can we ignore the 'key_len' if the 'key' is not supported and solve current 
issue as initially intended ([1])?


[1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8

[2] https://git.dpdk.org/dpdk/commit/?id=f3698c3d09a

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 13:52       ` Ferruh Yigit
@ 2020-10-15 14:04         ` oulijun
  2020-10-15 14:43           ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-15 14:04 UTC (permalink / raw)
  To: Ferruh Yigit, wenzhuo.lu, beilei.xing, adrien.mazarguil
  Cc: dev, linuxarm, Ophir Munk, Ori Kam



在 2020/10/15 21:52, Ferruh Yigit 写道:
> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>> When start the testpmd, the pmd driver initializes the RSS
>> configuration. Generally, the recommended RSS hash key is
>> used as the default key in the driver. In addition, the
>> default key is different from the default RSS flow in testpmd
>> without specifying RSS hash key. So. if you do not specify
>> the RSS key when creating an RSS rule, the testpmd uses the
>> default key as the default RSS key of the RSS rule. As a result,
>> you may mistakenly consider that the RSS key in use is the valid
>> default key of the NIC, actually, the key and the valid default
>> key of the NIC are two values.
>>
>> Consider the follow usage with testpmd:
>> 1. first, startup testpmd:
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>    all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>> RSS key:
>> -6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>> -20C6A42B73BBEAC01FA
>> 2. create a rss rule
>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions 
>> rss \
>> types ipv4-udp end queues end / end
>>
>> 3. show rss-hash key
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>    all ipv4-udp udp
>> RSS key:
>> -74657374706D6427732064656661756C74205253532068617368206B65792C206F
>> -76657272696465
>>
>> In order to solve the above problems, it use the NIC valid default
>> RSS key instead of the testpmd dummy RSS key in the flow configuration
>> when the RSS key is not specified in the flow rule. If the NIC RSS key
>> is invalid, it will use testpmd dummy RSS key as the default key.
>>
>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>> ---
>> V4->V5:
>> -rewrite the commit log
>> -add reviewed-by
> 
> Hi Lijun,
> 
> There were multiple other comments, it seems they are not addressed but 
> only updated the commit log, can you please check comments to prev 
> versions.
> 
> Before going into the details, my question was what happens if default 
> key not provided at all?
> It seems this has been already tried by Ophir [1], later reverted back 
> [2] bringing the initial issue back.
> 
> According commit, the reason of revert is to support following command:
> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
> 
> @Ophir, @Lijun,
> Can we ignore the 'key_len' if the 'key' is not supported and solve 
> current issue as initially intended ([1])?
> 
Hi, Ferruh
   I have discussed with Phil Yang about the problem in [1]. I think 
there may be other problems with the idea and there is no better 
solution. and we need to remove key_len definition from rte_rss_conf 
structure. They don't have a plan. And [1] was eventually reverted.

Thanks
Lijun Ou
> 
> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
> 
> [2] https://git.dpdk.org/dpdk/commit/?id=f3698c3d09a
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 14:04         ` oulijun
@ 2020-10-15 14:43           ` Ferruh Yigit
  2020-10-15 16:05             ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-15 14:43 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil
  Cc: dev, linuxarm, Ophir Munk, Ori Kam

On 10/15/2020 3:04 PM, oulijun wrote:
> 
> 
> 在 2020/10/15 21:52, Ferruh Yigit 写道:
>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>> When start the testpmd, the pmd driver initializes the RSS
>>> configuration. Generally, the recommended RSS hash key is
>>> used as the default key in the driver. In addition, the
>>> default key is different from the default RSS flow in testpmd
>>> without specifying RSS hash key. So. if you do not specify
>>> the RSS key when creating an RSS rule, the testpmd uses the
>>> default key as the default RSS key of the RSS rule. As a result,
>>> you may mistakenly consider that the RSS key in use is the valid
>>> default key of the NIC, actually, the key and the valid default
>>> key of the NIC are two values.
>>>
>>> Consider the follow usage with testpmd:
>>> 1. first, startup testpmd:
>>> testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>    all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>> RSS key:
>>> -6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>>> -20C6A42B73BBEAC01FA
>>> 2. create a rss rule
>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
>>> types ipv4-udp end queues end / end
>>>
>>> 3. show rss-hash key
>>> testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>    all ipv4-udp udp
>>> RSS key:
>>> -74657374706D6427732064656661756C74205253532068617368206B65792C206F
>>> -76657272696465
>>>
>>> In order to solve the above problems, it use the NIC valid default
>>> RSS key instead of the testpmd dummy RSS key in the flow configuration
>>> when the RSS key is not specified in the flow rule. If the NIC RSS key
>>> is invalid, it will use testpmd dummy RSS key as the default key.
>>>
>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>> ---
>>> V4->V5:
>>> -rewrite the commit log
>>> -add reviewed-by
>>
>> Hi Lijun,
>>
>> There were multiple other comments, it seems they are not addressed but only 
>> updated the commit log, can you please check comments to prev versions.
>>
>> Before going into the details, my question was what happens if default key not 
>> provided at all?
>> It seems this has been already tried by Ophir [1], later reverted back [2] 
>> bringing the initial issue back.
>>
>> According commit, the reason of revert is to support following command:
>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>
>> @Ophir, @Lijun,
>> Can we ignore the 'key_len' if the 'key' is not supported and solve current 
>> issue as initially intended ([1])?
>>
> Hi, Ferruh
>    I have discussed with Phil Yang about the problem in [1]. I think there may 
> be other problems with the idea and there is no better solution. and we need to 
> remove key_len definition from rte_rss_conf structure. They don't have a plan. 
> And [1] was eventually reverted.
> 

Why ignoring 'key_len' (set it to zero) when there is no 'key' provided doesn't 
work?

> Thanks
> Lijun Ou
>>
>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>>
>> [2] https://git.dpdk.org/dpdk/commit/?id=f3698c3d09a
>> .
>>


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 14:43           ` Ferruh Yigit
@ 2020-10-15 16:05             ` Ferruh Yigit
  2020-10-15 23:21               ` Ophir Munk
  0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-15 16:05 UTC (permalink / raw)
  To: oulijun, wenzhuo.lu, beilei.xing, adrien.mazarguil
  Cc: dev, linuxarm, Ophir Munk, Ori Kam

On 10/15/2020 3:43 PM, Ferruh Yigit wrote:
> On 10/15/2020 3:04 PM, oulijun wrote:
>>
>>
>> 在 2020/10/15 21:52, Ferruh Yigit 写道:
>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>> When start the testpmd, the pmd driver initializes the RSS
>>>> configuration. Generally, the recommended RSS hash key is
>>>> used as the default key in the driver. In addition, the
>>>> default key is different from the default RSS flow in testpmd
>>>> without specifying RSS hash key. So. if you do not specify
>>>> the RSS key when creating an RSS rule, the testpmd uses the
>>>> default key as the default RSS key of the RSS rule. As a result,
>>>> you may mistakenly consider that the RSS key in use is the valid
>>>> default key of the NIC, actually, the key and the valid default
>>>> key of the NIC are two values.
>>>>
>>>> Consider the follow usage with testpmd:
>>>> 1. first, startup testpmd:
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>    all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>> RSS key:
>>>> -6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
>>>> -20C6A42B73BBEAC01FA
>>>> 2. create a rss rule
>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
>>>> types ipv4-udp end queues end / end
>>>>
>>>> 3. show rss-hash key
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>    all ipv4-udp udp
>>>> RSS key:
>>>> -74657374706D6427732064656661756C74205253532068617368206B65792C206F
>>>> -76657272696465
>>>>
>>>> In order to solve the above problems, it use the NIC valid default
>>>> RSS key instead of the testpmd dummy RSS key in the flow configuration
>>>> when the RSS key is not specified in the flow rule. If the NIC RSS key
>>>> is invalid, it will use testpmd dummy RSS key as the default key.
>>>>
>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>> ---
>>>> V4->V5:
>>>> -rewrite the commit log
>>>> -add reviewed-by
>>>
>>> Hi Lijun,
>>>
>>> There were multiple other comments, it seems they are not addressed but only 
>>> updated the commit log, can you please check comments to prev versions.
>>>
>>> Before going into the details, my question was what happens if default key 
>>> not provided at all?
>>> It seems this has been already tried by Ophir [1], later reverted back [2] 
>>> bringing the initial issue back.
>>>
>>> According commit, the reason of revert is to support following command:
>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>
>>> @Ophir, @Lijun,
>>> Can we ignore the 'key_len' if the 'key' is not supported and solve current 
>>> issue as initially intended ([1])?
>>>
>> Hi, Ferruh
>>    I have discussed with Phil Yang about the problem in [1]. I think there may 
>> be other problems with the idea and there is no better solution. and we need 
>> to remove key_len definition from rte_rss_conf structure. They don't have a 
>> plan. And [1] was eventually reverted.
>>
> 
> Why ignoring 'key_len' (set it to zero) when there is no 'key' provided doesn't 
> work?
> 

What do you think [1] + following update, will it work?

  diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
  index ee4f3464fe..e7789c87b3 100644
  --- a/lib/librte_ethdev/rte_flow.c
  +++ b/lib/librte_ethdev/rte_flow.c
  @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t size,
                             }),
                             size > sizeof(*dst.rss) ? sizeof(*dst.rss) : size);
                  off = sizeof(*dst.rss);
  -               if (src.rss->key_len) {
  +               if (src.rss->key_len && src.rss->key) {
                          off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
                          tmp = sizeof(*src.rss->key) * src.rss->key_len;
                          if (size >= off + tmp)

>> Thanks
>> Lijun Ou
>>>
>>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>>>
>>> [2] https://git.dpdk.org/dpdk/commit/?id=f3698c3d09a
>>> .
>>>
> 


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 16:05             ` Ferruh Yigit
@ 2020-10-15 23:21               ` Ophir Munk
  2020-10-15 23:53                 ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: Ophir Munk @ 2020-10-15 23:21 UTC (permalink / raw)
  To: Ferruh Yigit, oulijun, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam

> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
> >>>> When start the testpmd, the pmd driver initializes the RSS
> >>>> configuration. Generally, the recommended RSS hash key is used as
> >>>> the default key in the driver. In addition, the default key is
> >>>> different from the default RSS flow in testpmd without specifying
> >>>> RSS hash key. So. if you do not specify the RSS key when creating
> >>>> an RSS rule, the testpmd uses the default key as the default RSS
> >>>> key of the RSS rule. As a result, you may mistakenly consider that
> >>>> the RSS key in use is the valid default key of the NIC, actually,
> >>>> the key and the valid default key of the NIC are two values.
> >>>>
> >>>> Consider the follow usage with testpmd:
> >>>> 1. first, startup testpmd:
> >>>> testpmd> show port 0 rss-hash key
> >>>> RSS functions:
> >>>>    all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
> >>>> -
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
> 0F
> >>>> -20C6A42B73BBEAC01FA
> >>>> 2. create a rss rule
> >>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
> >>>> testpmd> actions rss \
> >>>> types ipv4-udp end queues end / end
> >>>>
> >>>> 3. show rss-hash key
> >>>> testpmd> show port 0 rss-hash key
> >>>> RSS functions:
> >>>>    all ipv4-udp udp
> >>>> RSS key:
> >>>> -
> 74657374706D6427732064656661756C74205253532068617368206B65792C
> 206F
> >>>> -76657272696465
> >>>>
> >>>> In order to solve the above problems, it use the NIC valid default
> >>>> RSS key instead of the testpmd dummy RSS key in the flow
> >>>> configuration when the RSS key is not specified in the flow rule.
> >>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the
> default key.
> >>>>
> >>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
> >>>> API")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >>>> ---
> >>>> V4->V5:
> >>>> -rewrite the commit log
> >>>> -add reviewed-by
> >>>
> >>> Hi Lijun,
> >>>
> >>> There were multiple other comments, it seems they are not addressed
> >>> but only updated the commit log, can you please check comments to
> prev versions.
> >>>
> >>> Before going into the details, my question was what happens if
> >>> default key not provided at all?
> >>> It seems this has been already tried by Ophir [1], later reverted
> >>> back [2] bringing the initial issue back.
> >>>
> >>> According commit, the reason of revert is to support following
> command:
> >>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
> >>>
> >>> @Ophir, @Lijun,
> >>> Can we ignore the 'key_len' if the 'key' is not supported and solve
> >>> current issue as initially intended ([1])?
> >>>
> >> Hi, Ferruh
> >>    I have discussed with Phil Yang about the problem in [1]. I think
> >> there may be other problems with the idea and there is no better
> >> solution. and we need to remove key_len definition from rte_rss_conf
> >> structure. They don't have a plan. And [1] was eventually reverted.
> >>
> >
> > Why ignoring 'key_len' (set it to zero) when there is no 'key'
> > provided doesn't work?
> >
> 
> What do you think [1] + following update, will it work?
> 
>   diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
>   index ee4f3464fe..e7789c87b3 100644
>   --- a/lib/librte_ethdev/rte_flow.c
>   +++ b/lib/librte_ethdev/rte_flow.c
>   @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
> size,
>                              }),
>                              size > sizeof(*dst.rss) ? sizeof(*dst.rss) : size);
>                   off = sizeof(*dst.rss);
>   -               if (src.rss->key_len) {
>   +               if (src.rss->key_len && src.rss->key) {
>                           off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
>                           tmp = sizeof(*src.rss->key) * src.rss->key_len;
>                           if (size >= off + tmp)
> 

Ferruh, your suggestion ([1] + update) looks correct. I also verified it on mlx5 PMD.
Advantage: it's a generic fix for all dpdk applications using rte_flows (not just testpmd). 
It reduces code.
With this fix the responsibility of handling key==NULL and/or len==0 is moved to the PMDs (which is good).
 
With regard to Lijun patch - I liked the approach of overriding the default testpmd key with the default PMD key.
But it only addresses testpmd. More code was added. 
It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing RSS, but it would feel more confident if we could confirm it for all the PMDs (by testing) or at least review the PMDs rss_hash_conf_get() implementations.

> >> Thanks
> >> Lijun Ou
> >>>

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 23:21               ` Ophir Munk
@ 2020-10-15 23:53                 ` Ferruh Yigit
  2020-10-16  0:14                   ` Ajit Khaparde
                                     ` (2 more replies)
  0 siblings, 3 replies; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-15 23:53 UTC (permalink / raw)
  To: Ophir Munk, oulijun, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam

On 10/16/2020 12:21 AM, Ophir Munk wrote:
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>> the default key in the driver. In addition, the default key is
>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>
>>>>>> Consider the follow usage with testpmd:
>>>>>> 1. first, startup testpmd:
>>>>>> testpmd> show port 0 rss-hash key
>>>>>> RSS functions:
>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>> -
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>> 0F
>>>>>> -20C6A42B73BBEAC01FA
>>>>>> 2. create a rss rule
>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>> testpmd> actions rss \
>>>>>> types ipv4-udp end queues end / end
>>>>>>
>>>>>> 3. show rss-hash key
>>>>>> testpmd> show port 0 rss-hash key
>>>>>> RSS functions:
>>>>>>     all ipv4-udp udp
>>>>>> RSS key:
>>>>>> -
>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>> 206F
>>>>>> -76657272696465
>>>>>>
>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the
>> default key.
>>>>>>
>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>> API")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>> ---
>>>>>> V4->V5:
>>>>>> -rewrite the commit log
>>>>>> -add reviewed-by
>>>>>
>>>>> Hi Lijun,
>>>>>
>>>>> There were multiple other comments, it seems they are not addressed
>>>>> but only updated the commit log, can you please check comments to
>> prev versions.
>>>>>
>>>>> Before going into the details, my question was what happens if
>>>>> default key not provided at all?
>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>> back [2] bringing the initial issue back.
>>>>>
>>>>> According commit, the reason of revert is to support following
>> command:
>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>
>>>>> @Ophir, @Lijun,
>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>> current issue as initially intended ([1])?
>>>>>
>>>> Hi, Ferruh
>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>> there may be other problems with the idea and there is no better
>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>
>>>
>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>> provided doesn't work?
>>>
>>
>> What do you think [1] + following update, will it work?
>>
>>    diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
>>    index ee4f3464fe..e7789c87b3 100644
>>    --- a/lib/librte_ethdev/rte_flow.c
>>    +++ b/lib/librte_ethdev/rte_flow.c
>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>> size,
>>                               }),
>>                               size > sizeof(*dst.rss) ? sizeof(*dst.rss) : size);
>>                    off = sizeof(*dst.rss);
>>    -               if (src.rss->key_len) {
>>    +               if (src.rss->key_len && src.rss->key) {
>>                            off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
>>                            tmp = sizeof(*src.rss->key) * src.rss->key_len;
>>                            if (size >= off + tmp)
>>
> 
> Ferruh, your suggestion ([1] + update) looks correct. I also verified it on mlx5 PMD.
> Advantage: it's a generic fix for all dpdk applications using rte_flows (not just testpmd).
> It reduces code.
> With this fix the responsibility of handling key==NULL and/or len==0 is moved to the PMDs (which is good).
>   
> With regard to Lijun patch - I liked the approach of overriding the default testpmd key with the default PMD key.
> But it only addresses testpmd. More code was added.
> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing RSS, but it would feel more confident if we could confirm it for all the PMDs (by testing) or at least review the PMDs rss_hash_conf_get() implementations.
> 

Lijun's idea can work. There was a problem in implementation related to the key 
size assumption, which can be fixed.

Even it is fixed, when user gives a rss rule without a key, we are getting key 
from device and feeding same key back to device, this is unnecessary I think.
When user didn't provide a key, rss rule shouldn't touch the key at all.

Complication was when user provides key_len without a key, I think both ignoring 
or returning error in this case is OK.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 23:53                 ` Ferruh Yigit
@ 2020-10-16  0:14                   ` Ajit Khaparde
  2020-10-16  6:46                   ` Ophir Munk
  2020-10-16 10:04                   ` oulijun
  2 siblings, 0 replies; 44+ messages in thread
From: Ajit Khaparde @ 2020-10-16  0:14 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Ophir Munk, oulijun, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil, dev, linuxarm, Ori Kam

On Thu, Oct 15, 2020 at 4:54 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 10/16/2020 12:21 AM, Ophir Munk wrote:
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
> >>>>>> When start the testpmd, the pmd driver initializes the RSS
> >>>>>> configuration. Generally, the recommended RSS hash key is used as
> >>>>>> the default key in the driver. In addition, the default key is
> >>>>>> different from the default RSS flow in testpmd without specifying
> >>>>>> RSS hash key. So. if you do not specify the RSS key when creating
> >>>>>> an RSS rule, the testpmd uses the default key as the default RSS
> >>>>>> key of the RSS rule. As a result, you may mistakenly consider that
> >>>>>> the RSS key in use is the valid default key of the NIC, actually,
> >>>>>> the key and the valid default key of the NIC are two values.
> >>>>>>
> >>>>>> Consider the follow usage with testpmd:
> >>>>>> 1. first, startup testpmd:
> >>>>>> testpmd> show port 0 rss-hash key
> >>>>>> RSS functions:
> >>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
> >>>>>> -
> >> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
> >> 0F
> >>>>>> -20C6A42B73BBEAC01FA
> >>>>>> 2. create a rss rule
> >>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
> >>>>>> testpmd> actions rss \
> >>>>>> types ipv4-udp end queues end / end
> >>>>>>
> >>>>>> 3. show rss-hash key
> >>>>>> testpmd> show port 0 rss-hash key
> >>>>>> RSS functions:
> >>>>>>     all ipv4-udp udp
> >>>>>> RSS key:
> >>>>>> -
> >> 74657374706D6427732064656661756C74205253532068617368206B65792C
> >> 206F
> >>>>>> -76657272696465
> >>>>>>
> >>>>>> In order to solve the above problems, it use the NIC valid default
> >>>>>> RSS key instead of the testpmd dummy RSS key in the flow
> >>>>>> configuration when the RSS key is not specified in the flow rule.
> >>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the
> >> default key.
> >>>>>>
> >>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
> >>>>>> API")
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
> >>>>>> ---
> >>>>>> V4->V5:
> >>>>>> -rewrite the commit log
> >>>>>> -add reviewed-by
> >>>>>
> >>>>> Hi Lijun,
> >>>>>
> >>>>> There were multiple other comments, it seems they are not addressed
> >>>>> but only updated the commit log, can you please check comments to
> >> prev versions.
> >>>>>
> >>>>> Before going into the details, my question was what happens if
> >>>>> default key not provided at all?
> >>>>> It seems this has been already tried by Ophir [1], later reverted
> >>>>> back [2] bringing the initial issue back.
> >>>>>
> >>>>> According commit, the reason of revert is to support following
> >> command:
> >>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
> >>>>>
> >>>>> @Ophir, @Lijun,
> >>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
> >>>>> current issue as initially intended ([1])?
> >>>>>
> >>>> Hi, Ferruh
> >>>>     I have discussed with Phil Yang about the problem in [1]. I think
> >>>> there may be other problems with the idea and there is no better
> >>>> solution. and we need to remove key_len definition from rte_rss_conf
> >>>> structure. They don't have a plan. And [1] was eventually reverted.
> >>>>
> >>>
> >>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
> >>> provided doesn't work?
> >>>
> >>
> >> What do you think [1] + following update, will it work?
> >>
> >>    diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> >>    index ee4f3464fe..e7789c87b3 100644
> >>    --- a/lib/librte_ethdev/rte_flow.c
> >>    +++ b/lib/librte_ethdev/rte_flow.c
> >>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
> >> size,
> >>                               }),
> >>                               size > sizeof(*dst.rss) ? sizeof(*dst.rss) : size);
> >>                    off = sizeof(*dst.rss);
> >>    -               if (src.rss->key_len) {
> >>    +               if (src.rss->key_len && src.rss->key) {
> >>                            off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
> >>                            tmp = sizeof(*src.rss->key) * src.rss->key_len;
> >>                            if (size >= off + tmp)
> >>
> >
> > Ferruh, your suggestion ([1] + update) looks correct. I also verified it on mlx5 PMD.
> > Advantage: it's a generic fix for all dpdk applications using rte_flows (not just testpmd).
> > It reduces code.
> > With this fix the responsibility of handling key==NULL and/or len==0 is moved to the PMDs (which is good).
> >
> > With regard to Lijun patch - I liked the approach of overriding the default testpmd key with the default PMD key.
> > But it only addresses testpmd. More code was added.
> > It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing RSS, but it would feel more confident if we could confirm it for all the PMDs (by testing) or at least review the PMDs rss_hash_conf_get() implementations.
> >
>
> Lijun's idea can work. There was a problem in implementation related to the key
> size assumption, which can be fixed.
>
> Even it is fixed, when user gives a rss rule without a key, we are getting key
> from device and feeding same key back to device, this is unnecessary I think.
> When user didn't provide a key, rss rule shouldn't touch the key at all.
+1
But can we add this as expected behavior in the rte_flow document.

>
> Complication was when user provides key_len without a key, I think both ignoring
> or returning error in this case is OK.
Let's return an error if key_len is provided without a key.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 23:53                 ` Ferruh Yigit
  2020-10-16  0:14                   ` Ajit Khaparde
@ 2020-10-16  6:46                   ` Ophir Munk
  2020-10-16 10:05                     ` oulijun
  2020-10-16 10:04                   ` oulijun
  2 siblings, 1 reply; 44+ messages in thread
From: Ophir Munk @ 2020-10-16  6:46 UTC (permalink / raw)
  To: Ferruh Yigit, oulijun, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
<..>
> >>
> >
> > Ferruh, your suggestion ([1] + update) looks correct. I also verified it on
> mlx5 PMD.
> > Advantage: it's a generic fix for all dpdk applications using rte_flows (not
> just testpmd).
> > It reduces code.
> > With this fix the responsibility of handling key==NULL and/or len==0 is
> moved to the PMDs (which is good).
> >
> > With regard to Lijun patch - I liked the approach of overriding the default
> testpmd key with the default PMD key.
> > But it only addresses testpmd. More code was added.
> > It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing
> RSS, but it would feel more confident if we could confirm it for all the PMDs
> (by testing) or at least review the PMDs rss_hash_conf_get()
> implementations.
> >
> 
> Lijun's idea can work. There was a problem in implementation related to the
> key size assumption, which can be fixed.
> 
> Even it is fixed, when user gives a rss rule without a key, we are getting key
> from device and feeding same key back to device, this is unnecessary I think.

I agree.

> When user didn't provide a key, rss rule shouldn't touch the key at all.

Agreed as well.

> Complication was when user provides key_len without a key, I think both
> ignoring or returning error in this case is OK.

I think that in general flow rules should arrive "as is" to the PMD which has its
validation and translation APIs to handle all cases.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] |WARNING| pw80899 app/testpmd: fix the default RSS key configuration
       [not found]       ` <20201015195637.26476-1-robot@bytheb.org>
@ 2020-10-16  9:39         ` " oulijun
  2020-10-16  9:41           ` David Marchand
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-16  9:39 UTC (permalink / raw)
  To: 0-day Robot, test-report, dev

Hi, all
   Can the DPDK travis-CI check be configured locally? If yes, please 
provide guidance on the configuration check method.

Thanks
Lijun Ou

ÔÚ 2020/10/16 3:56, 0-day Robot дµÀ:
> From: robot@bytheb.org
> 
> Test-Label: travis-robot
> Test-Status: WARNING
> http://dpdk.org/patch/80899
> 
> _Travis build: failed_
> Build URL: https://travis-ci.com/ovsrobot/dpdk/builds/190253613
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] |WARNING| pw80899 app/testpmd: fix the default RSS key configuration
  2020-10-16  9:39         ` [dpdk-dev] |WARNING| pw80899 " oulijun
@ 2020-10-16  9:41           ` David Marchand
  0 siblings, 0 replies; 44+ messages in thread
From: David Marchand @ 2020-10-16  9:41 UTC (permalink / raw)
  To: oulijun; +Cc: test-report, dev

On Fri, Oct 16, 2020 at 11:40 AM oulijun <oulijun@huawei.com> wrote:
>
> Hi, all
>    Can the DPDK travis-CI check be configured locally? If yes, please
> provide guidance on the configuration check method.
>
> Thanks
> Lijun Ou
>
> 在 2020/10/16 3:56, 0-day Robot 写道:
> > From: robot@bytheb.org
> >
> > Test-Label: travis-robot
> > Test-Status: WARNING
> > http://dpdk.org/patch/80899
> >
> > _Travis build: failed_
> > Build URL: https://travis-ci.com/ovsrobot/dpdk/builds/190253613

The issue here should be fixed with
https://patchwork.dpdk.org/patch/81068/ which I am looking at.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-15 23:53                 ` Ferruh Yigit
  2020-10-16  0:14                   ` Ajit Khaparde
  2020-10-16  6:46                   ` Ophir Munk
@ 2020-10-16 10:04                   ` oulijun
  2020-10-16 10:57                     ` Ferruh Yigit
  2 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-16 10:04 UTC (permalink / raw)
  To: Ferruh Yigit, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



在 2020/10/16 7:53, Ferruh Yigit 写道:
> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>
>>>>>>> Consider the follow usage with testpmd:
>>>>>>> 1. first, startup testpmd:
>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>> RSS functions:
>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>> -
>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>> 0F
>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>> 2. create a rss rule
>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>> testpmd> actions rss \
>>>>>>> types ipv4-udp end queues end / end
>>>>>>>
>>>>>>> 3. show rss-hash key
>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>> RSS functions:
>>>>>>>     all ipv4-udp udp
>>>>>>> RSS key:
>>>>>>> -
>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>> 206F
>>>>>>> -76657272696465
>>>>>>>
>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key 
>>>>>>> as the
>>> default key.
>>>>>>>
>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>> API")
>>>>>>> Cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>> ---
>>>>>>> V4->V5:
>>>>>>> -rewrite the commit log
>>>>>>> -add reviewed-by
>>>>>>
>>>>>> Hi Lijun,
>>>>>>
>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>> but only updated the commit log, can you please check comments to
>>> prev versions.
>>>>>>
>>>>>> Before going into the details, my question was what happens if
>>>>>> default key not provided at all?
>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>> back [2] bringing the initial issue back.
>>>>>>
>>>>>> According commit, the reason of revert is to support following
>>> command:
>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>>
>>>>>> @Ophir, @Lijun,
>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>> current issue as initially intended ([1])?
>>>>>>
>>>>> Hi, Ferruh
>>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>>> there may be other problems with the idea and there is no better
>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>
>>>>
>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>> provided doesn't work?
>>>>
>>>
>>> What do you think [1] + following update, will it work?
>>>
>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>> b/lib/librte_ethdev/rte_flow.c
>>>    index ee4f3464fe..e7789c87b3 100644
>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>>> size,
>>>                               }),
>>>                               size > sizeof(*dst.rss) ? 
>>> sizeof(*dst.rss) : size);
>>>                    off = sizeof(*dst.rss);
>>>    -               if (src.rss->key_len) {
>>>    +               if (src.rss->key_len && src.rss->key) {
>>>                            off = RTE_ALIGN_CEIL(off, 
>>> sizeof(*dst.rss->key));
>>>                            tmp = sizeof(*src.rss->key) * 
>>> src.rss->key_len;
>>>                            if (size >= off + tmp)
>>>
>>
>> Ferruh, your suggestion ([1] + update) looks correct. I also verified 
>> it on mlx5 PMD.
>> Advantage: it's a generic fix for all dpdk applications using 
>> rte_flows (not just testpmd).
>> It reduces code.
>> With this fix the responsibility of handling key==NULL and/or len==0 
>> is moved to the PMDs (which is good).
>> With regard to Lijun patch - I liked the approach of overriding the 
>> default testpmd key with the default PMD key.
>> But it only addresses testpmd. More code was added.
>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing 
>> RSS, but it would feel more confident if we could confirm it for all 
>> the PMDs (by testing) or at least review the PMDs rss_hash_conf_get() 
>> implementations.
>>
> 
> Lijun's idea can work. There was a problem in implementation related to 
> the key size assumption, which can be fixed.
> 
> Even it is fixed, when user gives a rss rule without a key, we are 
> getting key from device and feeding same key back to device, this is 
> unnecessary I think.
> When user didn't provide a key, rss rule shouldn't touch the key at all.
> 
Do you mean that the driver should not configure the key to the hardware 
when the RSS key is not specified?
If so, we cannot identify when the user does not specify the RSS key to 
determine whether to configure the key.
In hardware design, most vendors do not configure keys for hardware 
independently, which may be associated with other RSS config parameters.
I think it would be reasonable for us to reconfigure the RSS key with 
the default value configured and valid in the hardware as the default 
value if the user does not specify the RSS key.
> Complication was when user provides key_len without a key, I think both 
> ignoring or returning error in this case is OK.
I agree. However, I think it is meaningless to expose the RSS key length 
to users. Do you consider deleting the RSS key length directly?
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-16  6:46                   ` Ophir Munk
@ 2020-10-16 10:05                     ` oulijun
  2020-10-16 15:13                       ` Ophir Munk
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-16 10:05 UTC (permalink / raw)
  To: Ophir Munk, Ferruh Yigit, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



在 2020/10/16 14:46, Ophir Munk 写道:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> <..>
>>>>
>>>
>>> Ferruh, your suggestion ([1] + update) looks correct. I also verified it on
>> mlx5 PMD.
>>> Advantage: it's a generic fix for all dpdk applications using rte_flows (not
>> just testpmd).
>>> It reduces code.
>>> With this fix the responsibility of handling key==NULL and/or len==0 is
>> moved to the PMDs (which is good).
>>>
>>> With regard to Lijun patch - I liked the approach of overriding the default
>> testpmd key with the default PMD key.
>>> But it only addresses testpmd. More code was added.
>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing
>> RSS, but it would feel more confident if we could confirm it for all the PMDs
>> (by testing) or at least review the PMDs rss_hash_conf_get()
>> implementations.
>>>
>>
>> Lijun's idea can work. There was a problem in implementation related to the
>> key size assumption, which can be fixed.
>>
>> Even it is fixed, when user gives a rss rule without a key, we are getting key
>> from device and feeding same key back to device, this is unnecessary I think.
> 
> I agree.
> 
>> When user didn't provide a key, rss rule shouldn't touch the key at all.
> 
> Agreed as well.
> 
>> Complication was when user provides key_len without a key, I think both
>> ignoring or returning error in this case is OK.
> 
> I think that in general flow rules should arrive "as is" to the PMD which has its
> validation and translation APIs to handle all cases.
> 
Do you agree with [1] + Ferruh update solution?

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-16 10:04                   ` oulijun
@ 2020-10-16 10:57                     ` Ferruh Yigit
  2020-10-16 14:59                       ` Ophir Munk
  2020-10-20  9:00                       ` oulijun
  0 siblings, 2 replies; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-16 10:57 UTC (permalink / raw)
  To: oulijun, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam

On 10/16/2020 11:04 AM, oulijun wrote:
> 
> 
> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>
>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>> 1. first, startup testpmd:
>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>> RSS functions:
>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>> -
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>> 0F
>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>> 2. create a rss rule
>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>> testpmd> actions rss \
>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>
>>>>>>>> 3. show rss-hash key
>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>> RSS functions:
>>>>>>>>     all ipv4-udp udp
>>>>>>>> RSS key:
>>>>>>>> -
>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>> 206F
>>>>>>>> -76657272696465
>>>>>>>>
>>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the
>>>> default key.
>>>>>>>>
>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>>> API")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>> ---
>>>>>>>> V4->V5:
>>>>>>>> -rewrite the commit log
>>>>>>>> -add reviewed-by
>>>>>>>
>>>>>>> Hi Lijun,
>>>>>>>
>>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>>> but only updated the commit log, can you please check comments to
>>>> prev versions.
>>>>>>>
>>>>>>> Before going into the details, my question was what happens if
>>>>>>> default key not provided at all?
>>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>>> back [2] bringing the initial issue back.
>>>>>>>
>>>>>>> According commit, the reason of revert is to support following
>>>> command:
>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>>>
>>>>>>> @Ophir, @Lijun,
>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>>> current issue as initially intended ([1])?
>>>>>>>
>>>>>> Hi, Ferruh
>>>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>>>> there may be other problems with the idea and there is no better
>>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>>
>>>>>
>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>> provided doesn't work?
>>>>>
>>>>
>>>> What do you think [1] + following update, will it work?
>>>>
>>>>    diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>>>> size,
>>>>                               }),
>>>>                               size > sizeof(*dst.rss) ? sizeof(*dst.rss) : 
>>>> size);
>>>>                    off = sizeof(*dst.rss);
>>>>    -               if (src.rss->key_len) {
>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>                            off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
>>>>                            tmp = sizeof(*src.rss->key) * src.rss->key_len;
>>>>                            if (size >= off + tmp)
>>>>
>>>
>>> Ferruh, your suggestion ([1] + update) looks correct. I also verified it on 
>>> mlx5 PMD.
>>> Advantage: it's a generic fix for all dpdk applications using rte_flows (not 
>>> just testpmd).
>>> It reduces code.
>>> With this fix the responsibility of handling key==NULL and/or len==0 is moved 
>>> to the PMDs (which is good).
>>> With regard to Lijun patch - I liked the approach of overriding the default 
>>> testpmd key with the default PMD key.
>>> But it only addresses testpmd. More code was added.
>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing RSS, 
>>> but it would feel more confident if we could confirm it for all the PMDs (by 
>>> testing) or at least review the PMDs rss_hash_conf_get() implementations.
>>>
>>
>> Lijun's idea can work. There was a problem in implementation related to the 
>> key size assumption, which can be fixed.
>>
>> Even it is fixed, when user gives a rss rule without a key, we are getting key 
>> from device and feeding same key back to device, this is unnecessary I think.
>> When user didn't provide a key, rss rule shouldn't touch the key at all.
>>
> Do you mean that the driver should not configure the key to the hardware when 
> the RSS key is not specified?

We are talking about 'key' in RSS rte flow rule, not generally configuring the 
device for RSS.

If a specific rss rte flow rule, lets say to filter some defined packets to some 
specific queues, don't have 'key' as a part of rule, I am suggesting not touch 
'key' configuration of the device.

> If so, we cannot identify when the user does not specify the RSS key to 
> determine whether to configure the key.

I think we can. If the rule has 'key' attribute, driver can use that key to 
program the device, if 'key' attribute is missing, don't do anything related to 
the rss key.

> In hardware design, most vendors do not configure keys for hardware 
> independently, which may be associated with other RSS config parameters.
> I think it would be reasonable for us to reconfigure the RSS key with the 
> default value configured and valid in the hardware as the default value if the 
> user does not specify the RSS key.

There are already APIs to update the RSS configuration, like RSS key, hash 
functions etc.. They are independent from the rte flow RSS rule.
Application should configure RSS according needs using those APIs.

The question here is about each RSS rte flow rule that can update the key. Am I 
missing something?

>> Complication was when user provides key_len without a key, I think both 
>> ignoring or returning error in this case is OK.
> I agree. However, I think it is meaningless to expose the RSS key length to 
> users. Do you consider deleting the RSS key length directly?

Isn't both 'key' and 'key_len' needed to program the RSS key? Can the driver 
know the size of the 'key' without 'key_len'?
And driver should verify these provided values.


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-16 10:57                     ` Ferruh Yigit
@ 2020-10-16 14:59                       ` Ophir Munk
  2020-10-20  9:00                       ` oulijun
  1 sibling, 0 replies; 44+ messages in thread
From: Ophir Munk @ 2020-10-16 14:59 UTC (permalink / raw)
  To: Ferruh Yigit, oulijun, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> Sent: Friday, October 16, 2020 1:57 PM
> >> Lijun's idea can work. There was a problem in implementation related
> >> to the key size assumption, which can be fixed.
> >>
> >> Even it is fixed, when user gives a rss rule without a key, we are
> >> getting key from device and feeding same key back to device, this is
> unnecessary I think.
> >> When user didn't provide a key, rss rule shouldn't touch the key at all.
> >>
> > Do you mean that the driver should not configure the key to the
> > hardware when the RSS key is not specified?
> 

I would consider 2 user types:
1. A user who knows the HW capabilities and wish to get a distribution by specifying an explicit key.
2. A clueless user that is not familiar with the NIC HW. For example, a switching application which runs on any NIC. In that case the switching application will not configure a key. The HW driver can decide to configure the default key if not configured.
On the rte flow level we should communicate to the PMD that no key was specified.
Inside the PMD any decision can be taken (e.g. configuring the default key).
All of this is supported by PMD call backs to rte calls such as flow_validate(), flow_translate().

> We are talking about 'key' in RSS rte flow rule, not generally configuring the
> device for RSS.
> 
> If a specific rss rte flow rule, lets say to filter some defined packets to some
> specific queues, don't have 'key' as a part of rule, I am suggesting not touch
> 'key' configuration of the device.
> 
> > If so, we cannot identify when the user does not specify the RSS key
> > to determine whether to configure the key.
> 
> I think we can. If the rule has 'key' attribute, driver can use that key to
> program the device, if 'key' attribute is missing, don't do anything related to
> the rss key.

The driver is free to take any decision when a key is missing.
 
> > In hardware design, most vendors do not configure keys for hardware
> > independently, which may be associated with other RSS config parameters.
> > I think it would be reasonable for us to reconfigure the RSS key with
> > the default value configured and valid in the hardware as the default
> > value if the user does not specify the RSS key.
> 
> There are already APIs to update the RSS configuration, like RSS key, hash
> functions etc.. They are independent from the rte flow RSS rule.
> Application should configure RSS according needs using those APIs.
> 
> The question here is about each RSS rte flow rule that can update the key.
> Am I missing something?
> 
> >> Complication was when user provides key_len without a key, I think
> >> both ignoring or returning error in this case is OK.
> > I agree. However, I think it is meaningless to expose the RSS key
> > length to users. Do you consider deleting the RSS key length directly?
> 
> Isn't both 'key' and 'key_len' needed to program the RSS key? Can the driver
> know the size of the 'key' without 'key_len'?
> And driver should verify these provided values.

The key_len must be explicit. Key itself is just a pointer to an array of bytes. 


^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-16 10:05                     ` oulijun
@ 2020-10-16 15:13                       ` Ophir Munk
  0 siblings, 0 replies; 44+ messages in thread
From: Ophir Munk @ 2020-10-16 15:13 UTC (permalink / raw)
  To: oulijun, Ferruh Yigit, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam


> -----Original Message-----
> From: oulijun <oulijun@huawei.com>
> Sent: Friday, October 16, 2020 1:06 PM
ndle all cases.
> >
> Do you agree with [1] + Ferruh update solution?

Yes.
In that case your PMD may get a NULL RSS key. Then you can override it with your default key (if HW is not already configured with the default).

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-16 10:57                     ` Ferruh Yigit
  2020-10-16 14:59                       ` Ophir Munk
@ 2020-10-20  9:00                       ` oulijun
  2020-10-20 10:02                         ` Ferruh Yigit
  1 sibling, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-20  9:00 UTC (permalink / raw)
  To: Ferruh Yigit, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



在 2020/10/16 18:57, Ferruh Yigit 写道:
> On 10/16/2020 11:04 AM, oulijun wrote:
>>
>>
>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>
>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>> RSS functions:
>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>>> -
>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>> 0F
>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>> 2. create a rss rule
>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>>> testpmd> actions rss \
>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>
>>>>>>>>> 3. show rss-hash key
>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>> RSS functions:
>>>>>>>>>     all ipv4-udp udp
>>>>>>>>> RSS key:
>>>>>>>>> -
>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>> 206F
>>>>>>>>> -76657272696465
>>>>>>>>>
>>>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS 
>>>>>>>>> key as the
>>>>> default key.
>>>>>>>>>
>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>>>> API")
>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>
>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>> ---
>>>>>>>>> V4->V5:
>>>>>>>>> -rewrite the commit log
>>>>>>>>> -add reviewed-by
>>>>>>>>
>>>>>>>> Hi Lijun,
>>>>>>>>
>>>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>>>> but only updated the commit log, can you please check comments to
>>>>> prev versions.
>>>>>>>>
>>>>>>>> Before going into the details, my question was what happens if
>>>>>>>> default key not provided at all?
>>>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>
>>>>>>>> According commit, the reason of revert is to support following
>>>>> command:
>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / 
>>>>>>>> end"
>>>>>>>>
>>>>>>>> @Ophir, @Lijun,
>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>
>>>>>>> Hi, Ferruh
>>>>>>>     I have discussed with Phil Yang about the problem in [1]. I 
>>>>>>> think
>>>>>>> there may be other problems with the idea and there is no better
>>>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>>>
>>>>>>
>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>> provided doesn't work?
>>>>>>
>>>>>
>>>>> What do you think [1] + following update, will it work?
>>>>>
>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>>>> b/lib/librte_ethdev/rte_flow.c
>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const 
>>>>> size_t
>>>>> size,
>>>>>                               }),
>>>>>                               size > sizeof(*dst.rss) ? 
>>>>> sizeof(*dst.rss) : size);
>>>>>                    off = sizeof(*dst.rss);
>>>>>    -               if (src.rss->key_len) {
>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>                            off = RTE_ALIGN_CEIL(off, 
>>>>> sizeof(*dst.rss->key));
>>>>>                            tmp = sizeof(*src.rss->key) * 
>>>>> src.rss->key_len;
>>>>>                            if (size >= off + tmp)
>>>>>
>>>>
>>>> Ferruh, your suggestion ([1] + update) looks correct. I also 
>>>> verified it on mlx5 PMD.
>>>> Advantage: it's a generic fix for all dpdk applications using 
>>>> rte_flows (not just testpmd).
>>>> It reduces code.
>>>> With this fix the responsibility of handling key==NULL and/or len==0 
>>>> is moved to the PMDs (which is good).
>>>> With regard to Lijun patch - I liked the approach of overriding the 
>>>> default testpmd key with the default PMD key.
>>>> But it only addresses testpmd. More code was added.
>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of 
>>>> parsing RSS, but it would feel more confident if we could confirm it 
>>>> for all the PMDs (by testing) or at least review the PMDs 
>>>> rss_hash_conf_get() implementations.
>>>>
>>>
>>> Lijun's idea can work. There was a problem in implementation related 
>>> to the key size assumption, which can be fixed.
>>>
>>> Even it is fixed, when user gives a rss rule without a key, we are 
>>> getting key from device and feeding same key back to device, this is 
>>> unnecessary I think.
>>> When user didn't provide a key, rss rule shouldn't touch the key at all.
>>>
>> Do you mean that the driver should not configure the key to the 
>> hardware when the RSS key is not specified?
> 
> We are talking about 'key' in RSS rte flow rule, not generally 
> configuring the device for RSS.
> 
Yes, I know. However, I am talking about some implementations. The 
configuration interface may be the same as that for configuring RSS
parameters of the device in general mode.
> If a specific rss rte flow rule, lets say to filter some defined packets 
> to some specific queues, don't have 'key' as a part of rule, I am 
> suggesting not touch 'key' configuration of the device.
> 
Yes, I agree. I think your point of view is very reasonable and a more
appropriate implementation. However, for the sake of simplicity and
other considerations in the architecture design of some devices, the
hardware may support reasonable hybrid configuration for paramter 
configuration in the RSS config, rather than independent configuration
for hw, that is, maybe touch A of rss configuration of the device and
must touch B of rss configuration. As a result, If the preceding 
scenario occurs, it is reasonable to configure an RSS config that does 
not change and specified.What do you think?
>> If so, we cannot identify when the user does not specify the RSS key 
>> to determine whether to configure the key.
> 
> I think we can. If the rule has 'key' attribute, driver can use that key 
> to program the device, if 'key' attribute is missing, don't do anything 
> related to the rss key.
> 
Yes, if 'key' attribute is missing, don't do anything related to the rss 
key. It's more reasonable.
But, If he can't do that, I think it is reasonable to configure the 
default key that already exists on the device. The only disadvantage is 
wasteful.
>> In hardware design, most vendors do not configure keys for hardware 
>> independently, which may be associated with other RSS config parameters.
>> I think it would be reasonable for us to reconfigure the RSS key with 
>> the default value configured and valid in the hardware as the default 
>> value if the user does not specify the RSS key.
> 
> There are already APIs to update the RSS configuration, like RSS key, 
> hash functions etc.. They are independent from the rte flow RSS rule.
> Application should configure RSS according needs using those APIs.
> 
Yes, should do this. Are there any unreasonable users who simply do not 
know these APIs or do not want to use them and want to configure some 
parameters through the rte flow RSS rule without changing other parameters?
> The question here is about each RSS rte flow rule that can update the 
> key. Am I missing something?
I don't think you've misunderstood the general situation.
Even if the RSS key is not specified in a rule, the driver uses the 
default key value to re-touch the device.
> 
>>> Complication was when user provides key_len without a key, I think 
>>> both ignoring or returning error in this case is OK.
>> I agree. However, I think it is meaningless to expose the RSS key 
>> length to users. Do you consider deleting the RSS key length directly?
> 
> Isn't both 'key' and 'key_len' needed to program the RSS key? Can the 
> driver know the size of the 'key' without 'key_len'?
> And driver should verify these provided values.
> 
I know and agree.It is only during the analysis of the [1] scheme, from 
the time it was proposed to the time it was withdrawn, that I found some 
guys in the community questioned the significance of key_len.

Back to the subject, what is your plan for the solution in the patch?
> .
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-20  9:00                       ` oulijun
@ 2020-10-20 10:02                         ` Ferruh Yigit
  2020-10-20 13:35                           ` oulijun
  0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-20 10:02 UTC (permalink / raw)
  To: oulijun, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam

On 10/20/2020 10:00 AM, oulijun wrote:
> 
> 
> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>
>>>
>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>>
>>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>> RSS functions:
>>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>>>> -
>>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>>> 0F
>>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>>> 2. create a rss rule
>>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>>>> testpmd> actions rss \
>>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>>
>>>>>>>>>> 3. show rss-hash key
>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>> RSS functions:
>>>>>>>>>>     all ipv4-udp udp
>>>>>>>>>> RSS key:
>>>>>>>>>> -
>>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>>> 206F
>>>>>>>>>> -76657272696465
>>>>>>>>>>
>>>>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the
>>>>>> default key.
>>>>>>>>>>
>>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>>>>> API")
>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>> ---
>>>>>>>>>> V4->V5:
>>>>>>>>>> -rewrite the commit log
>>>>>>>>>> -add reviewed-by
>>>>>>>>>
>>>>>>>>> Hi Lijun,
>>>>>>>>>
>>>>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>>>>> but only updated the commit log, can you please check comments to
>>>>>> prev versions.
>>>>>>>>>
>>>>>>>>> Before going into the details, my question was what happens if
>>>>>>>>> default key not provided at all?
>>>>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>>
>>>>>>>>> According commit, the reason of revert is to support following
>>>>>> command:
>>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>>>>>
>>>>>>>>> @Ophir, @Lijun,
>>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>>
>>>>>>>> Hi, Ferruh
>>>>>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>>>>>> there may be other problems with the idea and there is no better
>>>>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>>>>
>>>>>>>
>>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>>> provided doesn't work?
>>>>>>>
>>>>>>
>>>>>> What do you think [1] + following update, will it work?
>>>>>>
>>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
>>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>>>>>> size,
>>>>>>                               }),
>>>>>>                               size > sizeof(*dst.rss) ? sizeof(*dst.rss) : 
>>>>>> size);
>>>>>>                    off = sizeof(*dst.rss);
>>>>>>    -               if (src.rss->key_len) {
>>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>>                            off = RTE_ALIGN_CEIL(off, sizeof(*dst.rss->key));
>>>>>>                            tmp = sizeof(*src.rss->key) * src.rss->key_len;
>>>>>>                            if (size >= off + tmp)
>>>>>>
>>>>>
>>>>> Ferruh, your suggestion ([1] + update) looks correct. I also verified it on 
>>>>> mlx5 PMD.
>>>>> Advantage: it's a generic fix for all dpdk applications using rte_flows 
>>>>> (not just testpmd).
>>>>> It reduces code.
>>>>> With this fix the responsibility of handling key==NULL and/or len==0 is 
>>>>> moved to the PMDs (which is good).
>>>>> With regard to Lijun patch - I liked the approach of overriding the default 
>>>>> testpmd key with the default PMD key.
>>>>> But it only addresses testpmd. More code was added.
>>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing RSS, 
>>>>> but it would feel more confident if we could confirm it for all the PMDs 
>>>>> (by testing) or at least review the PMDs rss_hash_conf_get() implementations.
>>>>>
>>>>
>>>> Lijun's idea can work. There was a problem in implementation related to the 
>>>> key size assumption, which can be fixed.
>>>>
>>>> Even it is fixed, when user gives a rss rule without a key, we are getting 
>>>> key from device and feeding same key back to device, this is unnecessary I 
>>>> think.
>>>> When user didn't provide a key, rss rule shouldn't touch the key at all.
>>>>
>>> Do you mean that the driver should not configure the key to the hardware when 
>>> the RSS key is not specified?
>>
>> We are talking about 'key' in RSS rte flow rule, not generally configuring the 
>> device for RSS.
>>
> Yes, I know. However, I am talking about some implementations. The configuration 
> interface may be the same as that for configuring RSS
> parameters of the device in general mode.
>> If a specific rss rte flow rule, lets say to filter some defined packets to 
>> some specific queues, don't have 'key' as a part of rule, I am suggesting not 
>> touch 'key' configuration of the device.
>>
> Yes, I agree. I think your point of view is very reasonable and a more
> appropriate implementation. However, for the sake of simplicity and
> other considerations in the architecture design of some devices, the
> hardware may support reasonable hybrid configuration for paramter configuration 
> in the RSS config, rather than independent configuration
> for hw, that is, maybe touch A of rss configuration of the device and
> must touch B of rss configuration. As a result, If the preceding scenario 
> occurs, it is reasonable to configure an RSS config that does not change and 
> specified.What do you think?
>>> If so, we cannot identify when the user does not specify the RSS key to 
>>> determine whether to configure the key.
>>
>> I think we can. If the rule has 'key' attribute, driver can use that key to 
>> program the device, if 'key' attribute is missing, don't do anything related 
>> to the rss key.
>>
> Yes, if 'key' attribute is missing, don't do anything related to the rss key. 
> It's more reasonable.
> But, If he can't do that, I think it is reasonable to configure the default key 
> that already exists on the device. The only disadvantage is wasteful.
>>> In hardware design, most vendors do not configure keys for hardware 
>>> independently, which may be associated with other RSS config parameters.
>>> I think it would be reasonable for us to reconfigure the RSS key with the 
>>> default value configured and valid in the hardware as the default value if 
>>> the user does not specify the RSS key.
>>
>> There are already APIs to update the RSS configuration, like RSS key, hash 
>> functions etc.. They are independent from the rte flow RSS rule.
>> Application should configure RSS according needs using those APIs.
>>
> Yes, should do this. Are there any unreasonable users who simply do not know 
> these APIs or do not want to use them and want to configure some parameters 
> through the rte flow RSS rule without changing other parameters?
>> The question here is about each RSS rte flow rule that can update the key. Am 
>> I missing something?
> I don't think you've misunderstood the general situation.
> Even if the RSS key is not specified in a rule, the driver uses the default key 
> value to re-touch the device.
>>
>>>> Complication was when user provides key_len without a key, I think both 
>>>> ignoring or returning error in this case is OK.
>>> I agree. However, I think it is meaningless to expose the RSS key length to 
>>> users. Do you consider deleting the RSS key length directly?
>>
>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can the driver 
>> know the size of the 'key' without 'key_len'?
>> And driver should verify these provided values.
>>
> I know and agree.It is only during the analysis of the [1] scheme, from the time 
> it was proposed to the time it was withdrawn, that I found some guys in the 
> community questioned the significance of key_len.
> 
> Back to the subject, what is your plan for the solution in the patch?

I think, Ophir's old reverted patch [1] + 'rte_flow_conv_action_conf()' update I 
proposed above can work. Is this working for you? If so can you please send a patch?


[1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-20 10:02                         ` Ferruh Yigit
@ 2020-10-20 13:35                           ` oulijun
  2020-10-20 14:34                             ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-20 13:35 UTC (permalink / raw)
  To: Ferruh Yigit, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



在 2020/10/20 18:02, Ferruh Yigit 写道:
> On 10/20/2020 10:00 AM, oulijun wrote:
>>
>>
>> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>>
>>>>
>>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>>>> configuration. Generally, the recommended RSS hash key is 
>>>>>>>>>>> used as
>>>>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>>>>> different from the default RSS flow in testpmd without 
>>>>>>>>>>> specifying
>>>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when 
>>>>>>>>>>> creating
>>>>>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>>>>>> key of the RSS rule. As a result, you may mistakenly consider 
>>>>>>>>>>> that
>>>>>>>>>>> the RSS key in use is the valid default key of the NIC, 
>>>>>>>>>>> actually,
>>>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>>>
>>>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>> RSS functions:
>>>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>>>>> -
>>>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>>>> 0F
>>>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>>>> 2. create a rss rule
>>>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>>>>> testpmd> actions rss \
>>>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>>>
>>>>>>>>>>> 3. show rss-hash key
>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>> RSS functions:
>>>>>>>>>>>     all ipv4-udp udp
>>>>>>>>>>> RSS key:
>>>>>>>>>>> -
>>>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>>>> 206F
>>>>>>>>>>> -76657272696465
>>>>>>>>>>>
>>>>>>>>>>> In order to solve the above problems, it use the NIC valid 
>>>>>>>>>>> default
>>>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>>>> configuration when the RSS key is not specified in the flow 
>>>>>>>>>>> rule.
>>>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS 
>>>>>>>>>>> key as the
>>>>>>> default key.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>>>>>> API")
>>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>> ---
>>>>>>>>>>> V4->V5:
>>>>>>>>>>> -rewrite the commit log
>>>>>>>>>>> -add reviewed-by
>>>>>>>>>>
>>>>>>>>>> Hi Lijun,
>>>>>>>>>>
>>>>>>>>>> There were multiple other comments, it seems they are not 
>>>>>>>>>> addressed
>>>>>>>>>> but only updated the commit log, can you please check comments to
>>>>>>> prev versions.
>>>>>>>>>>
>>>>>>>>>> Before going into the details, my question was what happens if
>>>>>>>>>> default key not provided at all?
>>>>>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>>>
>>>>>>>>>> According commit, the reason of revert is to support following
>>>>>>> command:
>>>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 
>>>>>>>>>> / end"
>>>>>>>>>>
>>>>>>>>>> @Ophir, @Lijun,
>>>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and 
>>>>>>>>>> solve
>>>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>>>
>>>>>>>>> Hi, Ferruh
>>>>>>>>>     I have discussed with Phil Yang about the problem in [1]. I 
>>>>>>>>> think
>>>>>>>>> there may be other problems with the idea and there is no better
>>>>>>>>> solution. and we need to remove key_len definition from 
>>>>>>>>> rte_rss_conf
>>>>>>>>> structure. They don't have a plan. And [1] was eventually 
>>>>>>>>> reverted.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>>>> provided doesn't work?
>>>>>>>>
>>>>>>>
>>>>>>> What do you think [1] + following update, will it work?
>>>>>>>
>>>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>>>>>> b/lib/librte_ethdev/rte_flow.c
>>>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const 
>>>>>>> size_t
>>>>>>> size,
>>>>>>>                               }),
>>>>>>>                               size > sizeof(*dst.rss) ? 
>>>>>>> sizeof(*dst.rss) : size);
>>>>>>>                    off = sizeof(*dst.rss);
>>>>>>>    -               if (src.rss->key_len) {
>>>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>>>                            off = RTE_ALIGN_CEIL(off, 
>>>>>>> sizeof(*dst.rss->key));
>>>>>>>                            tmp = sizeof(*src.rss->key) * 
>>>>>>> src.rss->key_len;
>>>>>>>                            if (size >= off + tmp)
>>>>>>>
>>>>>>
>>>>>> Ferruh, your suggestion ([1] + update) looks correct. I also 
>>>>>> verified it on mlx5 PMD.
>>>>>> Advantage: it's a generic fix for all dpdk applications using 
>>>>>> rte_flows (not just testpmd).
>>>>>> It reduces code.
>>>>>> With this fix the responsibility of handling key==NULL and/or 
>>>>>> len==0 is moved to the PMDs (which is good).
>>>>>> With regard to Lijun patch - I liked the approach of overriding 
>>>>>> the default testpmd key with the default PMD key.
>>>>>> But it only addresses testpmd. More code was added.
>>>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of 
>>>>>> parsing RSS, but it would feel more confident if we could confirm 
>>>>>> it for all the PMDs (by testing) or at least review the PMDs 
>>>>>> rss_hash_conf_get() implementations.
>>>>>>
>>>>>
>>>>> Lijun's idea can work. There was a problem in implementation 
>>>>> related to the key size assumption, which can be fixed.
>>>>>
>>>>> Even it is fixed, when user gives a rss rule without a key, we are 
>>>>> getting key from device and feeding same key back to device, this 
>>>>> is unnecessary I think.
>>>>> When user didn't provide a key, rss rule shouldn't touch the key at 
>>>>> all.
>>>>>
>>>> Do you mean that the driver should not configure the key to the 
>>>> hardware when the RSS key is not specified?
>>>
>>> We are talking about 'key' in RSS rte flow rule, not generally 
>>> configuring the device for RSS.
>>>
>> Yes, I know. However, I am talking about some implementations. The 
>> configuration interface may be the same as that for configuring RSS
>> parameters of the device in general mode.
>>> If a specific rss rte flow rule, lets say to filter some defined 
>>> packets to some specific queues, don't have 'key' as a part of rule, 
>>> I am suggesting not touch 'key' configuration of the device.
>>>
>> Yes, I agree. I think your point of view is very reasonable and a more
>> appropriate implementation. However, for the sake of simplicity and
>> other considerations in the architecture design of some devices, the
>> hardware may support reasonable hybrid configuration for paramter 
>> configuration in the RSS config, rather than independent configuration
>> for hw, that is, maybe touch A of rss configuration of the device and
>> must touch B of rss configuration. As a result, If the preceding 
>> scenario occurs, it is reasonable to configure an RSS config that does 
>> not change and specified.What do you think?
>>>> If so, we cannot identify when the user does not specify the RSS key 
>>>> to determine whether to configure the key.
>>>
>>> I think we can. If the rule has 'key' attribute, driver can use that 
>>> key to program the device, if 'key' attribute is missing, don't do 
>>> anything related to the rss key.
>>>
>> Yes, if 'key' attribute is missing, don't do anything related to the 
>> rss key. It's more reasonable.
>> But, If he can't do that, I think it is reasonable to configure the 
>> default key that already exists on the device. The only disadvantage 
>> is wasteful.
>>>> In hardware design, most vendors do not configure keys for hardware 
>>>> independently, which may be associated with other RSS config 
>>>> parameters.
>>>> I think it would be reasonable for us to reconfigure the RSS key 
>>>> with the default value configured and valid in the hardware as the 
>>>> default value if the user does not specify the RSS key.
>>>
>>> There are already APIs to update the RSS configuration, like RSS key, 
>>> hash functions etc.. They are independent from the rte flow RSS rule.
>>> Application should configure RSS according needs using those APIs.
>>>
>> Yes, should do this. Are there any unreasonable users who simply do 
>> not know these APIs or do not want to use them and want to configure 
>> some parameters through the rte flow RSS rule without changing other 
>> parameters?
>>> The question here is about each RSS rte flow rule that can update the 
>>> key. Am I missing something?
>> I don't think you've misunderstood the general situation.
>> Even if the RSS key is not specified in a rule, the driver uses the 
>> default key value to re-touch the device.
>>>
>>>>> Complication was when user provides key_len without a key, I think 
>>>>> both ignoring or returning error in this case is OK.
>>>> I agree. However, I think it is meaningless to expose the RSS key 
>>>> length to users. Do you consider deleting the RSS key length directly?
>>>
>>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can the 
>>> driver know the size of the 'key' without 'key_len'?
>>> And driver should verify these provided values.
>>>
>> I know and agree.It is only during the analysis of the [1] scheme, 
>> from the time it was proposed to the time it was withdrawn, that I 
>> found some guys in the community questioned the significance of key_len.
>>
>> Back to the subject, what is your plan for the solution in the patch?
> 
> I think, Ophir's old reverted patch [1] + 'rte_flow_conv_action_conf()' 
> update I proposed above can work. Is this working for you? If so can you 
> please send a patch?
> 
> 
> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
> .
Hi, Ferruh
    I've just used [1] + 'rte_flow_conv_action_conf()' for testing based 
on the Kunpeng920 NIC plation and use hns3 pmd driver. it can be resolve 
the RSS key question.
    testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
testpmd> flow create 0 ingress pattern end actions rss func 
symmetric_toeplitz types all end / end
0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
hardware support, requested:7f83fffc configured:3ef8
0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
Flow rule #0 created
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag 
ipv6-tcp ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA

However, I also tested the case. I am not specified the RSS key and 
specified the key_len when create a RSS flow rule, it can create success.
testpmd> flow create 0 ingress pattern end actions rss queues 0 1 end 
key_len 40 / end
0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
hardware support, requested:a38c configured:2288
0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
Flow rule #0 created
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA

> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-20 13:35                           ` oulijun
@ 2020-10-20 14:34                             ` Ferruh Yigit
  2020-10-21  8:19                               ` oulijun
  0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-20 14:34 UTC (permalink / raw)
  To: oulijun, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam

On 10/20/2020 2:35 PM, oulijun wrote:
> 
> 
> 在 2020/10/20 18:02, Ferruh Yigit 写道:
>> On 10/20/2020 10:00 AM, oulijun wrote:
>>>
>>>
>>> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>>>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>>>
>>>>>
>>>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>>>>
>>>>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>>>>>> -
>>>>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>>>>> 0F
>>>>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>>>>> 2. create a rss rule
>>>>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>>>>>> testpmd> actions rss \
>>>>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>>>>
>>>>>>>>>>>> 3. show rss-hash key
>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>     all ipv4-udp udp
>>>>>>>>>>>> RSS key:
>>>>>>>>>>>> -
>>>>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>>>>> 206F
>>>>>>>>>>>> -76657272696465
>>>>>>>>>>>>
>>>>>>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key as the
>>>>>>>> default key.
>>>>>>>>>>>>
>>>>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>>>>>>> API")
>>>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> V4->V5:
>>>>>>>>>>>> -rewrite the commit log
>>>>>>>>>>>> -add reviewed-by
>>>>>>>>>>>
>>>>>>>>>>> Hi Lijun,
>>>>>>>>>>>
>>>>>>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>>>>>>> but only updated the commit log, can you please check comments to
>>>>>>>> prev versions.
>>>>>>>>>>>
>>>>>>>>>>> Before going into the details, my question was what happens if
>>>>>>>>>>> default key not provided at all?
>>>>>>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>>>>
>>>>>>>>>>> According commit, the reason of revert is to support following
>>>>>>>> command:
>>>>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>>>>>>>
>>>>>>>>>>> @Ophir, @Lijun,
>>>>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>>>>
>>>>>>>>>> Hi, Ferruh
>>>>>>>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>>>>>>>> there may be other problems with the idea and there is no better
>>>>>>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>>>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>>>>> provided doesn't work?
>>>>>>>>>
>>>>>>>>
>>>>>>>> What do you think [1] + following update, will it work?
>>>>>>>>
>>>>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
>>>>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>>>>>>>> size,
>>>>>>>>                               }),
>>>>>>>>                               size > sizeof(*dst.rss) ? sizeof(*dst.rss) 
>>>>>>>> : size);
>>>>>>>>                    off = sizeof(*dst.rss);
>>>>>>>>    -               if (src.rss->key_len) {
>>>>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>>>>                            off = RTE_ALIGN_CEIL(off, 
>>>>>>>> sizeof(*dst.rss->key));
>>>>>>>>                            tmp = sizeof(*src.rss->key) * src.rss->key_len;
>>>>>>>>                            if (size >= off + tmp)
>>>>>>>>
>>>>>>>
>>>>>>> Ferruh, your suggestion ([1] + update) looks correct. I also verified it 
>>>>>>> on mlx5 PMD.
>>>>>>> Advantage: it's a generic fix for all dpdk applications using rte_flows 
>>>>>>> (not just testpmd).
>>>>>>> It reduces code.
>>>>>>> With this fix the responsibility of handling key==NULL and/or len==0 is 
>>>>>>> moved to the PMDs (which is good).
>>>>>>> With regard to Lijun patch - I liked the approach of overriding the 
>>>>>>> default testpmd key with the default PMD key.
>>>>>>> But it only addresses testpmd. More code was added.
>>>>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing 
>>>>>>> RSS, but it would feel more confident if we could confirm it for all the 
>>>>>>> PMDs (by testing) or at least review the PMDs rss_hash_conf_get() 
>>>>>>> implementations.
>>>>>>>
>>>>>>
>>>>>> Lijun's idea can work. There was a problem in implementation related to 
>>>>>> the key size assumption, which can be fixed.
>>>>>>
>>>>>> Even it is fixed, when user gives a rss rule without a key, we are getting 
>>>>>> key from device and feeding same key back to device, this is unnecessary I 
>>>>>> think.
>>>>>> When user didn't provide a key, rss rule shouldn't touch the key at all.
>>>>>>
>>>>> Do you mean that the driver should not configure the key to the hardware 
>>>>> when the RSS key is not specified?
>>>>
>>>> We are talking about 'key' in RSS rte flow rule, not generally configuring 
>>>> the device for RSS.
>>>>
>>> Yes, I know. However, I am talking about some implementations. The 
>>> configuration interface may be the same as that for configuring RSS
>>> parameters of the device in general mode.
>>>> If a specific rss rte flow rule, lets say to filter some defined packets to 
>>>> some specific queues, don't have 'key' as a part of rule, I am suggesting 
>>>> not touch 'key' configuration of the device.
>>>>
>>> Yes, I agree. I think your point of view is very reasonable and a more
>>> appropriate implementation. However, for the sake of simplicity and
>>> other considerations in the architecture design of some devices, the
>>> hardware may support reasonable hybrid configuration for paramter 
>>> configuration in the RSS config, rather than independent configuration
>>> for hw, that is, maybe touch A of rss configuration of the device and
>>> must touch B of rss configuration. As a result, If the preceding scenario 
>>> occurs, it is reasonable to configure an RSS config that does not change and 
>>> specified.What do you think?
>>>>> If so, we cannot identify when the user does not specify the RSS key to 
>>>>> determine whether to configure the key.
>>>>
>>>> I think we can. If the rule has 'key' attribute, driver can use that key to 
>>>> program the device, if 'key' attribute is missing, don't do anything related 
>>>> to the rss key.
>>>>
>>> Yes, if 'key' attribute is missing, don't do anything related to the rss key. 
>>> It's more reasonable.
>>> But, If he can't do that, I think it is reasonable to configure the default 
>>> key that already exists on the device. The only disadvantage is wasteful.
>>>>> In hardware design, most vendors do not configure keys for hardware 
>>>>> independently, which may be associated with other RSS config parameters.
>>>>> I think it would be reasonable for us to reconfigure the RSS key with the 
>>>>> default value configured and valid in the hardware as the default value if 
>>>>> the user does not specify the RSS key.
>>>>
>>>> There are already APIs to update the RSS configuration, like RSS key, hash 
>>>> functions etc.. They are independent from the rte flow RSS rule.
>>>> Application should configure RSS according needs using those APIs.
>>>>
>>> Yes, should do this. Are there any unreasonable users who simply do not know 
>>> these APIs or do not want to use them and want to configure some parameters 
>>> through the rte flow RSS rule without changing other parameters?
>>>> The question here is about each RSS rte flow rule that can update the key. 
>>>> Am I missing something?
>>> I don't think you've misunderstood the general situation.
>>> Even if the RSS key is not specified in a rule, the driver uses the default 
>>> key value to re-touch the device.
>>>>
>>>>>> Complication was when user provides key_len without a key, I think both 
>>>>>> ignoring or returning error in this case is OK.
>>>>> I agree. However, I think it is meaningless to expose the RSS key length to 
>>>>> users. Do you consider deleting the RSS key length directly?
>>>>
>>>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can the driver 
>>>> know the size of the 'key' without 'key_len'?
>>>> And driver should verify these provided values.
>>>>
>>> I know and agree.It is only during the analysis of the [1] scheme, from the 
>>> time it was proposed to the time it was withdrawn, that I found some guys in 
>>> the community questioned the significance of key_len.
>>>
>>> Back to the subject, what is your plan for the solution in the patch?
>>
>> I think, Ophir's old reverted patch [1] + 'rte_flow_conv_action_conf()' update 
>> I proposed above can work. Is this working for you? If so can you please send 
>> a patch?
>>
>>
>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>> .
> Hi, Ferruh
>     I've just used [1] + 'rte_flow_conv_action_conf()' for testing based on the 
> Kunpeng920 NIC plation and use hns3 pmd driver. it can be resolve the RSS key 
> question.
>     testpmd> show port 0 rss-hash key
> RSS functions:
>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
> testpmd> flow create 0 ingress pattern end actions rss func symmetric_toeplitz 
> types all end / end
> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on hardware 
> support, requested:7f83fffc configured:3ef8
> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
> Flow rule #0 created
> testpmd> show port 0 rss-hash key
> RSS functions:
>   all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag ipv6-tcp 
> ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
> 
> However, I also tested the case. I am not specified the RSS key and specified 
> the key_len when create a RSS flow rule, it can create success.
> testpmd> flow create 0 ingress pattern end actions rss queues 0 1 end key_len 40 
> / end
> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on hardware 
> support, requested:a38c configured:2288
> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
> Flow rule #0 created
> testpmd> show port 0 rss-hash key
> RSS functions:
>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
> RSS key:
> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
> 

So are you happy with this solution?

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-20 14:34                             ` Ferruh Yigit
@ 2020-10-21  8:19                               ` oulijun
  2020-10-21  9:38                                 ` Ferruh Yigit
  0 siblings, 1 reply; 44+ messages in thread
From: oulijun @ 2020-10-21  8:19 UTC (permalink / raw)
  To: Ferruh Yigit, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



在 2020/10/20 22:34, Ferruh Yigit 写道:
> On 10/20/2020 2:35 PM, oulijun wrote:
>>
>>
>> 在 2020/10/20 18:02, Ferruh Yigit 写道:
>>> On 10/20/2020 10:00 AM, oulijun wrote:
>>>>
>>>>
>>>> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>>>>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>>>>
>>>>>>
>>>>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>>>>>> configuration. Generally, the recommended RSS hash key is 
>>>>>>>>>>>>> used as
>>>>>>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>>>>>>> different from the default RSS flow in testpmd without 
>>>>>>>>>>>>> specifying
>>>>>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when 
>>>>>>>>>>>>> creating
>>>>>>>>>>>>> an RSS rule, the testpmd uses the default key as the 
>>>>>>>>>>>>> default RSS
>>>>>>>>>>>>> key of the RSS rule. As a result, you may mistakenly 
>>>>>>>>>>>>> consider that
>>>>>>>>>>>>> the RSS key in use is the valid default key of the NIC, 
>>>>>>>>>>>>> actually,
>>>>>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>>>>>>> -
>>>>>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>>>>>> 0F
>>>>>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>>>>>> 2. create a rss rule
>>>>>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>>>>>>> testpmd> actions rss \
>>>>>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>>>>>
>>>>>>>>>>>>> 3. show rss-hash key
>>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>>     all ipv4-udp udp
>>>>>>>>>>>>> RSS key:
>>>>>>>>>>>>> -
>>>>>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>>>>>> 206F
>>>>>>>>>>>>> -76657272696465
>>>>>>>>>>>>>
>>>>>>>>>>>>> In order to solve the above problems, it use the NIC valid 
>>>>>>>>>>>>> default
>>>>>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>>>>>> configuration when the RSS key is not specified in the flow 
>>>>>>>>>>>>> rule.
>>>>>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy 
>>>>>>>>>>>>> RSS key as the
>>>>>>>>> default key.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in 
>>>>>>>>>>>>> flow
>>>>>>>>>>>>> API")
>>>>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> V4->V5:
>>>>>>>>>>>>> -rewrite the commit log
>>>>>>>>>>>>> -add reviewed-by
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Lijun,
>>>>>>>>>>>>
>>>>>>>>>>>> There were multiple other comments, it seems they are not 
>>>>>>>>>>>> addressed
>>>>>>>>>>>> but only updated the commit log, can you please check 
>>>>>>>>>>>> comments to
>>>>>>>>> prev versions.
>>>>>>>>>>>>
>>>>>>>>>>>> Before going into the details, my question was what happens if
>>>>>>>>>>>> default key not provided at all?
>>>>>>>>>>>> It seems this has been already tried by Ophir [1], later 
>>>>>>>>>>>> reverted
>>>>>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>>>>>
>>>>>>>>>>>> According commit, the reason of revert is to support following
>>>>>>>>> command:
>>>>>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 
>>>>>>>>>>>> 40 / end"
>>>>>>>>>>>>
>>>>>>>>>>>> @Ophir, @Lijun,
>>>>>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported 
>>>>>>>>>>>> and solve
>>>>>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>>>>>
>>>>>>>>>>> Hi, Ferruh
>>>>>>>>>>>     I have discussed with Phil Yang about the problem in [1]. 
>>>>>>>>>>> I think
>>>>>>>>>>> there may be other problems with the idea and there is no better
>>>>>>>>>>> solution. and we need to remove key_len definition from 
>>>>>>>>>>> rte_rss_conf
>>>>>>>>>>> structure. They don't have a plan. And [1] was eventually 
>>>>>>>>>>> reverted.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>>>>>> provided doesn't work?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> What do you think [1] + following update, will it work?
>>>>>>>>>
>>>>>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>>>>>>>> b/lib/librte_ethdev/rte_flow.c
>>>>>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, 
>>>>>>>>> const size_t
>>>>>>>>> size,
>>>>>>>>>                               }),
>>>>>>>>>                               size > sizeof(*dst.rss) ? 
>>>>>>>>> sizeof(*dst.rss) : size);
>>>>>>>>>                    off = sizeof(*dst.rss);
>>>>>>>>>    -               if (src.rss->key_len) {
>>>>>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>>>>>                            off = RTE_ALIGN_CEIL(off, 
>>>>>>>>> sizeof(*dst.rss->key));
>>>>>>>>>                            tmp = sizeof(*src.rss->key) * 
>>>>>>>>> src.rss->key_len;
>>>>>>>>>                            if (size >= off + tmp)
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ferruh, your suggestion ([1] + update) looks correct. I also 
>>>>>>>> verified it on mlx5 PMD.
>>>>>>>> Advantage: it's a generic fix for all dpdk applications using 
>>>>>>>> rte_flows (not just testpmd).
>>>>>>>> It reduces code.
>>>>>>>> With this fix the responsibility of handling key==NULL and/or 
>>>>>>>> len==0 is moved to the PMDs (which is good).
>>>>>>>> With regard to Lijun patch - I liked the approach of overriding 
>>>>>>>> the default testpmd key with the default PMD key.
>>>>>>>> But it only addresses testpmd. More code was added.
>>>>>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of 
>>>>>>>> parsing RSS, but it would feel more confident if we could 
>>>>>>>> confirm it for all the PMDs (by testing) or at least review the 
>>>>>>>> PMDs rss_hash_conf_get() implementations.
>>>>>>>>
>>>>>>>
>>>>>>> Lijun's idea can work. There was a problem in implementation 
>>>>>>> related to the key size assumption, which can be fixed.
>>>>>>>
>>>>>>> Even it is fixed, when user gives a rss rule without a key, we 
>>>>>>> are getting key from device and feeding same key back to device, 
>>>>>>> this is unnecessary I think.
>>>>>>> When user didn't provide a key, rss rule shouldn't touch the key 
>>>>>>> at all.
>>>>>>>
>>>>>> Do you mean that the driver should not configure the key to the 
>>>>>> hardware when the RSS key is not specified?
>>>>>
>>>>> We are talking about 'key' in RSS rte flow rule, not generally 
>>>>> configuring the device for RSS.
>>>>>
>>>> Yes, I know. However, I am talking about some implementations. The 
>>>> configuration interface may be the same as that for configuring RSS
>>>> parameters of the device in general mode.
>>>>> If a specific rss rte flow rule, lets say to filter some defined 
>>>>> packets to some specific queues, don't have 'key' as a part of 
>>>>> rule, I am suggesting not touch 'key' configuration of the device.
>>>>>
>>>> Yes, I agree. I think your point of view is very reasonable and a more
>>>> appropriate implementation. However, for the sake of simplicity and
>>>> other considerations in the architecture design of some devices, the
>>>> hardware may support reasonable hybrid configuration for paramter 
>>>> configuration in the RSS config, rather than independent configuration
>>>> for hw, that is, maybe touch A of rss configuration of the device and
>>>> must touch B of rss configuration. As a result, If the preceding 
>>>> scenario occurs, it is reasonable to configure an RSS config that 
>>>> does not change and specified.What do you think?
>>>>>> If so, we cannot identify when the user does not specify the RSS 
>>>>>> key to determine whether to configure the key.
>>>>>
>>>>> I think we can. If the rule has 'key' attribute, driver can use 
>>>>> that key to program the device, if 'key' attribute is missing, 
>>>>> don't do anything related to the rss key.
>>>>>
>>>> Yes, if 'key' attribute is missing, don't do anything related to the 
>>>> rss key. It's more reasonable.
>>>> But, If he can't do that, I think it is reasonable to configure the 
>>>> default key that already exists on the device. The only disadvantage 
>>>> is wasteful.
>>>>>> In hardware design, most vendors do not configure keys for 
>>>>>> hardware independently, which may be associated with other RSS 
>>>>>> config parameters.
>>>>>> I think it would be reasonable for us to reconfigure the RSS key 
>>>>>> with the default value configured and valid in the hardware as the 
>>>>>> default value if the user does not specify the RSS key.
>>>>>
>>>>> There are already APIs to update the RSS configuration, like RSS 
>>>>> key, hash functions etc.. They are independent from the rte flow 
>>>>> RSS rule.
>>>>> Application should configure RSS according needs using those APIs.
>>>>>
>>>> Yes, should do this. Are there any unreasonable users who simply do 
>>>> not know these APIs or do not want to use them and want to configure 
>>>> some parameters through the rte flow RSS rule without changing other 
>>>> parameters?
>>>>> The question here is about each RSS rte flow rule that can update 
>>>>> the key. Am I missing something?
>>>> I don't think you've misunderstood the general situation.
>>>> Even if the RSS key is not specified in a rule, the driver uses the 
>>>> default key value to re-touch the device.
>>>>>
>>>>>>> Complication was when user provides key_len without a key, I 
>>>>>>> think both ignoring or returning error in this case is OK.
>>>>>> I agree. However, I think it is meaningless to expose the RSS key 
>>>>>> length to users. Do you consider deleting the RSS key length 
>>>>>> directly?
>>>>>
>>>>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can 
>>>>> the driver know the size of the 'key' without 'key_len'?
>>>>> And driver should verify these provided values.
>>>>>
>>>> I know and agree.It is only during the analysis of the [1] scheme, 
>>>> from the time it was proposed to the time it was withdrawn, that I 
>>>> found some guys in the community questioned the significance of 
>>>> key_len.
>>>>
>>>> Back to the subject, what is your plan for the solution in the patch?
>>>
>>> I think, Ophir's old reverted patch [1] + 
>>> 'rte_flow_conv_action_conf()' update I proposed above can work. Is 
>>> this working for you? If so can you please send a patch?
>>>
>>>
>>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>>> .
>> Hi, Ferruh
>>     I've just used [1] + 'rte_flow_conv_action_conf()' for testing 
>> based on the Kunpeng920 NIC plation and use hns3 pmd driver. it can be 
>> resolve the RSS key question.
>>     testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>
>> testpmd> flow create 0 ingress pattern end actions rss func 
>> symmetric_toeplitz types all end / end
>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
>> hardware support, requested:7f83fffc configured:3ef8
>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
>> Flow rule #0 created
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag 
>> ipv6-tcp ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>
>>
>> However, I also tested the case. I am not specified the RSS key and 
>> specified the key_len when create a RSS flow rule, it can create success.
>> testpmd> flow create 0 ingress pattern end actions rss queues 0 1 end 
>> key_len 40 / end
>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
>> hardware support, requested:a38c configured:2288
>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
>> Flow rule #0 created
>> testpmd> show port 0 rss-hash key
>> RSS functions:
>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>> RSS key:
>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>
>>
> 
> So are you happy with this solution?
> .
Yes, thanks.
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-21  8:19                               ` oulijun
@ 2020-10-21  9:38                                 ` Ferruh Yigit
  2020-10-21 10:07                                   ` oulijun
  0 siblings, 1 reply; 44+ messages in thread
From: Ferruh Yigit @ 2020-10-21  9:38 UTC (permalink / raw)
  To: oulijun, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam

On 10/21/2020 9:19 AM, oulijun wrote:
> 
> 
> 在 2020/10/20 22:34, Ferruh Yigit 写道:
>> On 10/20/2020 2:35 PM, oulijun wrote:
>>>
>>>
>>> 在 2020/10/20 18:02, Ferruh Yigit 写道:
>>>> On 10/20/2020 10:00 AM, oulijun wrote:
>>>>>
>>>>>
>>>>> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>>>>>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>>>>>
>>>>>>>
>>>>>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>>>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>>>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>>>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>>>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>>>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>>>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>>>>>>>>> -
>>>>>>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>>>>>>> 0F
>>>>>>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>>>>>>> 2. create a rss rule
>>>>>>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>>>>>>>>> testpmd> actions rss \
>>>>>>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 3. show rss-hash key
>>>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>>>     all ipv4-udp udp
>>>>>>>>>>>>>> RSS key:
>>>>>>>>>>>>>> -
>>>>>>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>>>>>>> 206F
>>>>>>>>>>>>>> -76657272696465
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key 
>>>>>>>>>>>>>> as the
>>>>>>>>>> default key.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>>>>>>>>> API")
>>>>>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> V4->V5:
>>>>>>>>>>>>>> -rewrite the commit log
>>>>>>>>>>>>>> -add reviewed-by
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Lijun,
>>>>>>>>>>>>>
>>>>>>>>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>>>>>>>>> but only updated the commit log, can you please check comments to
>>>>>>>>>> prev versions.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Before going into the details, my question was what happens if
>>>>>>>>>>>>> default key not provided at all?
>>>>>>>>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>>>>>>
>>>>>>>>>>>>> According commit, the reason of revert is to support following
>>>>>>>>>> command:
>>>>>>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>>>>>>>>>
>>>>>>>>>>>>> @Ophir, @Lijun,
>>>>>>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>>>>>>
>>>>>>>>>>>> Hi, Ferruh
>>>>>>>>>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>>>>>>>>>> there may be other problems with the idea and there is no better
>>>>>>>>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>>>>>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>>>>>>> provided doesn't work?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> What do you think [1] + following update, will it work?
>>>>>>>>>>
>>>>>>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>>>>>>>>> b/lib/librte_ethdev/rte_flow.c
>>>>>>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>>>>>>>>>> size,
>>>>>>>>>>                               }),
>>>>>>>>>>                               size > sizeof(*dst.rss) ? 
>>>>>>>>>> sizeof(*dst.rss) : size);
>>>>>>>>>>                    off = sizeof(*dst.rss);
>>>>>>>>>>    -               if (src.rss->key_len) {
>>>>>>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>>>>>>                            off = RTE_ALIGN_CEIL(off, 
>>>>>>>>>> sizeof(*dst.rss->key));
>>>>>>>>>>                            tmp = sizeof(*src.rss->key) * 
>>>>>>>>>> src.rss->key_len;
>>>>>>>>>>                            if (size >= off + tmp)
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Ferruh, your suggestion ([1] + update) looks correct. I also verified 
>>>>>>>>> it on mlx5 PMD.
>>>>>>>>> Advantage: it's a generic fix for all dpdk applications using rte_flows 
>>>>>>>>> (not just testpmd).
>>>>>>>>> It reduces code.
>>>>>>>>> With this fix the responsibility of handling key==NULL and/or len==0 is 
>>>>>>>>> moved to the PMDs (which is good).
>>>>>>>>> With regard to Lijun patch - I liked the approach of overriding the 
>>>>>>>>> default testpmd key with the default PMD key.
>>>>>>>>> But it only addresses testpmd. More code was added.
>>>>>>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing 
>>>>>>>>> RSS, but it would feel more confident if we could confirm it for all 
>>>>>>>>> the PMDs (by testing) or at least review the PMDs rss_hash_conf_get() 
>>>>>>>>> implementations.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Lijun's idea can work. There was a problem in implementation related to 
>>>>>>>> the key size assumption, which can be fixed.
>>>>>>>>
>>>>>>>> Even it is fixed, when user gives a rss rule without a key, we are 
>>>>>>>> getting key from device and feeding same key back to device, this is 
>>>>>>>> unnecessary I think.
>>>>>>>> When user didn't provide a key, rss rule shouldn't touch the key at all.
>>>>>>>>
>>>>>>> Do you mean that the driver should not configure the key to the hardware 
>>>>>>> when the RSS key is not specified?
>>>>>>
>>>>>> We are talking about 'key' in RSS rte flow rule, not generally configuring 
>>>>>> the device for RSS.
>>>>>>
>>>>> Yes, I know. However, I am talking about some implementations. The 
>>>>> configuration interface may be the same as that for configuring RSS
>>>>> parameters of the device in general mode.
>>>>>> If a specific rss rte flow rule, lets say to filter some defined packets 
>>>>>> to some specific queues, don't have 'key' as a part of rule, I am 
>>>>>> suggesting not touch 'key' configuration of the device.
>>>>>>
>>>>> Yes, I agree. I think your point of view is very reasonable and a more
>>>>> appropriate implementation. However, for the sake of simplicity and
>>>>> other considerations in the architecture design of some devices, the
>>>>> hardware may support reasonable hybrid configuration for paramter 
>>>>> configuration in the RSS config, rather than independent configuration
>>>>> for hw, that is, maybe touch A of rss configuration of the device and
>>>>> must touch B of rss configuration. As a result, If the preceding scenario 
>>>>> occurs, it is reasonable to configure an RSS config that does not change 
>>>>> and specified.What do you think?
>>>>>>> If so, we cannot identify when the user does not specify the RSS key to 
>>>>>>> determine whether to configure the key.
>>>>>>
>>>>>> I think we can. If the rule has 'key' attribute, driver can use that key 
>>>>>> to program the device, if 'key' attribute is missing, don't do anything 
>>>>>> related to the rss key.
>>>>>>
>>>>> Yes, if 'key' attribute is missing, don't do anything related to the rss 
>>>>> key. It's more reasonable.
>>>>> But, If he can't do that, I think it is reasonable to configure the default 
>>>>> key that already exists on the device. The only disadvantage is wasteful.
>>>>>>> In hardware design, most vendors do not configure keys for hardware 
>>>>>>> independently, which may be associated with other RSS config parameters.
>>>>>>> I think it would be reasonable for us to reconfigure the RSS key with the 
>>>>>>> default value configured and valid in the hardware as the default value 
>>>>>>> if the user does not specify the RSS key.
>>>>>>
>>>>>> There are already APIs to update the RSS configuration, like RSS key, hash 
>>>>>> functions etc.. They are independent from the rte flow RSS rule.
>>>>>> Application should configure RSS according needs using those APIs.
>>>>>>
>>>>> Yes, should do this. Are there any unreasonable users who simply do not 
>>>>> know these APIs or do not want to use them and want to configure some 
>>>>> parameters through the rte flow RSS rule without changing other parameters?
>>>>>> The question here is about each RSS rte flow rule that can update the key. 
>>>>>> Am I missing something?
>>>>> I don't think you've misunderstood the general situation.
>>>>> Even if the RSS key is not specified in a rule, the driver uses the default 
>>>>> key value to re-touch the device.
>>>>>>
>>>>>>>> Complication was when user provides key_len without a key, I think both 
>>>>>>>> ignoring or returning error in this case is OK.
>>>>>>> I agree. However, I think it is meaningless to expose the RSS key length 
>>>>>>> to users. Do you consider deleting the RSS key length directly?
>>>>>>
>>>>>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can the 
>>>>>> driver know the size of the 'key' without 'key_len'?
>>>>>> And driver should verify these provided values.
>>>>>>
>>>>> I know and agree.It is only during the analysis of the [1] scheme, from the 
>>>>> time it was proposed to the time it was withdrawn, that I found some guys 
>>>>> in the community questioned the significance of key_len.
>>>>>
>>>>> Back to the subject, what is your plan for the solution in the patch?
>>>>
>>>> I think, Ophir's old reverted patch [1] + 'rte_flow_conv_action_conf()' 
>>>> update I proposed above can work. Is this working for you? If so can you 
>>>> please send a patch?
>>>>
>>>>
>>>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>>>> .
>>> Hi, Ferruh
>>>     I've just used [1] + 'rte_flow_conv_action_conf()' for testing based on 
>>> the Kunpeng920 NIC plation and use hns3 pmd driver. it can be resolve the RSS 
>>> key question.
>>>     testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>> RSS key:
>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
>>> testpmd> flow create 0 ingress pattern end actions rss func 
>>> symmetric_toeplitz types all end / end
>>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on hardware 
>>> support, requested:7f83fffc configured:3ef8
>>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
>>> Flow rule #0 created
>>> testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>   all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag ipv6-tcp 
>>> ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
>>> RSS key:
>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
>>>
>>> However, I also tested the case. I am not specified the RSS key and specified 
>>> the key_len when create a RSS flow rule, it can create success.
>>> testpmd> flow create 0 ingress pattern end actions rss queues 0 1 end key_len 
>>> 40 / end
>>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on hardware 
>>> support, requested:a38c configured:2288
>>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using default
>>> Flow rule #0 created
>>> testpmd> show port 0 rss-hash key
>>> RSS functions:
>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>> RSS key:
>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA
>>>
>>
>> So are you happy with this solution?
>> .
> Yes, thanks.

cool, will you able to send a new version? Thanks.

^ permalink raw reply	[flat|nested] 44+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
  2020-10-21  9:38                                 ` Ferruh Yigit
@ 2020-10-21 10:07                                   ` oulijun
  0 siblings, 0 replies; 44+ messages in thread
From: oulijun @ 2020-10-21 10:07 UTC (permalink / raw)
  To: Ferruh Yigit, Ophir Munk, wenzhuo.lu, beilei.xing,
	NBU-Contact-Adrien Mazarguil
  Cc: dev, linuxarm, Ori Kam



在 2020/10/21 17:38, Ferruh Yigit 写道:
> On 10/21/2020 9:19 AM, oulijun wrote:
>>
>>
>> 在 2020/10/20 22:34, Ferruh Yigit 写道:
>>> On 10/20/2020 2:35 PM, oulijun wrote:
>>>>
>>>>
>>>> 在 2020/10/20 18:02, Ferruh Yigit 写道:
>>>>> On 10/20/2020 10:00 AM, oulijun wrote:
>>>>>>
>>>>>>
>>>>>> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>>>>>>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>>>>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>>>>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>>>>>>>>>> configuration. Generally, the recommended RSS hash key is 
>>>>>>>>>>>>>>> used as
>>>>>>>>>>>>>>> the default key in the driver. In addition, the default 
>>>>>>>>>>>>>>> key is
>>>>>>>>>>>>>>> different from the default RSS flow in testpmd without 
>>>>>>>>>>>>>>> specifying
>>>>>>>>>>>>>>> RSS hash key. So. if you do not specify the RSS key when 
>>>>>>>>>>>>>>> creating
>>>>>>>>>>>>>>> an RSS rule, the testpmd uses the default key as the 
>>>>>>>>>>>>>>> default RSS
>>>>>>>>>>>>>>> key of the RSS rule. As a result, you may mistakenly 
>>>>>>>>>>>>>>> consider that
>>>>>>>>>>>>>>> the RSS key in use is the valid default key of the NIC, 
>>>>>>>>>>>>>>> actually,
>>>>>>>>>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Consider the follow usage with testpmd:
>>>>>>>>>>>>>>> 1. first, startup testpmd:
>>>>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS 
>>>>>>>>>>>>>>> key:
>>>>>>>>>>>>>>> -
>>>>>>>>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>>>>>>>>>> 0F
>>>>>>>>>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>>>>>>>>>> 2. create a rss rule
>>>>>>>>>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / 
>>>>>>>>>>>>>>> end
>>>>>>>>>>>>>>> testpmd> actions rss \
>>>>>>>>>>>>>>> types ipv4-udp end queues end / end
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 3. show rss-hash key
>>>>>>>>>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>>>>>>>>>> RSS functions:
>>>>>>>>>>>>>>>     all ipv4-udp udp
>>>>>>>>>>>>>>> RSS key:
>>>>>>>>>>>>>>> -
>>>>>>>>>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>>>>>>>>>> 206F
>>>>>>>>>>>>>>> -76657272696465
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In order to solve the above problems, it use the NIC 
>>>>>>>>>>>>>>> valid default
>>>>>>>>>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>>>>>>>>>> configuration when the RSS key is not specified in the 
>>>>>>>>>>>>>>> flow rule.
>>>>>>>>>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy 
>>>>>>>>>>>>>>> RSS key as the
>>>>>>>>>>> default key.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration 
>>>>>>>>>>>>>>> in flow
>>>>>>>>>>>>>>> API")
>>>>>>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> V4->V5:
>>>>>>>>>>>>>>> -rewrite the commit log
>>>>>>>>>>>>>>> -add reviewed-by
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Lijun,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There were multiple other comments, it seems they are not 
>>>>>>>>>>>>>> addressed
>>>>>>>>>>>>>> but only updated the commit log, can you please check 
>>>>>>>>>>>>>> comments to
>>>>>>>>>>> prev versions.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Before going into the details, my question was what 
>>>>>>>>>>>>>> happens if
>>>>>>>>>>>>>> default key not provided at all?
>>>>>>>>>>>>>> It seems this has been already tried by Ophir [1], later 
>>>>>>>>>>>>>> reverted
>>>>>>>>>>>>>> back [2] bringing the initial issue back.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> According commit, the reason of revert is to support 
>>>>>>>>>>>>>> following
>>>>>>>>>>> command:
>>>>>>>>>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end 
>>>>>>>>>>>>>> key_len 40 / end"
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> @Ophir, @Lijun,
>>>>>>>>>>>>>> Can we ignore the 'key_len' if the 'key' is not supported 
>>>>>>>>>>>>>> and solve
>>>>>>>>>>>>>> current issue as initially intended ([1])?
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi, Ferruh
>>>>>>>>>>>>>     I have discussed with Phil Yang about the problem in 
>>>>>>>>>>>>> [1]. I think
>>>>>>>>>>>>> there may be other problems with the idea and there is no 
>>>>>>>>>>>>> better
>>>>>>>>>>>>> solution. and we need to remove key_len definition from 
>>>>>>>>>>>>> rte_rss_conf
>>>>>>>>>>>>> structure. They don't have a plan. And [1] was eventually 
>>>>>>>>>>>>> reverted.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>>>>>>>>>> provided doesn't work?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> What do you think [1] + following update, will it work?
>>>>>>>>>>>
>>>>>>>>>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>>>>>>>>>> b/lib/librte_ethdev/rte_flow.c
>>>>>>>>>>>    index ee4f3464fe..e7789c87b3 100644
>>>>>>>>>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>>>>>>>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>>>>>>>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, 
>>>>>>>>>>> const size_t
>>>>>>>>>>> size,
>>>>>>>>>>>                               }),
>>>>>>>>>>>                               size > sizeof(*dst.rss) ? 
>>>>>>>>>>> sizeof(*dst.rss) : size);
>>>>>>>>>>>                    off = sizeof(*dst.rss);
>>>>>>>>>>>    -               if (src.rss->key_len) {
>>>>>>>>>>>    +               if (src.rss->key_len && src.rss->key) {
>>>>>>>>>>>                            off = RTE_ALIGN_CEIL(off, 
>>>>>>>>>>> sizeof(*dst.rss->key));
>>>>>>>>>>>                            tmp = sizeof(*src.rss->key) * 
>>>>>>>>>>> src.rss->key_len;
>>>>>>>>>>>                            if (size >= off + tmp)
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Ferruh, your suggestion ([1] + update) looks correct. I also 
>>>>>>>>>> verified it on mlx5 PMD.
>>>>>>>>>> Advantage: it's a generic fix for all dpdk applications using 
>>>>>>>>>> rte_flows (not just testpmd).
>>>>>>>>>> It reduces code.
>>>>>>>>>> With this fix the responsibility of handling key==NULL and/or 
>>>>>>>>>> len==0 is moved to the PMDs (which is good).
>>>>>>>>>> With regard to Lijun patch - I liked the approach of 
>>>>>>>>>> overriding the default testpmd key with the default PMD key.
>>>>>>>>>> But it only addresses testpmd. More code was added.
>>>>>>>>>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of 
>>>>>>>>>> parsing RSS, but it would feel more confident if we could 
>>>>>>>>>> confirm it for all the PMDs (by testing) or at least review 
>>>>>>>>>> the PMDs rss_hash_conf_get() implementations.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Lijun's idea can work. There was a problem in implementation 
>>>>>>>>> related to the key size assumption, which can be fixed.
>>>>>>>>>
>>>>>>>>> Even it is fixed, when user gives a rss rule without a key, we 
>>>>>>>>> are getting key from device and feeding same key back to 
>>>>>>>>> device, this is unnecessary I think.
>>>>>>>>> When user didn't provide a key, rss rule shouldn't touch the 
>>>>>>>>> key at all.
>>>>>>>>>
>>>>>>>> Do you mean that the driver should not configure the key to the 
>>>>>>>> hardware when the RSS key is not specified?
>>>>>>>
>>>>>>> We are talking about 'key' in RSS rte flow rule, not generally 
>>>>>>> configuring the device for RSS.
>>>>>>>
>>>>>> Yes, I know. However, I am talking about some implementations. The 
>>>>>> configuration interface may be the same as that for configuring RSS
>>>>>> parameters of the device in general mode.
>>>>>>> If a specific rss rte flow rule, lets say to filter some defined 
>>>>>>> packets to some specific queues, don't have 'key' as a part of 
>>>>>>> rule, I am suggesting not touch 'key' configuration of the device.
>>>>>>>
>>>>>> Yes, I agree. I think your point of view is very reasonable and a 
>>>>>> more
>>>>>> appropriate implementation. However, for the sake of simplicity and
>>>>>> other considerations in the architecture design of some devices, the
>>>>>> hardware may support reasonable hybrid configuration for paramter 
>>>>>> configuration in the RSS config, rather than independent 
>>>>>> configuration
>>>>>> for hw, that is, maybe touch A of rss configuration of the device and
>>>>>> must touch B of rss configuration. As a result, If the preceding 
>>>>>> scenario occurs, it is reasonable to configure an RSS config that 
>>>>>> does not change and specified.What do you think?
>>>>>>>> If so, we cannot identify when the user does not specify the RSS 
>>>>>>>> key to determine whether to configure the key.
>>>>>>>
>>>>>>> I think we can. If the rule has 'key' attribute, driver can use 
>>>>>>> that key to program the device, if 'key' attribute is missing, 
>>>>>>> don't do anything related to the rss key.
>>>>>>>
>>>>>> Yes, if 'key' attribute is missing, don't do anything related to 
>>>>>> the rss key. It's more reasonable.
>>>>>> But, If he can't do that, I think it is reasonable to configure 
>>>>>> the default key that already exists on the device. The only 
>>>>>> disadvantage is wasteful.
>>>>>>>> In hardware design, most vendors do not configure keys for 
>>>>>>>> hardware independently, which may be associated with other RSS 
>>>>>>>> config parameters.
>>>>>>>> I think it would be reasonable for us to reconfigure the RSS key 
>>>>>>>> with the default value configured and valid in the hardware as 
>>>>>>>> the default value if the user does not specify the RSS key.
>>>>>>>
>>>>>>> There are already APIs to update the RSS configuration, like RSS 
>>>>>>> key, hash functions etc.. They are independent from the rte flow 
>>>>>>> RSS rule.
>>>>>>> Application should configure RSS according needs using those APIs.
>>>>>>>
>>>>>> Yes, should do this. Are there any unreasonable users who simply 
>>>>>> do not know these APIs or do not want to use them and want to 
>>>>>> configure some parameters through the rte flow RSS rule without 
>>>>>> changing other parameters?
>>>>>>> The question here is about each RSS rte flow rule that can update 
>>>>>>> the key. Am I missing something?
>>>>>> I don't think you've misunderstood the general situation.
>>>>>> Even if the RSS key is not specified in a rule, the driver uses 
>>>>>> the default key value to re-touch the device.
>>>>>>>
>>>>>>>>> Complication was when user provides key_len without a key, I 
>>>>>>>>> think both ignoring or returning error in this case is OK.
>>>>>>>> I agree. However, I think it is meaningless to expose the RSS 
>>>>>>>> key length to users. Do you consider deleting the RSS key length 
>>>>>>>> directly?
>>>>>>>
>>>>>>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can 
>>>>>>> the driver know the size of the 'key' without 'key_len'?
>>>>>>> And driver should verify these provided values.
>>>>>>>
>>>>>> I know and agree.It is only during the analysis of the [1] scheme, 
>>>>>> from the time it was proposed to the time it was withdrawn, that I 
>>>>>> found some guys in the community questioned the significance of 
>>>>>> key_len.
>>>>>>
>>>>>> Back to the subject, what is your plan for the solution in the patch?
>>>>>
>>>>> I think, Ophir's old reverted patch [1] + 
>>>>> 'rte_flow_conv_action_conf()' update I proposed above can work. Is 
>>>>> this working for you? If so can you please send a patch?
>>>>>
>>>>>
>>>>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>>>>> .
>>>> Hi, Ferruh
>>>>     I've just used [1] + 'rte_flow_conv_action_conf()' for testing 
>>>> based on the Kunpeng920 NIC plation and use hns3 pmd driver. it can 
>>>> be resolve the RSS key question.
>>>>     testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>>>
>>>> testpmd> flow create 0 ingress pattern end actions rss func 
>>>> symmetric_toeplitz types all end / end
>>>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
>>>> hardware support, requested:7f83fffc configured:3ef8
>>>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using 
>>>> default
>>>> Flow rule #0 created
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag 
>>>> ipv6-tcp ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>>>
>>>>
>>>> However, I also tested the case. I am not specified the RSS key and 
>>>> specified the key_len when create a RSS flow rule, it can create 
>>>> success.
>>>> testpmd> flow create 0 ingress pattern end actions rss queues 0 1 
>>>> end key_len 40 / end
>>>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
>>>> hardware support, requested:a38c configured:2288
>>>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using 
>>>> default
>>>> Flow rule #0 created
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>>>
>>>>
>>>
>>> So are you happy with this solution?
>>> .
>> Yes, thanks.
> 
> cool, will you able to send a new version? Thanks.
> .
Yes
> 

^ permalink raw reply	[flat|nested] 44+ messages in thread

* [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
@ 2020-10-15 12:30 Lijun Ou
  0 siblings, 0 replies; 44+ messages in thread
From: Lijun Ou @ 2020-10-15 12:30 UTC (permalink / raw)
  To: wenzhuo.lu, beilei.xing, adrien.mazarguil, ferruh.yigit; +Cc: dev, linuxarm

When start the testpmd, the pmd driver initializes the RSS
configuration. Generally, the recommended RSS hash key is
used as the default key in the driver. In addition, the
default key is different from the default RSS flow in testpmd
without specifying RSS hash key. So. if you do not specify
the RSS key when creating an RSS rule, the testpmd uses the
default key as the default RSS key of the RSS rule. As a result,
you may mistakenly consider that the RSS key in use is the valid
default key of the NIC, actually, the key and the valid default
key of the NIC are two values.

Consider the follow usage with testpmd:
1. first, startup testpmd:
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
RSS key:
-6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F
-20C6A42B73BBEAC01FA
2. create a rss rule
testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end actions rss \
types ipv4-udp end queues end / end

3. show rss-hash key
testpmd> show port 0 rss-hash key
RSS functions:
  all ipv4-udp udp
RSS key:
-74657374706D6427732064656661756C74205253532068617368206B65792C206F
-76657272696465

In order to solve the above problems, it use the NIC valid default
RSS key instead of the testpmd dummy RSS key in the flow configuration
when the RSS key is not specified in the flow rule. If the NIC RSS key
is invalid, it will use testpmd dummy RSS key as the default key.

Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API")
Cc: stable@dpdk.org

Signed-off-by: Lijun Ou <oulijun@huawei.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
V4->V5:
-rewrite the commit log
-add reviewed-by

V3->V4:
-fix checkpatch warning and shorter commit content.

V2->V3:
-fix checkpatch warning.

V1->V2:
-fix the commit.
---
 app/test-pmd/cmdline_flow.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 84bba0f..578555e 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -4735,6 +4735,7 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->queue[i] = i;
 	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
 	    ctx->port != (portid_t)RTE_PORT_ALL) {
+		struct rte_eth_rss_conf rss_conf = {0};
 		struct rte_eth_dev_info info;
 		int ret2;
 
@@ -4745,6 +4746,13 @@ parse_vc_action_rss(struct context *ctx, const struct token *token,
 		action_rss_data->conf.key_len =
 			RTE_MIN(sizeof(action_rss_data->key),
 				info.hash_key_size);
+
+		rss_conf.rss_key_len = sizeof(action_rss_data->key);
+		rss_conf.rss_key = action_rss_data->key;
+		ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf);
+		if (ret2 != 0)
+			return ret2;
+		action_rss_data->conf.key = rss_conf.rss_key;
 	}
 	action->conf = &action_rss_data->conf;
 	return ret;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 44+ messages in thread

end of thread, back to index

Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10  1:51 [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration Lijun Ou
2020-09-22  9:51 ` Phil Yang
2020-09-22 13:50   ` oulijun
2020-09-22 15:44     ` Phil Yang
2020-09-24 13:45 ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Lijun Ou
2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
2020-09-29 15:35     ` Phil Yang
2020-09-30 12:57     ` Ferruh Yigit
2020-10-09 11:55       ` oulijun
2020-10-09 18:27         ` Ferruh Yigit
2020-10-09 18:54           ` Ferruh Yigit
2020-09-30 13:36     ` Ferruh Yigit
2020-10-09 11:59       ` oulijun
2020-10-09 18:36         ` Ferruh Yigit
2020-10-15 12:41     ` [dpdk-dev] [PATCH v5] " Lijun Ou
2020-10-15 13:52       ` Ferruh Yigit
2020-10-15 14:04         ` oulijun
2020-10-15 14:43           ` Ferruh Yigit
2020-10-15 16:05             ` Ferruh Yigit
2020-10-15 23:21               ` Ophir Munk
2020-10-15 23:53                 ` Ferruh Yigit
2020-10-16  0:14                   ` Ajit Khaparde
2020-10-16  6:46                   ` Ophir Munk
2020-10-16 10:05                     ` oulijun
2020-10-16 15:13                       ` Ophir Munk
2020-10-16 10:04                   ` oulijun
2020-10-16 10:57                     ` Ferruh Yigit
2020-10-16 14:59                       ` Ophir Munk
2020-10-20  9:00                       ` oulijun
2020-10-20 10:02                         ` Ferruh Yigit
2020-10-20 13:35                           ` oulijun
2020-10-20 14:34                             ` Ferruh Yigit
2020-10-21  8:19                               ` oulijun
2020-10-21  9:38                                 ` Ferruh Yigit
2020-10-21 10:07                                   ` oulijun
     [not found]       ` <20201015195637.26476-1-robot@bytheb.org>
2020-10-16  9:39         ` [dpdk-dev] |WARNING| pw80899 " oulijun
2020-10-16  9:41           ` David Marchand
2020-09-30 13:17   ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Ferruh Yigit
2020-10-09 12:09     ` oulijun
2020-10-09 18:52       ` Ferruh Yigit
2020-10-10  3:07         ` Phil Yang
2020-10-14  6:15         ` oulijun
2020-10-14  8:41           ` Ferruh Yigit
2020-10-15 12:30 [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration Lijun Ou

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox