[2/3] app/testpmd: fix RSS hash type update

  port_rss_hash_key_update() initializes rss_conf with the
RSS hash type and key provided by the user, but it calls
rte_eth_dev_rss_hash_conf_get() before calling
rte_eth_dev_rss_hash_update(), which overides the parsed
config with current NIC's config.

While the RSS key value is set again after, this is not
the case of the key length and the type of hash.

There is no need to read the RSS config from the NIC, let's
just try to set the user defined one.

Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
Cc: stable@dpdk.org
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 app/test-pmd/config.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
  

On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> port_rss_hash_key_update() initializes rss_conf with the
> RSS hash type and key provided by the user, but it calls
> rte_eth_dev_rss_hash_conf_get() before calling
> rte_eth_dev_rss_hash_update(), which overides the parsed
> config with current NIC's config.
> 
> While the RSS key value is set again after, this is not
> the case of the key length and the type of hash.
> 
> There is no need to read the RSS config from the NIC, let's
> just try to set the user defined one.
> 
> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> Cc: stable@dpdk.org
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  app/test-pmd/config.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 31d8ba1b91..451bda53b1 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>  	int diag;
>  	unsigned int i;
>  
> -	rss_conf.rss_key = NULL;
> +	rss_conf.rss_key = hash_key;
>  	rss_conf.rss_key_len = hash_key_len;
>  	rss_conf.rss_hf = 0;
>  	for (i = 0; rss_type_table[i].str; i++) {
>  		if (!strcmp(rss_type_table[i].str, rss_type))
>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>  	}
> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> -	if (diag == 0) {
> -		rss_conf.rss_key = hash_key;
> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> -	}
> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);

I'm not 100% sure, but I'd say the intent above could be
to update key only as the function name says. I.e. keep
rss_hf as is. That could be the reason to get first.
  

On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>> port_rss_hash_key_update() initializes rss_conf with the
>> RSS hash type and key provided by the user, but it calls
>> rte_eth_dev_rss_hash_conf_get() before calling
>> rte_eth_dev_rss_hash_update(), which overides the parsed
>> config with current NIC's config.
>>
>> While the RSS key value is set again after, this is not
>> the case of the key length and the type of hash.
>>
>> There is no need to read the RSS config from the NIC, let's
>> just try to set the user defined one.
>>
>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
>> Cc: stable@dpdk.org
>> Cc: nelio.laranjeiro@6wind.com
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  app/test-pmd/config.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 31d8ba1b91..451bda53b1 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>>  	int diag;
>>  	unsigned int i;
>>  
>> -	rss_conf.rss_key = NULL;
>> +	rss_conf.rss_key = hash_key;
>>  	rss_conf.rss_key_len = hash_key_len;
>>  	rss_conf.rss_hf = 0;
>>  	for (i = 0; rss_type_table[i].str; i++) {
>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>  	}
>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>> -	if (diag == 0) {
>> -		rss_conf.rss_key = hash_key;
>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>> -	}
>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> 
> I'm not 100% sure, but I'd say the intent above could be
> to update key only as the function name says. I.e. keep
> rss_hf as is. That could be the reason to get first.
> 

I think that was the intial purpose of the command, but patch
8205e241b2b0 added setting the hash type as mandatory. There are
no other command to configure the hash type from testpmd AFAICT.

Also, even without 8205e241b2b0, the function was broken because the
key length was overiden.

Regards,
Maxime
  

On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> 
> 
> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>> port_rss_hash_key_update() initializes rss_conf with the
>>> RSS hash type and key provided by the user, but it calls
>>> rte_eth_dev_rss_hash_conf_get() before calling
>>> rte_eth_dev_rss_hash_update(), which overides the parsed
>>> config with current NIC's config.
>>>
>>> While the RSS key value is set again after, this is not
>>> the case of the key length and the type of hash.
>>>
>>> There is no need to read the RSS config from the NIC, let's
>>> just try to set the user defined one.
>>>
>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
>>> Cc: stable@dpdk.org
>>> Cc: nelio.laranjeiro@6wind.com
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>  app/test-pmd/config.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 31d8ba1b91..451bda53b1 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>>>  	int diag;
>>>  	unsigned int i;
>>>  
>>> -	rss_conf.rss_key = NULL;
>>> +	rss_conf.rss_key = hash_key;
>>>  	rss_conf.rss_key_len = hash_key_len;
>>>  	rss_conf.rss_hf = 0;
>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>  	}
>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>> -	if (diag == 0) {
>>> -		rss_conf.rss_key = hash_key;
>>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>> -	}
>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>
>> I'm not 100% sure, but I'd say the intent above could be
>> to update key only as the function name says. I.e. keep
>> rss_hf as is. That could be the reason to get first.
>>
> 
> I think that was the intial purpose of the command, but patch
> 8205e241b2b0 added setting the hash type as mandatory. There are
> no other command to configure the hash type from testpmd AFAICT.
> 
> Also, even without 8205e241b2b0, the function was broken because the
> key length was overiden.

I see, many thanks for explanations.
  

