Re: [dpdk-dev] [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash offload

["Li, Xiaoyun" <xiaoyun.li@intel.com>]

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.

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.