On 7/19/21 7:18 PM, Ferruh Yigit wrote:
On 7/19/2021 10:55 AM, Wang, Jie1X wrote:
-----Original Message-----
From: Yigit, Ferruh <ferruh.yi...@intel.com>
Sent: Friday, July 16, 2021 4:52 PM
To: Li, Xiaoyun <xiaoyun...@intel.com>; Wang, Jie1X <jie1x.w...@intel.com>;
dev@dpdk.org
Cc: andrew.rybche...@oktetlabs.ru; sta...@dpdk.org
Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show
RSS hash offload
On 7/16/2021 9:30 AM, Li, Xiaoyun wrote:
-----Original Message-----
From: stable <stable-boun...@dpdk.org> On Behalf Of Li, Xiaoyun
Sent: Thursday, July 15, 2021 12:54
To: Wang, Jie1X <jie1x.w...@intel.com>; dev@dpdk.org
Cc: andrew.rybche...@oktetlabs.ru; sta...@dpdk.org
Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd
doesn't show RSS hash offload
-----Original Message-----
From: Wang, Jie1X <jie1x.w...@intel.com>
Sent: Thursday, July 15, 2021 19:57
To: dev@dpdk.org
Cc: Li, Xiaoyun <xiaoyun...@intel.com>;
andrew.rybche...@oktetlabs.ru; Wang, Jie1X <jie1x.w...@intel.com>;
sta...@dpdk.org
Subject: [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash
offload
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")
Cc: sta...@dpdk.org
Signed-off-by: Jie Wang <jie1x.w...@intel.com>
---
v4: delete the whitespace at the end of the line.
v3:
- check and update the "offloads" of "port->dev_conf.rx/txmode".
- update the commit log.
v2: copy "rx/txmode.offloads", instead of copying the entire struct
"dev->data-
dev_conf.rx/txmode".
---
app/test-pmd/testpmd.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
Acked-by: Xiaoyun Li <xiaoyun...@intel.com>
Although I gave my ack, app shouldn't touch rte_eth_devices which this patch
does. Usually, testpmd should only call function like
eth_dev_info_get_print_err().
But dev_info doesn't contain the info dev->data->dev_conf which the driver
modifies.
Probably we need a better fix.
Agree, an application accessing directly to 'rte_eth_devices' is sign of
something
missing/wrong.
In this case there is no way for application to know what is the configured
offload settings per port and queue. Which is missing part I think.
As you said normally we get data from PMD mainly via 'rte_eth_dev_info_get()',
which is an overloaded function, it provides many different things, like driver
default values, limitations, current config/status, capabilities etc...
So I think we can do a few things:
1) Add current offload configuration to 'rte_eth_dev_info_get()', so application
can get it and use it.
The advantage is this API already called many places, many times, so there is a
big chance that application already have this information when it needs.
Disadvantage is, as mentioned above the API already big and messy, making it
bigger makes more error prone and makes easier to break ABI.
I prefer to choose the 1st suggestion.
Normally PMD gets data via 'rte_eth_dev_info_get()'. When we add offloads
configuration
to it, we can get offloads as same as getting other info.
Most probably it is easier to implement 1), I see your point but as said before
I think 'rte_eth_dev_info_get()' is already messy and I am worried to make it
even bigger.
IMHO, (1) is not an option.
I prefer option 2).
I'm not sure that API function for each config parameter is an option as
well. We should find a balance. May be I'd add something like
rte_eth_dev_get_conf(uint16_t port_id, const struct rte_eth_conf **conf)
which returns a pointer to up-to-date configuration. I.e. option (3).
The tricky part here is to ensure that all specific API which modifies
various bits of the configuration updates dev_conf.
@Thomas, @Andrew, what do you think?
2) Add a new API to get configured offload information, so a specific API for
it.
3) Get a more generic API to get configured config (dev_conf) which will cover
offloads too.
Disadvantage can be leaking out too many internal config to user
unintentionally.
I don't understand it. dev_conf is provided by user on
rte_eth_dev_configure().