On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> > 
> > 
> > On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>> port_rss_hash_key_update() initializes rss_conf with the
> >>> RSS hash type and key provided by the user, but it calls
> >>> rte_eth_dev_rss_hash_conf_get() before calling
> >>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>> config with current NIC's config.
> >>>
> >>> While the RSS key value is set again after, this is not
> >>> the case of the key length and the type of hash.
> >>>
> >>> There is no need to read the RSS config from the NIC, let's
> >>> just try to set the user defined one.
> >>>
> >>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> >>> Cc: stable@dpdk.org
> >>> Cc: nelio.laranjeiro@6wind.com
> >>>
> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> ---
> >>>  app/test-pmd/config.c | 8 ++------
> >>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>> index 31d8ba1b91..451bda53b1 100644
> >>> --- a/app/test-pmd/config.c
> >>> +++ b/app/test-pmd/config.c
> >>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
> >>>  	int diag;
> >>>  	unsigned int i;
> >>>  
> >>> -	rss_conf.rss_key = NULL;
> >>> +	rss_conf.rss_key = hash_key;
> >>>  	rss_conf.rss_key_len = hash_key_len;
> >>>  	rss_conf.rss_hf = 0;
> >>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>  	}
> >>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>> -	if (diag == 0) {
> >>> -		rss_conf.rss_key = hash_key;
> >>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>> -	}
> >>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>
> >> I'm not 100% sure, but I'd say the intent above could be
> >> to update key only as the function name says. I.e. keep
> >> rss_hf as is. That could be the reason to get first.

True,

> > I think that was the intial purpose of the command, but patch
> > 8205e241b2b0 added setting the hash type as mandatory. There are
> > no other command to configure the hash type from testpmd AFAICT.

Also for the same initial purpose, some NIC have an hash key per
protocol, by default it uses the same key for all of them but it can be
configured individually making for example key0 for all protocols expect
IPv4 which uses key1.

> > Also, even without 8205e241b2b0, the function was broken because the
> > key length was overiden.
> 
> I see, many thanks for explanations.
  

Hi Nélio,

On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>> port_rss_hash_key_update() initializes rss_conf with the
>>>>> RSS hash type and key provided by the user, but it calls
>>>>> rte_eth_dev_rss_hash_conf_get() before calling
>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
>>>>> config with current NIC's config.
>>>>>
>>>>> While the RSS key value is set again after, this is not
>>>>> the case of the key length and the type of hash.
>>>>>
>>>>> There is no need to read the RSS config from the NIC, let's
>>>>> just try to set the user defined one.
>>>>>
>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
>>>>> Cc: stable@dpdk.org
>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>>
>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> ---
>>>>>  app/test-pmd/config.c | 8 ++------
>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>>> index 31d8ba1b91..451bda53b1 100644
>>>>> --- a/app/test-pmd/config.c
>>>>> +++ b/app/test-pmd/config.c
>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
>>>>>  	int diag;
>>>>>  	unsigned int i;
>>>>>  
>>>>> -	rss_conf.rss_key = NULL;
>>>>> +	rss_conf.rss_key = hash_key;
>>>>>  	rss_conf.rss_key_len = hash_key_len;
>>>>>  	rss_conf.rss_hf = 0;
>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>>  	}
>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>> -	if (diag == 0) {
>>>>> -		rss_conf.rss_key = hash_key;
>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>> -	}
>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>
>>>> I'm not 100% sure, but I'd say the intent above could be
>>>> to update key only as the function name says. I.e. keep
>>>> rss_hf as is. That could be the reason to get first.
> 
> True,
> 
>>> I think that was the intial purpose of the command, but patch
>>> 8205e241b2b0 added setting the hash type as mandatory. There are
>>> no other command to configure the hash type from testpmd AFAICT.
> 
> Also for the same initial purpose, some NIC have an hash key per
> protocol, by default it uses the same key for all of them but it can be
> configured individually making for example key0 for all protocols expect
> IPv4 which uses key1.

Thanks for the info, I have looked at most drivers but didn't found one
that support this feature, could you give some pointer?

Given how the drivers implément the callback, do you agree with the fix,
or do you have something else in mind?

Thanks,
Maxime

>>> Also, even without 8205e241b2b0, the function was broken because the
>>> key length was overiden.
>>
>> I see, many thanks for explanations.
>
  

+Shahaf,

Hi Maxime,

On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> Hi Nélio,
> 
> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> > On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>
> >>>
> >>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>> port_rss_hash_key_update() initializes rss_conf with the
> >>>>> RSS hash type and key provided by the user, but it calls
> >>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>>>> config with current NIC's config.
> >>>>>
> >>>>> While the RSS key value is set again after, this is not
> >>>>> the case of the key length and the type of hash.
> >>>>>
> >>>>> There is no need to read the RSS config from the NIC, let's
> >>>>> just try to set the user defined one.
> >>>>>
> >>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash commands")
> >>>>> Cc: stable@dpdk.org
> >>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>>
> >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>> ---
> >>>>>  app/test-pmd/config.c | 8 ++------
> >>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>> index 31d8ba1b91..451bda53b1 100644
> >>>>> --- a/app/test-pmd/config.c
> >>>>> +++ b/app/test-pmd/config.c
> >>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t port_id, char rss_type[], uint8_t *hash_key,
> >>>>>  	int diag;
> >>>>>  	unsigned int i;
> >>>>>  
> >>>>> -	rss_conf.rss_key = NULL;
> >>>>> +	rss_conf.rss_key = hash_key;
> >>>>>  	rss_conf.rss_key_len = hash_key_len;
> >>>>>  	rss_conf.rss_hf = 0;
> >>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>>  	}
> >>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>> -	if (diag == 0) {
> >>>>> -		rss_conf.rss_key = hash_key;
> >>>>> -		diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>> -	}
> >>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>
> >>>> I'm not 100% sure, but I'd say the intent above could be
> >>>> to update key only as the function name says. I.e. keep
> >>>> rss_hf as is. That could be the reason to get first.
> > 
> > True,
> > 
> >>> I think that was the intial purpose of the command, but patch
> >>> 8205e241b2b0 added setting the hash type as mandatory. There are
> >>> no other command to configure the hash type from testpmd AFAICT.
> > 
> > Also for the same initial purpose, some NIC have an hash key per
> > protocol, by default it uses the same key for all of them but it can be
> > configured individually making for example key0 for all protocols expect
> > IPv4 which uses key1.
> 
> Thanks for the info, I have looked at most drivers but didn't found one
> that support this feature, could you give some pointer?

