Re: [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration

[Ferruh Yigit <ferruh.yigit@intel.com>]

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;