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/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?

Thanks,
ferruh