Well, I have done the modification at that time for MLX5 PMD, since I
left DPDK in 2018 I don't know if they still support such configuration
from this API or if they fully moved to rte_flow.

> Given how the drivers implément the callback, do you agree with the fix,
> or do you have something else in mind?

I cannot answer if this get() is mandatory, this predates my arrival on
DPDK (original commit written in 2014), looking at DPDK state on 
 commit f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
Maybe someone from Intel can help, eventually you can contact PMD
maintainers to review this patch.

Regards,
Nélio

> Thanks,
> Maxime
> 
> >>> Also, even without 8205e241b2b0, the function was broken because the
> >>> key length was overiden.
> >>
> >> I see, many thanks for explanations.
> > 
>
  

Hi

> -----Original Message-----
> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
> Sent: Tuesday, September 14, 2021 15:20
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
> 
> +Shahaf,
> 
> Hi Maxime,
> 
> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> > Hi Nélio,
> >
> > On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> > > On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> > >> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> > >>>
> > >>>
> > >>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> > >>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> > >>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
> > >>>>> hash type and key provided by the user, but it calls
> > >>>>> rte_eth_dev_rss_hash_conf_get() before calling
> > >>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
> > >>>>> with current NIC's config.
> > >>>>>
> > >>>>> While the RSS key value is set again after, this is not the case
> > >>>>> of the key length and the type of hash.
> > >>>>>
> > >>>>> There is no need to read the RSS config from the NIC, let's just
> > >>>>> try to set the user defined one.
> > >>>>>
> > >>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
> > >>>>> commands")
> > >>>>> Cc: stable@dpdk.org
> > >>>>> Cc: nelio.laranjeiro@6wind.com
> > >>>>>
> > >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >>>>> ---
> > >>>>>  app/test-pmd/config.c | 8 ++------
> > >>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> > >>>>>
> > >>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > >>>>> 31d8ba1b91..451bda53b1 100644
> > >>>>> --- a/app/test-pmd/config.c
> > >>>>> +++ b/app/test-pmd/config.c
> > >>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
> port_id, char rss_type[], uint8_t *hash_key,
> > >>>>>  	int diag;
> > >>>>>  	unsigned int i;
> > >>>>>
> > >>>>> -	rss_conf.rss_key = NULL;
> > >>>>> +	rss_conf.rss_key = hash_key;
> > >>>>>  	rss_conf.rss_key_len = hash_key_len;
> > >>>>>  	rss_conf.rss_hf = 0;
> > >>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> > >>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> > >>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> > >>>>>  	}
> > >>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> > >>>>> -	if (diag == 0) {
> > >>>>> -		rss_conf.rss_key = hash_key;
> > >>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
> &rss_conf);
> > >>>>> -	}
> > >>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> > >>>>
> > >>>> I'm not 100% sure, but I'd say the intent above could be to
> > >>>> update key only as the function name says. I.e. keep rss_hf as
> > >>>> is. That could be the reason to get first.

+1
The intent is only updating rss key. That's why to get_rss_conf first.
At least for all intel devices. RSS key is a global value for all rss_hf.

And since the intent is to only update the key value. I don't think this patch makes sense since you're changing rss_hf which will break current test cases.
And before 8205e241b2b01c, the command is what we want. port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates key.

But if mlx has the configuration of config key for each rss_type then the code should remain to the current code in which keylen and rss_hf will be overridden to the correct value intel wants and mlx has their own configuration for specific rss type.
But to be honest, I checked mlx5. I don't see they have this kind of configuration. Need to confirm with their driver maintainer though.

But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0 does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have key update for specific rss type. Otherwise, remain what it's like wight now.

BRs
Xiaoyun

> > >
> > > True,
> > >
> > >>> I think that was the intial purpose of the command, but patch
> > >>> 8205e241b2b0 added setting the hash type as mandatory. There are
> > >>> no other command to configure the hash type from testpmd AFAICT.
> > >
> > > Also for the same initial purpose, some NIC have an hash key per
> > > protocol, by default it uses the same key for all of them but it can
> > > be configured individually making for example key0 for all protocols
> > > expect
> > > IPv4 which uses key1.
> >
> > Thanks for the info, I have looked at most drivers but didn't found
> > one that support this feature, could you give some pointer?
> 
> Well, I have done the modification at that time for MLX5 PMD, since I left DPDK
> in 2018 I don't know if they still support such configuration from this API or if
> they fully moved to rte_flow.
> 
> > Given how the drivers implément the callback, do you agree with the
> > fix, or do you have something else in mind?
> 
> I cannot answer if this get() is mandatory, this predates my arrival on DPDK
> (original commit written in 2014), looking at DPDK state on  commit
> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
> Maybe someone from Intel can help, eventually you can contact PMD
> maintainers to review this patch.
> 
> Regards,
> Nélio
> 
> > Thanks,
> > Maxime
> >
> > >>> Also, even without 8205e241b2b0, the function was broken because
> > >>> the key length was overiden.
> > >>
> > >> I see, many thanks for explanations.
> > >
> >
> 
> --
> Nélio Laranjeiro
> 6WIND
  

