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

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

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