Hi

-----Original Message-----
From: Yigit, Ferruh <redacted>
Sent: Thursday, September 9, 2021 00:51
To: Wang, Jie1X <redacted>; dev@dpdk.org; Li, Xiaoyun
[off-list ref]
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:

quoted

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 <redacted>
---
 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",

quoted

+					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;

quoted

+			}
+			/* 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;

quoted

+			}
 		}

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.

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.

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.