Hi Xiaoyun,

On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
>> Sent: Tuesday, September 14, 2021 15:20
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>
>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
>> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
>> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
>>
>> +Shahaf,
>>
>> Hi Maxime,
>>
>> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
>>> Hi Nélio,
>>>
>>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
>>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
>>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
>>>>>>
>>>>>>
>>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
>>>>>>>> hash type and key provided by the user, but it calls
>>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
>>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
>>>>>>>> with current NIC's config.
>>>>>>>>
>>>>>>>> While the RSS key value is set again after, this is not the case
>>>>>>>> of the key length and the type of hash.
>>>>>>>>
>>>>>>>> There is no need to read the RSS config from the NIC, let's just
>>>>>>>> try to set the user defined one.
>>>>>>>>
>>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
>>>>>>>> commands")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>>>>>
>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>> ---
>>>>>>>>  app/test-pmd/config.c | 8 ++------
>>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>>>>> 31d8ba1b91..451bda53b1 100644
>>>>>>>> --- a/app/test-pmd/config.c
>>>>>>>> +++ b/app/test-pmd/config.c
>>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
>> port_id, char rss_type[], uint8_t *hash_key,
>>>>>>>>  	int diag;
>>>>>>>>  	unsigned int i;
>>>>>>>>
>>>>>>>> -	rss_conf.rss_key = NULL;
>>>>>>>> +	rss_conf.rss_key = hash_key;
>>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
>>>>>>>>  	rss_conf.rss_hf = 0;
>>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>>>>>  	}
>>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>>>>> -	if (diag == 0) {
>>>>>>>> -		rss_conf.rss_key = hash_key;
>>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
>> &rss_conf);
>>>>>>>> -	}
>>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>>>>
>>>>>>> I'm not 100% sure, but I'd say the intent above could be to
>>>>>>> update key only as the function name says. I.e. keep rss_hf as
>>>>>>> is. That could be the reason to get first.
> 
> +1
> The intent is only updating rss key. That's why to get_rss_conf first.
> At least for all intel devices. RSS key is a global value for all rss_hf.
> 
> And since the intent is to only update the key value. I don't think this patch makes sense since you're changing rss_hf which will break current test cases.
> And before 8205e241b2b01c, the command is what we want. port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates key.
> 
> But if mlx has the configuration of config key for each rss_type then the code should remain to the current code in which keylen and rss_hf will be overridden to the correct value intel wants and mlx has their own configuration for specific rss type.
> But to be honest, I checked mlx5. I don't see they have this kind of configuration. Need to confirm with their driver maintainer though.
> 
> But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0 does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have key update for specific rss type. Otherwise, remain what it's like wight now.

For the main branch, we could revert 8205e241b2b01c0, but in my opinion,
we need to keep the hash_key_length otherwise it could lead to out of
bounds accesses if the key passed by the user is shorter than the key
length in use by the driver.

Note that it would also imply changes in the DTS and CIs tests that make
use of this command. We would also need to introduce a new command to be
able to set the rss hash types, and rename the existing one to be key-
specific. Otherwise we have no way with testpmd to configure RSS
properly. Given there does not seem to have any driver that leverages
RSS HF/Key pairs, maybe the simple thing is to just do what I did in
this patch.

For stable, I don't think this is a good idea to change testpmd
commands, as it could break users setups.

Thanks,
Maxime

> 
> BRs
> Xiaoyun
> 
>>>>
>>>> True,
>>>>
>>>>>> I think that was the intial purpose of the command, but patch
>>>>>> 8205e241b2b0 added setting the hash type as mandatory. There are
>>>>>> no other command to configure the hash type from testpmd AFAICT.
>>>>
>>>> Also for the same initial purpose, some NIC have an hash key per
>>>> protocol, by default it uses the same key for all of them but it can
>>>> be configured individually making for example key0 for all protocols
>>>> expect
>>>> IPv4 which uses key1.
>>>
>>> Thanks for the info, I have looked at most drivers but didn't found
>>> one that support this feature, could you give some pointer?
>>
>> Well, I have done the modification at that time for MLX5 PMD, since I left DPDK
>> in 2018 I don't know if they still support such configuration from this API or if
>> they fully moved to rte_flow.
>>
>>> Given how the drivers implément the callback, do you agree with the
>>> fix, or do you have something else in mind?
>>
>> I cannot answer if this get() is mandatory, this predates my arrival on DPDK
>> (original commit written in 2014), looking at DPDK state on  commit
>> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
>> Maybe someone from Intel can help, eventually you can contact PMD
>> maintainers to review this patch.
>>
>> Regards,
>> Nélio
>>
>>> Thanks,
>>> Maxime
>>>
>>>>>> Also, even without 8205e241b2b0, the function was broken because
>>>>>> the key length was overiden.
>>>>>
>>>>> I see, many thanks for explanations.
>>>>
>>>
>>
>> --
>> Nélio Laranjeiro
>> 6WIND
>
  

