Hi

-----Original Message-----
From: Yigit, Ferruh <redacted>
Sent: Friday, September 17, 2021 18:20
To: Li, Xiaoyun <redacted>; Wang, Jie1X <redacted>;
dev@dpdk.org
Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net;
jerinj@marvell.com; Ananyev, Konstantin [off-list ref]
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:

quoted

Hi

quoted

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

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

So keep port->rx/tx_conf? But then it still needs copy from dev_conf since this

may change.

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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?

This makes sense to me if I understand it right.

Thanks,
ferruh