Hi
> -----Original Message-----
> From: stable <stable-boun...@dpdk.org> On Behalf Of Nélio Laranjeiro
> Sent: Tuesday, September 14, 2021 15:20
> To: Maxime Coquelin <maxime.coque...@redhat.com>; Yigit, Ferruh
> <ferruh.yi...@intel.com>
> Cc: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>; dev@dpdk.org; Xia,
> Chenbo <chenbo....@intel.com>; amore...@redhat.com;
> david.march...@redhat.com; michae...@nvidia.com; viachesl...@nvidia.com;
> sta...@dpdk.org; Shahaf Shuler <shah...@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: sta...@dpdk.org
> > >>>>> Cc: nelio.laranje...@6wind.com
> > >>>>>
> > >>>>> Signed-off-by: Maxime Coquelin <maxime.coque...@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