Hi

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, September 16, 2021 15:34
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
> 
> Hi Xiaoyun,
> 
> On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
> >> Sent: Tuesday, September 14, 2021 15:20
> >> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> >> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> >> david.marchand@redhat.com; michaelba@nvidia.com;
> >> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
> >> <shahafs@nvidia.com>
> >> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type
> >> update
> >>
> >> +Shahaf,
> >>
> >> Hi Maxime,
> >>
> >> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> >>> Hi Nélio,
> >>>
> >>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> >>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
> >>>>>>>> hash type and key provided by the user, but it calls
> >>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
> >>>>>>>> with current NIC's config.
> >>>>>>>>
> >>>>>>>> While the RSS key value is set again after, this is not the
> >>>>>>>> case of the key length and the type of hash.
> >>>>>>>>
> >>>>>>>> There is no need to read the RSS config from the NIC, let's
> >>>>>>>> just try to set the user defined one.
> >>>>>>>>
> >>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
> >>>>>>>> commands")
> >>>>>>>> Cc: stable@dpdk.org
> >>>>>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>>>>>
> >>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>>>> ---
> >>>>>>>>  app/test-pmd/config.c | 8 ++------
> >>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>>>>> index
> >>>>>>>> 31d8ba1b91..451bda53b1 100644
> >>>>>>>> --- a/app/test-pmd/config.c
> >>>>>>>> +++ b/app/test-pmd/config.c
> >>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
> >> port_id, char rss_type[], uint8_t *hash_key,
> >>>>>>>>  	int diag;
> >>>>>>>>  	unsigned int i;
> >>>>>>>>
> >>>>>>>> -	rss_conf.rss_key = NULL;
> >>>>>>>> +	rss_conf.rss_key = hash_key;
> >>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
> >>>>>>>>  	rss_conf.rss_hf = 0;
> >>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>>>>>  	}
> >>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>>>>> -	if (diag == 0) {
> >>>>>>>> -		rss_conf.rss_key = hash_key;
> >>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
> >> &rss_conf);
> >>>>>>>> -	}
> >>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>>>>
> >>>>>>> I'm not 100% sure, but I'd say the intent above could be to
> >>>>>>> update key only as the function name says. I.e. keep rss_hf as
> >>>>>>> is. That could be the reason to get first.
> >
> > +1
> > The intent is only updating rss key. That's why to get_rss_conf first.
> > At least for all intel devices. RSS key is a global value for all rss_hf.
> >
> > And since the intent is to only update the key value. I don't think this patch
> makes sense since you're changing rss_hf which will break current test cases.
> > And before 8205e241b2b01c, the command is what we want.
> port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates
> key.
> >
> > But if mlx has the configuration of config key for each rss_type then the code
> should remain to the current code in which keylen and rss_hf will be overridden
> to the correct value intel wants and mlx has their own configuration for specific
> rss type.
> > But to be honest, I checked mlx5. I don't see they have this kind of
> configuration. Need to confirm with their driver maintainer though.
> >
> > But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0
> does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have
> key update for specific rss type. Otherwise, remain what it's like wight now.
> 
> For the main branch, we could revert 8205e241b2b01c0, but in my opinion, we
> need to keep the hash_key_length otherwise it could lead to out of bounds
> accesses if the key passed by the user is shorter than the key length in use by the
> driver.
> 
> Note that it would also imply changes in the DTS and CIs tests that make use of
> this command. We would also need to introduce a new command to be able to
> set the rss hash types, and rename the existing one to be key- specific.
> Otherwise we have no way with testpmd to configure RSS properly. Given there
> does not seem to have any driver that leverages RSS HF/Key pairs, maybe the
> simple thing is to just do what I did in this patch.
> 
> For stable, I don't think this is a good idea to change testpmd commands, as it
> could break users setups.

I don’t think so. This command is used for key setting only like the name says. And users use this command only setting key. What you did in this patch actually will break test cases results.

And change hash type you can use command "port config all rss eth|vlan|tcp|...".

> 
> Thanks,
> Maxime
> 
> >
> > BRs
> > Xiaoyun
> >
> >>>>
> >>>> True,
> >>>>
> >>>>>> I think that was the intial purpose of the command, but patch
> >>>>>> 8205e241b2b0 added setting the hash type as mandatory. There are
> >>>>>> no other command to configure the hash type from testpmd AFAICT.
> >>>>
> >>>> Also for the same initial purpose, some NIC have an hash key per
> >>>> protocol, by default it uses the same key for all of them but it
> >>>> can be configured individually making for example key0 for all
> >>>> protocols expect
> >>>> IPv4 which uses key1.
> >>>
> >>> Thanks for the info, I have looked at most drivers but didn't found
> >>> one that support this feature, could you give some pointer?
> >>
> >> Well, I have done the modification at that time for MLX5 PMD, since I
> >> left DPDK in 2018 I don't know if they still support such
> >> configuration from this API or if they fully moved to rte_flow.
> >>
> >>> Given how the drivers implément the callback, do you agree with the
> >>> fix, or do you have something else in mind?
> >>
> >> I cannot answer if this get() is mandatory, this predates my arrival
> >> on DPDK (original commit written in 2014), looking at DPDK state on
> >> commit
> >> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
> >> Maybe someone from Intel can help, eventually you can contact PMD
> >> maintainers to review this patch.
> >>
> >> Regards,
> >> Nélio
> >>
> >>> Thanks,
> >>> Maxime
> >>>
> >>>>>> Also, even without 8205e241b2b0, the function was broken because
> >>>>>> the key length was overiden.
> >>>>>
> >>>>> I see, many thanks for explanations.
> >>>>
> >>>
> >>
> >> --
> >> Nélio Laranjeiro
> >> 6WIND
> >
  

