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. I prefer option 2). @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.