[dpdk-stable] [PATCH 2/3] app/testpmd: fix RSS hash type update

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin at redhat.com>
> Sent: Thursday, September 16, 2021 16:09
> To: Li, Xiaoyun <xiaoyun.li at intel.com>; Nélio Laranjeiro
> <nelio.laranjeiro at 6wind.com>; Yigit, Ferruh <ferruh.yigit at intel.com>
> Cc: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org; Xia,
> Chenbo <chenbo.xia at intel.com>; amorenoz at redhat.com;
> david.marchand at redhat.com; michaelba at nvidia.com; viacheslavo at nvidia.com;
> stable at dpdk.org; Shahaf Shuler <shahafs at 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 at redhat.com>
> >> Sent: Thursday, September 16, 2021 15:34
> >> To: Li, Xiaoyun <xiaoyun.li at intel.com>; Nélio Laranjeiro
> >> <nelio.laranjeiro at 6wind.com>; Yigit, Ferruh <ferruh.yigit at intel.com>
> >> Cc: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org;
> >> Xia, Chenbo <chenbo.xia at intel.com>; amorenoz at redhat.com;
> >> david.marchand at redhat.com; michaelba at nvidia.com;
> >> viacheslavo at nvidia.com; stable at dpdk.org; Shahaf Shuler
> >> <shahafs at 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 at dpdk.org> On Behalf Of Nélio
> >>>> Laranjeiro
> >>>> Sent: Tuesday, September 14, 2021 15:20
> >>>> To: Maxime Coquelin <maxime.coquelin at redhat.com>; Yigit, Ferruh
> >>>> <ferruh.yigit at intel.com>
> >>>> Cc: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>; dev at dpdk.org;
> >>>> Xia, Chenbo <chenbo.xia at intel.com>; amorenoz at redhat.com;
> >>>> david.marchand at redhat.com; michaelba at nvidia.com;
> >>>> viacheslavo at nvidia.com; stable at dpdk.org; Shahaf Shuler
> >>>> <shahafs at 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 at dpdk.org
> >>>>>>>>>> Cc: nelio.laranjeiro at 6wind.com
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin at 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
> >>>
> >