On 9/16/21 9:41 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, September 16, 2021 15:34
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
>> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
>> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
>> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
>>
>> Hi Xiaoyun,
>>
>> On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio Laranjeiro
>>>> Sent: Tuesday, September 14, 2021 15:20
>>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
>>>> <ferruh.yigit@intel.com>
>>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
>>>> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
>>>> david.marchand@redhat.com; michaelba@nvidia.com;
>>>> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
>>>> <shahafs@nvidia.com>
>>>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type
>>>> update
>>>>
>>>> +Shahaf,
>>>>
>>>> Hi Maxime,
>>>>
>>>> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
>>>>> Hi Nélio,
>>>>>
>>>>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
>>>>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
>>>>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
>>>>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
>>>>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
>>>>>>>>>> hash type and key provided by the user, but it calls
>>>>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
>>>>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed config
>>>>>>>>>> with current NIC's config.
>>>>>>>>>>
>>>>>>>>>> While the RSS key value is set again after, this is not the
>>>>>>>>>> case of the key length and the type of hash.
>>>>>>>>>>
>>>>>>>>>> There is no need to read the RSS config from the NIC, let's
>>>>>>>>>> just try to set the user defined one.
>>>>>>>>>>
>>>>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS hash
>>>>>>>>>> commands")
>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>> Cc: nelio.laranjeiro@6wind.com
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  app/test-pmd/config.c | 8 ++------
>>>>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>>>>>>>> index
>>>>>>>>>> 31d8ba1b91..451bda53b1 100644
>>>>>>>>>> --- a/app/test-pmd/config.c
>>>>>>>>>> +++ b/app/test-pmd/config.c
>>>>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
>>>> port_id, char rss_type[], uint8_t *hash_key,
>>>>>>>>>>  	int diag;
>>>>>>>>>>  	unsigned int i;
>>>>>>>>>>
>>>>>>>>>> -	rss_conf.rss_key = NULL;
>>>>>>>>>> +	rss_conf.rss_key = hash_key;
>>>>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
>>>>>>>>>>  	rss_conf.rss_hf = 0;
>>>>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
>>>>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
>>>>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
>>>>>>>>>>  	}
>>>>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
>>>>>>>>>> -	if (diag == 0) {
>>>>>>>>>> -		rss_conf.rss_key = hash_key;
>>>>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
>>>> &rss_conf);
>>>>>>>>>> -	}
>>>>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
>>>>>>>>>
>>>>>>>>> I'm not 100% sure, but I'd say the intent above could be to
>>>>>>>>> update key only as the function name says. I.e. keep rss_hf as
>>>>>>>>> is. That could be the reason to get first.
>>>
>>> +1
>>> The intent is only updating rss key. That's why to get_rss_conf first.
>>> At least for all intel devices. RSS key is a global value for all rss_hf.
>>>
>>> And since the intent is to only update the key value. I don't think this patch
>> makes sense since you're changing rss_hf which will break current test cases.
>>> And before 8205e241b2b01c, the command is what we want.
>> port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only updates
>> key.
>>>
>>> But if mlx has the configuration of config key for each rss_type then the code
>> should remain to the current code in which keylen and rss_hf will be overridden
>> to the correct value intel wants and mlx has their own configuration for specific
>> rss type.
>>> But to be honest, I checked mlx5. I don't see they have this kind of
>> configuration. Need to confirm with their driver maintainer though.
>>>
>>> But anyway, from my point of view, I prefer to revert what 8205e241b2b01c0
>> does and remove rss_hf, rss_key_len setting in this command if mlx doesn't have
>> key update for specific rss type. Otherwise, remain what it's like wight now.
>>
>> For the main branch, we could revert 8205e241b2b01c0, but in my opinion, we
>> need to keep the hash_key_length otherwise it could lead to out of bounds
>> accesses if the key passed by the user is shorter than the key length in use by the
>> driver.
>>
>> Note that it would also imply changes in the DTS and CIs tests that make use of
>> this command. We would also need to introduce a new command to be able to
>> set the rss hash types, and rename the existing one to be key- specific.
>> Otherwise we have no way with testpmd to configure RSS properly. Given there
>> does not seem to have any driver that leverages RSS HF/Key pairs, maybe the
>> simple thing is to just do what I did in this patch.
>>
>> For stable, I don't think this is a good idea to change testpmd commands, as it
>> could break users setups.
> 
> I don’t think so. This command is used for key setting only like the name says. And users use this command only setting key. What you did in this patch actually will break test cases results.
> 
> And change hash type you can use command "port config all rss eth|vlan|tcp|...".

Ok, I did not find this command as I was trying completion with 'port
config 0 rss'. It might be good to to change it so that we can configure
it per port, but not a big deal.

Remains the key length that should be passed, otherwise can have out of
bounds accesses, no? And we still need to update DTS if we revert
8205e241b2b01c0.

Thanks,
Maxime

> 
>>
>> Thanks,
>> Maxime
>>
>>>
>>> BRs
>>> Xiaoyun
>>>
>>>>>>
>>>>>> True,
>>>>>>
>>>>>>>> I think that was the intial purpose of the command, but patch
>>>>>>>> 8205e241b2b0 added setting the hash type as mandatory. There are
>>>>>>>> no other command to configure the hash type from testpmd AFAICT.
>>>>>>
>>>>>> Also for the same initial purpose, some NIC have an hash key per
>>>>>> protocol, by default it uses the same key for all of them but it
>>>>>> can be configured individually making for example key0 for all
>>>>>> protocols expect
>>>>>> IPv4 which uses key1.
>>>>>
>>>>> Thanks for the info, I have looked at most drivers but didn't found
>>>>> one that support this feature, could you give some pointer?
>>>>
>>>> Well, I have done the modification at that time for MLX5 PMD, since I
>>>> left DPDK in 2018 I don't know if they still support such
>>>> configuration from this API or if they fully moved to rte_flow.
>>>>
>>>>> Given how the drivers implément the callback, do you agree with the
>>>>> fix, or do you have something else in mind?
>>>>
>>>> I cannot answer if this get() is mandatory, this predates my arrival
>>>> on DPDK (original commit written in 2014), looking at DPDK state on
>>>> commit
>>>> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
>>>> Maybe someone from Intel can help, eventually you can contact PMD
>>>> maintainers to review this patch.
>>>>
>>>> Regards,
>>>> Nélio
>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>>
>>>>>>>> Also, even without 8205e241b2b0, the function was broken because
>>>>>>>> the key length was overiden.
>>>>>>>
>>>>>>> I see, many thanks for explanations.
>>>>>>
>>>>>
>>>>
>>>> --
>>>> Nélio Laranjeiro
>>>> 6WIND
>>>
>
  

