Re: [dpdk-dev] [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash offload

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

On 9/18/2021 3:18 AM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Friday, September 17, 2021 18:20
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wang, Jie1X <jie1x.wang@intel.com>;
>> dev@dpdk.org
>> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net;
>> jerinj@marvell.com; Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
>> offload
>>
>> On 9/9/2021 4:31 AM, Li, Xiaoyun wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>> Sent: Thursday, September 9, 2021 00:51
>>>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org; Li, Xiaoyun
>>>> <xiaoyun.li@intel.com>
>>>> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net
>>>> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS
>>>> hash offload
>>>>
>>>> On 8/27/2021 9:17 AM, Jie Wang wrote:
>>>>> The driver may change offloads info into dev->data->dev_conf in
>>>>> dev_configure which may cause port->dev_conf and port->rx_conf
>>>>> contain outdated values.
>>>>>
>>>>> This patch updates the offloads info if it changes to fix this issue.
>>>>>
>>>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>>>>>
>>>>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>>>>> ---
>>>>>  app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>  app/test-pmd/testpmd.h |  2 ++
>>>>>  app/test-pmd/util.c    | 15 +++++++++++++++
>>>>>  3 files changed, 51 insertions(+)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> 6cbe9ba3c8..bd67291160 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2461,6 +2461,9 @@ start_port(portid_t pid)
>>>>>  		}
>>>>>
>>>>>  		if (port->need_reconfig > 0) {
>>>>> +			struct rte_eth_conf dev_conf_info;
>>>>> +			int k;
>>>>> +
>>>>>  			port->need_reconfig = 0;
>>>>>
>>>>>  			if (flow_isolate_all) {
>>>>> @@ -2498,6 +2501,37 @@ start_port(portid_t pid)
>>>>>  				port->need_reconfig = 1;
>>>>>  				return -1;
>>>>>  			}
>>>>> +			/* get rte_eth_conf info */
>>>>> +			if (0 !=
>>>>> +				eth_dev_conf_info_get_print_err(pi,
>>>>> +							&dev_conf_info)) {
>>>>> +				fprintf(stderr,
>>>>> +					"port %d can not get device
>>>> configuration info\n",
>>>>> +					pi);
>>>>> +				return -1;
>>>>> +			}
>>>>> +			/* Apply Rx offloads configuration */
>>>>> +			if (dev_conf_info.rxmode.offloads !=
>>>>> +				port->dev_conf.rxmode.offloads) {
>>>>> +				port->dev_conf.rxmode.offloads =
>>>>> +					dev_conf_info.rxmode.offloads;
>>>>> +				for (k = 0;
>>>>> +				     k < port->dev_info.max_rx_queues;
>>>>> +				     k++)
>>>>> +					port->rx_conf[k].offloads =
>>>>> +
>>>> 	dev_conf_info.rxmode.offloads;
>>>>> +			}
>>>>> +			/* Apply Tx offloads configuration */
>>>>> +			if (dev_conf_info.txmode.offloads !=
>>>>> +				port->dev_conf.txmode.offloads) {
>>>>> +				port->dev_conf.txmode.offloads =
>>>>> +					dev_conf_info.txmode.offloads;
>>>>> +				for (k = 0;
>>>>> +				     k < port->dev_info.max_tx_queues;
>>>>> +				     k++)
>>>>> +					port->tx_conf[k].offloads =
>>>>> +
>>>> 	dev_conf_info.txmode.offloads;
>>>>> +			}
>>>>>  		}
>>>>
>>>> Above implementation gets the configuration from device and applies
>>>> it to the testpmd configuration.
>>>>
>>>> Instead, what about a long level target to get rid of testpmd
>>>> specific copy of the configuration and rely and the config provided
>>>> by devices. @Xiaoyun, what do you think, does this make sense?
>>>
>>> You mean remove port->dev_conf and rx/tx_conf completely in the future? Or
>> keep it in initial stage?
>>>
>>> Now, port->dev_conf will take global tx/rx_mode, fdir_conf and change some
>> based on dev_info capabilities. And then use dev_configure to apply them for
>> device.
>>> After this, actually, dev->data->dev_conf contains all device configuration.
>>>
>>> So It seems it's OK to remove port->dev_conf completely. Just testpmd needs
>> to be refactored a lot and regression test in case of issues.
>>> But from long term view, it's good to keep one source and avoid copy.
>>>
>>
>> Yes, this is the intention I have for long term. I expect that testpmd still will keep
>> some configuration in application level but we can prevent some duplication.
>>
>> And the main point is, by cleaning up testpmd we can recognize blockers and fix
>> them in libraries to help user applications.
>>
>>> As for rx/tx_conf, it takes device default tx/rx_conf in dev_info and some
>> settings in testpmd parameters also offloads from dev_conf.
>>> So keep port->rx/tx_conf? But then it still needs copy from dev_conf since this
>> may change.
>>>
>>
>> I am not very clear what is suggested above, can you please elaborate?
>>
>> And 'struct rte_port' seems has following structs that can be get from library:
>> struct rte_eth_dev_info dev_info;
>> struct rte_eth_conf     dev_conf;
>> struct rte_eth_rxconf   rx_conf[]
>> struct rte_eth_txconf   tx_conf[]
>>
>> I don't think we can remove them, but perhaps reduce the usage of them, please
>> see below.
>>
>>>>
>>>> So instead of above code, update where RSS hash offload information
>>>> printed to use device retrieved config instead of testpmd config, will it work?
>>>
>>> It's OK for device offload configurations.
>>> But queue offloads are a bit tricky since dev->data->dev_conf doesn't include
>> queue conf.
>>> And it's not fair to use device offload configurations for queue offloads since
>> user can use cmdline to config queue offload and that info can only be saved in
>> port->rx/tx_conf and configure the device in setup_queue.
>>>
>>
>> It is common in testpmd that, a command changes the application copy of the
>> configs, and mark as device configuration is required (for port or for queue).
>> So in later stage this changed configuration is applied to device.
>>
>> This async approach has its benefits and I don't think we should change it.
>> (Also has some disadvantages that we hit in the past, like detecting some
>> configuration can't be applied in later stage when we try to apply the config, not
>> when command is issued at first place.).
>>
>> What we can do it, reduce the testpmd config usage for the case to gather user
>> requests and apply them to device.
>> But to display device configuration, or to decide based on device configuration
>> we can user config values get by device by APIs.
>>
>> What do you think, can above distinction makes sense, or does it work?
>>
>>
>> And there is still a chance that application copy of config diverge from device
>> config, and since we provide full config in our APIs (not changes), there is a
>> chance to overwrite a device configuration.
>> To prevent this it is possible to read device config and overwrite testpmd config
>> with that, similar to what this patch does, but I am not sure where this sync can
>> be done. What do you think about doing this just after device configured?
> 
> I'm not sure I fully understand.
> So for showing cmd, just use API rte_eth_tx/rx_queue_info_get to get dev queue config and new added API rte_eth_dev_conf_info_get to get dev config.
> 
> And for the cases where port->dev_config is used as a right value, replace them with use getting API.
> For example: "if (res->value == port->dev_conf.rxmode.max_rx_pkt_len)" will be changed like "if (res->value == rte_eth_dev_conf_info_get().rxmode.max_rx_pkt_len)"
> 
> But other things keep the same as what this patch does?
> 

Yes. (Only I have a small comment on this patch, I will comment on other tread.)

And for this patch I don't suggest any additional change other than RSS show,
rest can be updated gradually.

> This makes sense to me if I understand it right.