Hi

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, September 16, 2021 16:09
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> david.marchand@redhat.com; michaelba@nvidia.com; viacheslavo@nvidia.com;
> stable@dpdk.org; Shahaf Shuler <shahafs@nvidia.com>
> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update
> 
> 
> 
> On 9/16/21 9:41 AM, Li, Xiaoyun wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Thursday, September 16, 2021 15:34
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Nélio Laranjeiro
> >> <nelio.laranjeiro@6wind.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> >> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> >> david.marchand@redhat.com; michaelba@nvidia.com;
> >> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
> >> <shahafs@nvidia.com>
> >> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type
> >> update
> >>
> >> Hi Xiaoyun,
> >>
> >> On 9/16/21 5:03 AM, Li, Xiaoyun wrote:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: stable <stable-bounces@dpdk.org> On Behalf Of Nélio
> >>>> Laranjeiro
> >>>> Sent: Tuesday, September 14, 2021 15:20
> >>>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Yigit, Ferruh
> >>>> <ferruh.yigit@intel.com>
> >>>> Cc: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org;
> >>>> Xia, Chenbo <chenbo.xia@intel.com>; amorenoz@redhat.com;
> >>>> david.marchand@redhat.com; michaelba@nvidia.com;
> >>>> viacheslavo@nvidia.com; stable@dpdk.org; Shahaf Shuler
> >>>> <shahafs@nvidia.com>
> >>>> Subject: Re: [dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash
> >>>> type update
> >>>>
> >>>> +Shahaf,
> >>>>
> >>>> Hi Maxime,
> >>>>
> >>>> On Mon, Sep 13, 2021 at 11:41:04AM +0200, Maxime Coquelin wrote:
> >>>>> Hi Nélio,
> >>>>>
> >>>>> On 9/10/21 4:16 PM, Nélio Laranjeiro wrote:
> >>>>>> On Fri, Sep 10, 2021 at 01:06:53PM +0300, Andrew Rybchenko wrote:
> >>>>>>> On 9/10/21 12:57 PM, Maxime Coquelin wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 9/10/21 11:51 AM, Andrew Rybchenko wrote:
> >>>>>>>>> On 9/10/21 12:17 PM, Maxime Coquelin wrote:
> >>>>>>>>>> port_rss_hash_key_update() initializes rss_conf with the RSS
> >>>>>>>>>> hash type and key provided by the user, but it calls
> >>>>>>>>>> rte_eth_dev_rss_hash_conf_get() before calling
> >>>>>>>>>> rte_eth_dev_rss_hash_update(), which overides the parsed
> >>>>>>>>>> config with current NIC's config.
> >>>>>>>>>>
> >>>>>>>>>> While the RSS key value is set again after, this is not the
> >>>>>>>>>> case of the key length and the type of hash.
> >>>>>>>>>>
> >>>>>>>>>> There is no need to read the RSS config from the NIC, let's
> >>>>>>>>>> just try to set the user defined one.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: 8205e241b2b0 ("app/testpmd: add missing type to RSS
> >>>>>>>>>> hash
> >>>>>>>>>> commands")
> >>>>>>>>>> Cc: stable@dpdk.org
> >>>>>>>>>> Cc: nelio.laranjeiro@6wind.com
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  app/test-pmd/config.c | 8 ++------
> >>>>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> >>>>>>>>>> index
> >>>>>>>>>> 31d8ba1b91..451bda53b1 100644
> >>>>>>>>>> --- a/app/test-pmd/config.c
> >>>>>>>>>> +++ b/app/test-pmd/config.c
> >>>>>>>>>> @@ -2853,18 +2853,14 @@ port_rss_hash_key_update(portid_t
> >>>> port_id, char rss_type[], uint8_t *hash_key,
> >>>>>>>>>>  	int diag;
> >>>>>>>>>>  	unsigned int i;
> >>>>>>>>>>
> >>>>>>>>>> -	rss_conf.rss_key = NULL;
> >>>>>>>>>> +	rss_conf.rss_key = hash_key;
> >>>>>>>>>>  	rss_conf.rss_key_len = hash_key_len;
> >>>>>>>>>>  	rss_conf.rss_hf = 0;
> >>>>>>>>>>  	for (i = 0; rss_type_table[i].str; i++) {
> >>>>>>>>>>  		if (!strcmp(rss_type_table[i].str, rss_type))
> >>>>>>>>>>  			rss_conf.rss_hf = rss_type_table[i].rss_type;
> >>>>>>>>>>  	}
> >>>>>>>>>> -	diag = rte_eth_dev_rss_hash_conf_get(port_id, &rss_conf);
> >>>>>>>>>> -	if (diag == 0) {
> >>>>>>>>>> -		rss_conf.rss_key = hash_key;
> >>>>>>>>>> -		diag = rte_eth_dev_rss_hash_update(port_id,
> >>>> &rss_conf);
> >>>>>>>>>> -	}
> >>>>>>>>>> +	diag = rte_eth_dev_rss_hash_update(port_id, &rss_conf);
> >>>>>>>>>
> >>>>>>>>> I'm not 100% sure, but I'd say the intent above could be to
> >>>>>>>>> update key only as the function name says. I.e. keep rss_hf as
> >>>>>>>>> is. That could be the reason to get first.
> >>>
> >>> +1
> >>> The intent is only updating rss key. That's why to get_rss_conf first.
> >>> At least for all intel devices. RSS key is a global value for all rss_hf.
> >>>
> >>> And since the intent is to only update the key value. I don't think
> >>> this patch
> >> makes sense since you're changing rss_hf which will break current test cases.
> >>> And before 8205e241b2b01c, the command is what we want.
> >> port_rss_hash_key_update(portid_t port_id, uint8_t *hash_key) only
> >> updates key.
> >>>
> >>> But if mlx has the configuration of config key for each rss_type
> >>> then the code
> >> should remain to the current code in which keylen and rss_hf will be
> >> overridden to the correct value intel wants and mlx has their own
> >> configuration for specific rss type.
> >>> But to be honest, I checked mlx5. I don't see they have this kind of
> >> configuration. Need to confirm with their driver maintainer though.
> >>>
> >>> But anyway, from my point of view, I prefer to revert what
> >>> 8205e241b2b01c0
> >> does and remove rss_hf, rss_key_len setting in this command if mlx
> >> doesn't have key update for specific rss type. Otherwise, remain what it's like
> wight now.
> >>
> >> For the main branch, we could revert 8205e241b2b01c0, but in my
> >> opinion, we need to keep the hash_key_length otherwise it could lead
> >> to out of bounds accesses if the key passed by the user is shorter
> >> than the key length in use by the driver.
> >>
> >> Note that it would also imply changes in the DTS and CIs tests that
> >> make use of this command. We would also need to introduce a new
> >> command to be able to set the rss hash types, and rename the existing one to
> be key- specific.
> >> Otherwise we have no way with testpmd to configure RSS properly.
> >> Given there does not seem to have any driver that leverages RSS
> >> HF/Key pairs, maybe the simple thing is to just do what I did in this patch.
> >>
> >> For stable, I don't think this is a good idea to change testpmd
> >> commands, as it could break users setups.
> >
> > I don’t think so. This command is used for key setting only like the name says.
> And users use this command only setting key. What you did in this patch actually
> will break test cases results.
> >
> > And change hash type you can use command "port config all rss
> eth|vlan|tcp|...".
> 
> Ok, I did not find this command as I was trying completion with 'port config 0
> rss'. It might be good to to change it so that we can configure it per port, but
> not a big deal.
> 
> Remains the key length that should be passed, otherwise can have out of bounds
> accesses, no? And we still need to update DTS if we revert 8205e241b2b01c0.

The key length will be overridden after calling hash_conf_get so I think it doesn’t need to be passed.
It's useful before calling port_rss_hash_key_update() for "key" check.
And I agree with the breaking test cases for cmdlines, what about just remove the useless setting key_len and rss_hf assignment since they will be overridden anyway?
But rss_hf that users set is not taking effect anyway. Want to clean it but we can't. Tricky.

But I don't have a strong opinion to revert that patch.

> 
> Thanks,
> Maxime
> 
> >
> >>
> >> Thanks,
> >> Maxime
> >>
> >>>
> >>> BRs
> >>> Xiaoyun
> >>>
> >>>>>>
> >>>>>> True,
> >>>>>>
> >>>>>>>> I think that was the intial purpose of the command, but patch
> >>>>>>>> 8205e241b2b0 added setting the hash type as mandatory. There
> >>>>>>>> are no other command to configure the hash type from testpmd
> AFAICT.
> >>>>>>
> >>>>>> Also for the same initial purpose, some NIC have an hash key per
> >>>>>> protocol, by default it uses the same key for all of them but it
> >>>>>> can be configured individually making for example key0 for all
> >>>>>> protocols expect
> >>>>>> IPv4 which uses key1.
> >>>>>
> >>>>> Thanks for the info, I have looked at most drivers but didn't
> >>>>> found one that support this feature, could you give some pointer?
> >>>>
> >>>> Well, I have done the modification at that time for MLX5 PMD, since
> >>>> I left DPDK in 2018 I don't know if they still support such
> >>>> configuration from this API or if they fully moved to rte_flow.
> >>>>
> >>>>> Given how the drivers implément the callback, do you agree with
> >>>>> the fix, or do you have something else in mind?
> >>>>
> >>>> I cannot answer if this get() is mandatory, this predates my
> >>>> arrival on DPDK (original commit written in 2014), looking at DPDK
> >>>> state on commit
> >>>> f79959ea1504 ("app/testpmd: allow to configure RSS hash key").
> >>>> Maybe someone from Intel can help, eventually you can contact PMD
> >>>> maintainers to review this patch.
> >>>>
> >>>> Regards,
> >>>> Nélio
> >>>>
> >>>>> Thanks,
> >>>>> Maxime
> >>>>>
> >>>>>>>> Also, even without 8205e241b2b0, the function was broken
> >>>>>>>> because the key length was overiden.
> >>>>>>>
> >>>>>>> I see, many thanks for explanations.
> >>>>>>
> >>>>>
> >>>>
> >>>> --
> >>>> Nélio Laranjeiro
> >>>> 6WIND
> >>>
> >