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

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

[Andrew Rybchenko]

On 7/9/21 6:57 PM, Jie Wang wrote:
> This patch reapply Rx/Tx offloads configuration for all ports
> after the program configuring the device port. When the program
> configures the ports, the default Rx/Tx offloads are modified.
> 
> So it is need to reapply Rx/Tx offloads configuration before
> testpmd showing offloads.
> 
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
>  app/test-pmd/testpmd.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 1cdd3cdd12..7089ae216d 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2475,6 +2475,9 @@ start_port(portid_t pid)
>  		}
>  
>  		if (port->need_reconfig > 0) {
> +			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
> +			int k;
> +
>  			port->need_reconfig = 0;
>  
>  			if (flow_isolate_all) {
> @@ -2508,6 +2511,18 @@ start_port(portid_t pid)
>  				port->need_reconfig = 1;
>  				return -1;
>  			}
> +
> +			/* Apply TxRx configuration for all ports */
> +			port->dev_conf.txmode = dev->data->dev_conf.txmode;
> +			port->dev_conf.rxmode = dev->data->dev_conf.rxmode;
> +			/* Apply Rx offloads configuration */
> +			for (k = 0; k < port->dev_info.max_rx_queues; k++)
> +				port->rx_conf[k].offloads =
> +					port->dev_conf.rxmode.offloads;
> +			/* Apply Tx offloads configuration */
> +			for (k = 0; k < port->dev_info.max_tx_queues; k++)
> +				port->tx_conf[k].offloads =
> +					port->dev_conf.txmode.offloads;

Does testpmd really require these copies? May be the right fix is to get
rid of these copies at all and show actual
information from data->dev_conf ?

[dpdk-stable] [PATCH] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

This patch reapply Rx/Tx offloads configuration for all ports
after the program configuring the device port. When the program
configures the ports, the default Rx/Tx offloads are modified.

So it is need to reapply Rx/Tx offloads configuration before
testpmd showing offloads.

Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
 app/test-pmd/testpmd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1cdd3cdd12..7089ae216d 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2475,6 +2475,9 @@ start_port(portid_t pid)
 		}
 
 		if (port->need_reconfig > 0) {
+			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
+			int k;
+
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2508,6 +2511,18 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+
+			/* Apply TxRx configuration for all ports */
+			port->dev_conf.txmode = dev->data->dev_conf.txmode;
+			port->dev_conf.rxmode = dev->data->dev_conf.rxmode;
+			/* Apply Rx offloads configuration */
+			for (k = 0; k < port->dev_info.max_rx_queues; k++)
+				port->rx_conf[k].offloads =
+					port->dev_conf.rxmode.offloads;
+			/* Apply Tx offloads configuration */
+			for (k = 0; k < port->dev_info.max_tx_queues; k++)
+				port->tx_conf[k].offloads =
+					port->dev_conf.txmode.offloads;
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
-- 
2.25.1

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

[Li, Xiaoyun]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, July 9, 2021 17:27
> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix testpmd doesn't show RSS
> hash offload
> 
> On 7/9/21 6:57 PM, Jie Wang wrote:
> > This patch reapply Rx/Tx offloads configuration for all ports after
> > the program configuring the device port. When the program configures
> > the ports, the default Rx/Tx offloads are modified.
> >
> > So it is need to reapply Rx/Tx offloads configuration before testpmd
> > showing offloads.
> >
> > Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 1cdd3cdd12..7089ae216d 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2475,6 +2475,9 @@ start_port(portid_t pid)
> >  		}
> >
> >  		if (port->need_reconfig > 0) {
> > +			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
> > +			int k;
> > +
> >  			port->need_reconfig = 0;
> >
> >  			if (flow_isolate_all) {
> > @@ -2508,6 +2511,18 @@ start_port(portid_t pid)
> >  				port->need_reconfig = 1;
> >  				return -1;
> >  			}
> > +
> > +			/* Apply TxRx configuration for all ports */
> > +			port->dev_conf.txmode = dev->data-
> >dev_conf.txmode;
> > +			port->dev_conf.rxmode = dev->data-
> >dev_conf.rxmode;
> > +			/* Apply Rx offloads configuration */
> > +			for (k = 0; k < port->dev_info.max_rx_queues; k++)
> > +				port->rx_conf[k].offloads =
> > +					port->dev_conf.rxmode.offloads;
> > +			/* Apply Tx offloads configuration */
> > +			for (k = 0; k < port->dev_info.max_tx_queues; k++)
> > +				port->tx_conf[k].offloads =
> > +					port->dev_conf.txmode.offloads;
> 
> Does testpmd really require these copies? May be the right fix is to get rid of
> these copies at all and show actual information from data->dev_conf ?
> 
The "show" command cmd_rx_offload_get_configuration_parsed() actually should only touch port->dev_conf.
And It doesn't only show dev_conf, but also per queue rx_conf. It's impossible to show per queue rx conf info from data->dev_conf.

The copy is already done in init_config actually. But by then, dev->data->dev_conf doesn't include RSS_HASH.
RSS_HASH is added by driver in dev_configure. That's why this patch wants to copy again after dev_configure I guess.

But this patch is INCORRECT anyway.
Because any cmd that changes port offload will change port->dev_conf and port->rx/tx_conf NOT dev->data->dev_conf.
So after this patch, if a user " port config 0 rx_offload xxx on", all of the new config will disappear.

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

[Li, Xiaoyun]

> -----Original Message-----
> From: Wang, Jie1X <jie1x.wang@intel.com>
> Sent: Tuesday, July 13, 2021 10:34
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru;
> dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd doesn't show RSS
> hash offload
> 
> Hi all,
> I have tested this patch, it still worked on.
> 
> When we changed the port offloads, the port->dev_conf would change in the
> same time. And in the function " rte_eth_dev_configure ", dev->data->dev_conf
> copied the port->dev_conf parameter into the dev structure.

OK. A reminder: Please answer these under every part you want to explain not the very start. It looks messy in mail-list.
But still, this patch has issues.
Please see inlines.
> 
> -----Original Message-----
> From: Li, Xiaoyun <xiaoyun.li@intel.com>
> Sent: Monday, July 12, 2021 11:12 AM
> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Wang, Jie1X
> <jie1x.wang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix testpmd doesn't show RSS
> hash offload
> 
> 
> 
> > -----Original Message-----
> > From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> > Sent: Friday, July 9, 2021 17:27
> > To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> > Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix testpmd doesn't show
> > RSS hash offload
> >
> > On 7/9/21 6:57 PM, Jie Wang wrote:
> > > This patch reapply Rx/Tx offloads configuration for all ports after
> > > the program configuring the device port. When the program configures
> > > the ports, the default Rx/Tx offloads are modified.
> > >
> > > So it is need to reapply Rx/Tx offloads configuration before testpmd
> > > showing offloads.
> > >
> > > Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> > > ---
> > >  app/test-pmd/testpmd.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > 1cdd3cdd12..7089ae216d 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -2475,6 +2475,9 @@ start_port(portid_t pid)
> > >  		}
> > >
> > >  		if (port->need_reconfig > 0) {
> > > +			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
> > > +			int k;
> > > +
> > >  			port->need_reconfig = 0;
> > >
> > >  			if (flow_isolate_all) {
> > > @@ -2508,6 +2511,18 @@ start_port(portid_t pid)
> > >  				port->need_reconfig = 1;
> > >  				return -1;
> > >  			}
> > > +
> > > +			/* Apply TxRx configuration for all ports */
> > > +			port->dev_conf.txmode = dev->data-
> > >dev_conf.txmode;
> > > +			port->dev_conf.rxmode = dev->data-
> > >dev_conf.rxmode;

Direct assignment for a big struct is dangerous. Next time, if you're going to assign a struct, please use memcpy.
And actually, your issue will only happen for one time for rss_hash because it seems only rss_hash will be added in driver.
Doing these copies every time seems very redundant.
You'd better check if port->rxmode.offloads==dev->data->dev_conf.rx_mode.offloads, if it's not equal, do the new assignment, including queue offload. Otherwise, don't.
Same with txmode.offloads.

> > > +			/* Apply Rx offloads configuration */
> > > +			for (k = 0; k < port->dev_info.max_rx_queues; k++)
> > > +				port->rx_conf[k].offloads =
> > > +					port->dev_conf.rxmode.offloads;
> > > +			/* Apply Tx offloads configuration */
> > > +			for (k = 0; k < port->dev_info.max_tx_queues; k++)
> > > +				port->tx_conf[k].offloads =
> > > +					port->dev_conf.txmode.offloads;
> >
> > Does testpmd really require these copies? May be the right fix is to
> > get rid of these copies at all and show actual information from data-
> >dev_conf ?
> >
> The "show" command cmd_rx_offload_get_configuration_parsed() actually
> should only touch port->dev_conf.
> And It doesn't only show dev_conf, but also per queue rx_conf. It's impossible to
> show per queue rx conf info from data->dev_conf.
> 
> The copy is already done in init_config actually. But by then, dev->data-
> >dev_conf doesn't include RSS_HASH.
> RSS_HASH is added by driver in dev_configure. That's why this patch wants to
> copy again after dev_configure I guess.
> 
> But this patch is INCORRECT anyway.
> Because any cmd that changes port offload will change port->dev_conf and
> port->rx/tx_conf NOT dev->data->dev_conf.
> So after this patch, if a user " port config 0 rx_offload xxx on", all of the new
> config will disappear.

[dpdk-stable] [PATCH v2] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

This patch reapply Rx/Tx offloads configuration for all ports
after the program configuring the device port. When the program
configures the ports, the default Rx/Tx offloads are modified in
dev->data->dev_conf.

So it is need to reapply Rx/Tx offloads configuration after
dev_configure.

Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
v2: copy "rx/txmode.offloads", instead of copying the entire struct 
"dev->data->dev_conf.rx/txmode".
---
 app/test-pmd/testpmd.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1cdd3cdd12..dc9ce5330b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2475,6 +2475,11 @@ start_port(portid_t pid)
 		}
 
 		if (port->need_reconfig > 0) {
+			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
+			struct rte_eth_conf *dev_conf;
+			int k;
+
+			dev_conf = &dev->data->dev_conf;
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2508,6 +2513,20 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+			/* Apply Rx offloads configuration */
+			for (k = 0; k < port->dev_info.max_rx_queues; k++) {
+				if (port->rx_conf[k].offloads !=
+					dev_conf->rxmode.offloads)
+					port->rx_conf[k].offloads =
+						dev_conf->rxmode.offloads;
+			}
+			/* Apply Tx offloads configuration */
+			for (k = 0; k < port->dev_info.max_tx_queues; k++) {
+				if (port->tx_conf[k].offloads !=
+					dev_conf->txmode.offloads)
+					port->tx_conf[k].offloads =
+						dev_conf->txmode.offloads;
+			}
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
-- 
2.25.1

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix testpmd doesn't show RSS hash offload

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Wang, Jie1X <jie1x.wang@intel.com>
> Sent: Wednesday, July 14, 2021 01:04
> To: dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru; Wang,
> Jie1X <jie1x.wang@intel.com>; stable@dpdk.org
> Subject: [PATCH v2] app/testpmd: fix testpmd doesn't show RSS hash offload
> 
> This patch reapply Rx/Tx offloads configuration for all ports after the program
> configuring the device port. When the program configures the ports, the default
> Rx/Tx offloads are modified in
> dev->data->dev_conf.
> 
> So it is need to reapply Rx/Tx offloads configuration after dev_configure.
> 
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
> v2: copy "rx/txmode.offloads", instead of copying the entire struct "dev->data-
> >dev_conf.rx/txmode".
> ---
>  app/test-pmd/testpmd.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 1cdd3cdd12..dc9ce5330b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2475,6 +2475,11 @@ start_port(portid_t pid)
>  		}
> 
>  		if (port->need_reconfig > 0) {
> +			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
> +			struct rte_eth_conf *dev_conf;
> +			int k;
> +
> +			dev_conf = &dev->data->dev_conf;
>  			port->need_reconfig = 0;
> 
>  			if (flow_isolate_all) {
> @@ -2508,6 +2513,20 @@ start_port(portid_t pid)
>  				port->need_reconfig = 1;
>  				return -1;
>  			}
> +			/* Apply Rx offloads configuration */
> +			for (k = 0; k < port->dev_info.max_rx_queues; k++) {
> +				if (port->rx_conf[k].offloads !=
> +					dev_conf->rxmode.offloads)
> +					port->rx_conf[k].offloads =
> +						dev_conf->rxmode.offloads;
> +			}

I was suggesting something like:
	if (dev_conf->rxmode.offloads != port->dev_conf->rxmode.offloads) {
		port->dev_conf->rxmode.offloads = dev_conf->rxmode.offloads;
		for (queues)
			update offloads for each queue of this port
	}

Not only check and change queue offload.

> +			/* Apply Tx offloads configuration */
> +			for (k = 0; k < port->dev_info.max_tx_queues; k++) {
> +				if (port->tx_conf[k].offloads !=
> +					dev_conf->txmode.offloads)
> +					port->tx_conf[k].offloads =
> +						dev_conf->txmode.offloads;
> +			}
>  		}
>  		if (port->need_reconfig_queues > 0) {
>  			port->need_reconfig_queues = 0;
> --
> 2.25.1

Re: [dpdk-stable] [PATCH v2] app/testpmd: fix testpmd doesn't show RSS hash offload

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: stable <stable-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
> Sent: Thursday, July 15, 2021 10:30
> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH v2] app/testpmd: fix testpmd doesn't show
> RSS hash offload
> 
> Hi
> 
> > -----Original Message-----
> > From: Wang, Jie1X <jie1x.wang@intel.com>
> > Sent: Wednesday, July 14, 2021 01:04
> > To: dev@dpdk.org
> > Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru;
> > Wang, Jie1X <jie1x.wang@intel.com>; stable@dpdk.org
> > Subject: [PATCH v2] app/testpmd: fix testpmd doesn't show RSS hash
> > offload
> >
> > This patch reapply Rx/Tx offloads configuration for all ports after
> > the program configuring the device port. When the program configures
> > the ports, the default Rx/Tx offloads are modified in
> > dev->data->dev_conf.
> >
> > So it is need to reapply Rx/Tx offloads configuration after dev_configure.

Forgot to mention. The commit log has many grammar mistakes especially time tense.
You can use the following:
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: stable@dpdk.org
> >
> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> > ---
> > v2: copy "rx/txmode.offloads", instead of copying the entire struct
> > "dev->data-
> > >dev_conf.rx/txmode".
> > ---
> >  app/test-pmd/testpmd.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 1cdd3cdd12..dc9ce5330b 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2475,6 +2475,11 @@ start_port(portid_t pid)
> >  		}
> >
> >  		if (port->need_reconfig > 0) {
> > +			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
> > +			struct rte_eth_conf *dev_conf;
> > +			int k;
> > +
> > +			dev_conf = &dev->data->dev_conf;
> >  			port->need_reconfig = 0;
> >
> >  			if (flow_isolate_all) {
> > @@ -2508,6 +2513,20 @@ start_port(portid_t pid)
> >  				port->need_reconfig = 1;
> >  				return -1;
> >  			}
> > +			/* Apply Rx offloads configuration */
> > +			for (k = 0; k < port->dev_info.max_rx_queues; k++) {
> > +				if (port->rx_conf[k].offloads !=
> > +					dev_conf->rxmode.offloads)
> > +					port->rx_conf[k].offloads =
> > +						dev_conf->rxmode.offloads;
> > +			}
> 
> I was suggesting something like:
> 	if (dev_conf->rxmode.offloads != port->dev_conf->rxmode.offloads) {
> 		port->dev_conf->rxmode.offloads = dev_conf-
> >rxmode.offloads;
> 		for (queues)
> 			update offloads for each queue of this port
> 	}
> 
> Not only check and change queue offload.
> 
> > +			/* Apply Tx offloads configuration */
> > +			for (k = 0; k < port->dev_info.max_tx_queues; k++) {
> > +				if (port->tx_conf[k].offloads !=
> > +					dev_conf->txmode.offloads)
> > +					port->tx_conf[k].offloads =
> > +						dev_conf->txmode.offloads;
> > +			}
> >  		}
> >  		if (port->need_reconfig_queues > 0) {
> >  			port->need_reconfig_queues = 0;
> > --
> > 2.25.1

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Li, Xiaoyun]

> -----Original Message-----
> From: Wang, Jie1X <jie1x.wang@intel.com>
> Sent: Thursday, July 15, 2021 19:57
> To: dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru; Wang,
> Jie1X <jie1x.wang@intel.com>; stable@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: stable@dpdk.org
> 
> Signed-off-by: Jie Wang <jie1x.wang@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.li@intel.com>

[dpdk-stable] [PATCH v3] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

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: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
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(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1cdd3cdd12..26550340e4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2475,6 +2475,11 @@ start_port(portid_t pid)
 		}
 
 		if (port->need_reconfig > 0) {
+			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
+			struct rte_eth_conf *dev_conf;
+			int k;
+
+			dev_conf = &dev->data->dev_conf;
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2508,6 +2513,28 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+			/* Apply Rx offloads configuration */
+			if (dev_conf->rxmode.offloads !=
+				port->dev_conf.rxmode.offloads) {
+				port->dev_conf.rxmode.offloads =
+					dev_conf->rxmode.offloads;
+				for (k = 0;
+				     k < port->dev_info.max_rx_queues;
+				     k++)
+					port->rx_conf[k].offloads =
+						dev_conf->rxmode.offloads;
+			}
+			/* Apply Tx offloads configuration */
+			if (dev_conf->txmode.offloads !=
+				port->dev_conf.txmode.offloads) {
+				port->dev_conf.txmode.offloads =
+					dev_conf->txmode.offloads;
+				for (k = 0;
+				     k < port->dev_info.max_tx_queues; 
+				     k++)
+					port->tx_conf[k].offloads =
+						dev_conf->txmode.offloads;
+			}
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
-- 
2.25.1

[dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

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: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@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(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 1cdd3cdd12..b6e8d7cdd9 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2475,6 +2475,11 @@ start_port(portid_t pid)
 		}
 
 		if (port->need_reconfig > 0) {
+			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
+			struct rte_eth_conf *dev_conf;
+			int k;
+
+			dev_conf = &dev->data->dev_conf;
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2508,6 +2513,28 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+			/* Apply Rx offloads configuration */
+			if (dev_conf->rxmode.offloads !=
+				port->dev_conf.rxmode.offloads) {
+				port->dev_conf.rxmode.offloads =
+					dev_conf->rxmode.offloads;
+				for (k = 0;
+				     k < port->dev_info.max_rx_queues;
+				     k++)
+					port->rx_conf[k].offloads =
+						dev_conf->rxmode.offloads;
+			}
+			/* Apply Tx offloads configuration */
+			if (dev_conf->txmode.offloads !=
+				port->dev_conf.txmode.offloads) {
+				port->dev_conf.txmode.offloads =
+					dev_conf->txmode.offloads;
+				for (k = 0;
+				     k < port->dev_info.max_tx_queues;
+				     k++)
+					port->tx_conf[k].offloads =
+						dev_conf->txmode.offloads;
+			}
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
-- 
2.25.1

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Li, Xiaoyun]

> -----Original Message-----
> From: stable <stable-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
> Sent: Thursday, July 15, 2021 12:54
> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show
> RSS hash offload
> 
> > -----Original Message-----
> > From: Wang, Jie1X <jie1x.wang@intel.com>
> > Sent: Thursday, July 15, 2021 19:57
> > To: dev@dpdk.org
> > Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru;
> > Wang, Jie1X <jie1x.wang@intel.com>; stable@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: stable@dpdk.org
> >
> > Signed-off-by: Jie Wang <jie1x.wang@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.li@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.

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Ferruh Yigit]

On 7/16/2021 9:30 AM, Li, Xiaoyun wrote:
>> -----Original Message-----
>> From: stable <stable-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
>> Sent: Thursday, July 15, 2021 12:54
>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
>> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show
>> RSS hash offload
>>
>>> -----Original Message-----
>>> From: Wang, Jie1X <jie1x.wang@intel.com>
>>> Sent: Thursday, July 15, 2021 19:57
>>> To: dev@dpdk.org
>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru;
>>> Wang, Jie1X <jie1x.wang@intel.com>; stable@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: stable@dpdk.org
>>>
>>> Signed-off-by: Jie Wang <jie1x.wang@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.li@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.

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.

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

[Ferruh Yigit]

On 7/12/2021 4:12 AM, Li, Xiaoyun wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Friday, July 9, 2021 17:27
>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix testpmd doesn't show RSS
>> hash offload
>>
>> On 7/9/21 6:57 PM, Jie Wang wrote:
>>> This patch reapply Rx/Tx offloads configuration for all ports after
>>> the program configuring the device port. When the program configures
>>> the ports, the default Rx/Tx offloads are modified.
>>>
>>> So it is need to reapply Rx/Tx offloads configuration before testpmd
>>> showing offloads.
>>>
>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>>> ---
>>>  app/test-pmd/testpmd.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 1cdd3cdd12..7089ae216d 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2475,6 +2475,9 @@ start_port(portid_t pid)
>>>  		}
>>>
>>>  		if (port->need_reconfig > 0) {
>>> +			const struct rte_eth_dev *dev = &rte_eth_devices[pi];
>>> +			int k;
>>> +
>>>  			port->need_reconfig = 0;
>>>
>>>  			if (flow_isolate_all) {
>>> @@ -2508,6 +2511,18 @@ start_port(portid_t pid)
>>>  				port->need_reconfig = 1;
>>>  				return -1;
>>>  			}
>>> +
>>> +			/* Apply TxRx configuration for all ports */
>>> +			port->dev_conf.txmode = dev->data-
>>> dev_conf.txmode;
>>> +			port->dev_conf.rxmode = dev->data-
>>> dev_conf.rxmode;
>>> +			/* Apply Rx offloads configuration */
>>> +			for (k = 0; k < port->dev_info.max_rx_queues; k++)
>>> +				port->rx_conf[k].offloads =
>>> +					port->dev_conf.rxmode.offloads;
>>> +			/* Apply Tx offloads configuration */
>>> +			for (k = 0; k < port->dev_info.max_tx_queues; k++)
>>> +				port->tx_conf[k].offloads =
>>> +					port->dev_conf.txmode.offloads;
>>
>> Does testpmd really require these copies? May be the right fix is to get rid of
>> these copies at all and show actual information from data->dev_conf ?
>>
> The "show" command cmd_rx_offload_get_configuration_parsed() actually should only touch port->dev_conf.
> And It doesn't only show dev_conf, but also per queue rx_conf. It's impossible to show per queue rx conf info from data->dev_conf.
> 

Testpmd having these copies also bothers me. This seems because ethdev doesn't
provide a way to get current config, so application has to keep the copy, but
this is causing complexities and diversions as this case.

So we can try to address this in a larger scale and try to eliminate the need of
application config copies.

Also RSS_HASH offload flag being set by PMDs unconventionally (instead of
application) making this issue visible, but I think there isn't much we can do
about RSS_HASH.


> The copy is already done in init_config actually. But by then, dev->data->dev_conf doesn't include RSS_HASH.
> RSS_HASH is added by driver in dev_configure. That's why this patch wants to copy again after dev_configure I guess.
> 
> But this patch is INCORRECT anyway.
> Because any cmd that changes port offload will change port->dev_conf and port->rx/tx_conf NOT dev->data->dev_conf.
> So after this patch, if a user " port config 0 rx_offload xxx on", all of the new config will disappear.
> 

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Ferruh Yigit]

On 7/19/2021 10:55 AM, Wang, Jie1X wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Friday, July 16, 2021 4:52 PM
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wang, Jie1X <jie1x.wang@intel.com>;
>> dev@dpdk.org
>> Cc: andrew.rybchenko@oktetlabs.ru; stable@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-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
>>>> Sent: Thursday, July 15, 2021 12:54
>>>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
>>>> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org
>>>> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd
>>>> doesn't show RSS hash offload
>>>>
>>>>> -----Original Message-----
>>>>> From: Wang, Jie1X <jie1x.wang@intel.com>
>>>>> Sent: Thursday, July 15, 2021 19:57
>>>>> To: dev@dpdk.org
>>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>;
>>>>> andrew.rybchenko@oktetlabs.ru; Wang, Jie1X <jie1x.wang@intel.com>;
>>>>> stable@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: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Jie Wang <jie1x.wang@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.li@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.

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Andrew Rybchenko]

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.yigit@intel.com>
>>> Sent: Friday, July 16, 2021 4:52 PM
>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wang, Jie1X <jie1x.wang@intel.com>;
>>> dev@dpdk.org
>>> Cc: andrew.rybchenko@oktetlabs.ru; stable@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-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
>>>>> Sent: Thursday, July 15, 2021 12:54
>>>>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
>>>>> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org
>>>>> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd
>>>>> doesn't show RSS hash offload
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Wang, Jie1X <jie1x.wang@intel.com>
>>>>>> Sent: Thursday, July 15, 2021 19:57
>>>>>> To: dev@dpdk.org
>>>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>;
>>>>>> andrew.rybchenko@oktetlabs.ru; Wang, Jie1X <jie1x.wang@intel.com>;
>>>>>> stable@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: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Jie Wang <jie1x.wang@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.li@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().

Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload

[Ferruh Yigit]

On 7/22/2021 12:03 PM, Andrew Rybchenko wrote:
> 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.yigit@intel.com>
>>>> Sent: Friday, July 16, 2021 4:52 PM
>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wang, Jie1X <jie1x.wang@intel.com>;
>>>> dev@dpdk.org
>>>> Cc: andrew.rybchenko@oktetlabs.ru; stable@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-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
>>>>>> Sent: Thursday, July 15, 2021 12:54
>>>>>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
>>>>>> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org
>>>>>> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd
>>>>>> doesn't show RSS hash offload
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Wang, Jie1X <jie1x.wang@intel.com>
>>>>>>> Sent: Thursday, July 15, 2021 19:57
>>>>>>> To: dev@dpdk.org
>>>>>>> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>;
>>>>>>> andrew.rybchenko@oktetlabs.ru; Wang, Jie1X <jie1x.wang@intel.com>;
>>>>>>> stable@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: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Jie Wang <jie1x.wang@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.li@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).
> 

That is option 3, that can work too.

> The tricky part here is to ensure that all specific API which modifies
> various bits of the configuration updates dev_conf.
> 

They have to, aren't they? Otherwise there is no where to record the current
config for PMD too.

>>
>> @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().

Yes but application doesn't provide all config, my concern was if some internal
config should be hidden from applications (possibly via some APIs).

Overall I am OK to go with option 3, I think it can simplify the applications
life. And later we can have some more updates on testpmd to benefit from new API.

[dpdk-stable] [PATCH v5] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

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.

It adds a new API "rte_eth_dev_conf_info_get()" to help testpmd get
device configuration info.

Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Cc: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
 app/test-pmd/testpmd.c  | 33 +++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h  |  2 ++
 app/test-pmd/util.c     | 15 +++++++++++++++
 lib/ethdev/rte_ethdev.c | 27 +++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.h | 26 ++++++++++++++++++++++++++
 5 files changed, 103 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 6cbe9ba3c8..9a78805673 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_dev_conf_info dev_conf_info;
+			int k;
+
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2498,6 +2501,36 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+			/* get offloads */
+			if (0 !=
+				eth_dev_conf_info_get_print_err(pi,
+							&dev_conf_info)) {
+				rte_exit(EXIT_FAILURE,
+				    "rte_eth_dev_conf_info_get() failed\n");
+				return -1;
+			}
+			/* Apply Rx offloads configuration */
+			if (dev_conf_info.rx_offloads !=
+				port->dev_conf.rxmode.offloads) {
+				port->dev_conf.rxmode.offloads =
+					dev_conf_info.rx_offloads;
+				for (k = 0;
+				     k < port->dev_info.max_rx_queues;
+				     k++)
+					port->rx_conf[k].offloads =
+						dev_conf_info.rx_offloads;
+			}
+			/* Apply Tx offloads configuration */
+			if (dev_conf_info.tx_offloads !=
+				port->dev_conf.txmode.offloads) {
+				port->dev_conf.txmode.offloads =
+					dev_conf_info.tx_offloads;
+				for (k = 0;
+				     k < port->dev_info.max_tx_queues;
+				     k++)
+					port->tx_conf[k].offloads =
+						dev_conf_info.tx_offloads;
+			}
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 16a3598e48..4cf69a17a3 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -950,6 +950,8 @@ void show_gro(portid_t port_id);
 void setup_gso(const char *mode, portid_t port_id);
 int eth_dev_info_get_print_err(uint16_t port_id,
 			struct rte_eth_dev_info *dev_info);
+int eth_dev_conf_info_get_print_err(uint16_t port_id,
+			struct rte_eth_dev_conf_info *dev_info);
 void eth_set_promisc_mode(uint16_t port_id, int enable);
 void eth_set_allmulticast_mode(uint16_t port, int enable);
 int eth_link_get_nowait_print_err(uint16_t port_id, struct rte_eth_link *link);
diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
index 5dd7157947..c6992fba7a 100644
--- a/app/test-pmd/util.c
+++ b/app/test-pmd/util.c
@@ -440,6 +440,21 @@ eth_dev_info_get_print_err(uint16_t port_id,
 	return ret;
 }
 
+int
+eth_dev_conf_info_get_print_err(uint16_t port_id,
+				struct rte_eth_dev_conf_info *dev_conf_info)
+{
+	int ret;
+
+	ret = rte_eth_dev_conf_info_get(port_id, dev_conf_info);
+	if (ret != 0)
+		fprintf(stderr,
+			"Error during getting device config (port %u) info: %s\n",
+			port_id, strerror(-ret));
+
+	return ret;
+}
+
 void
 eth_set_promisc_mode(uint16_t port, int enable)
 {
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 9d95cd11e1..74184099a1 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -3458,6 +3458,33 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 	return 0;
 }
 
+int
+rte_eth_dev_conf_info_get(uint16_t port_id,
+				struct rte_eth_dev_conf_info *dev_conf_info)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (dev_conf_info == NULL) {
+		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u config info to NULL\n",
+			port_id);
+		return -EINVAL;
+	}
+
+	/*
+	 * Init dev_conf_info before port_id check since caller does not have
+	 * return status and does not know if get is successful or not.
+	 */
+	memset(dev_conf_info, 0, sizeof(struct rte_eth_dev_conf_info));
+
+	dev_conf_info->rx_offloads = dev->data->dev_conf.rxmode.offloads;
+	dev_conf_info->tx_offloads = dev->data->dev_conf.txmode.offloads;
+
+	return 0;
+}
+
 int
 rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask,
 				 uint32_t *ptypes, int num)
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index d2b27c351f..70a2db550f 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -1587,6 +1587,15 @@ struct rte_eth_dev_info {
 	void *reserved_ptrs[2];   /**< Reserved for future fields */
 };
 
+/**
+ * Ethernet device configuration information structure.
+ * Used to retrieve information about configured device.
+ */
+struct rte_eth_dev_conf_info {
+	uint64_t rx_offloads; /**rxmode offloads */
+	uint64_t tx_offloads; /**txmode offloads */
+};
+
 /**
  * RX/TX queue states
  */
@@ -3058,6 +3067,23 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
  */
 int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
 
+/**
+ * Retrieve the contextual information of an Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param dev_conf_info
+ *   A pointer to a structure of type *rte_eth_dev_conf_info* to be filled with
+ *   the contextual information of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
+ *   - (-ENODEV) if *port_id* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int rte_eth_dev_conf_info_get(uint16_t port_id,
+				struct rte_eth_dev_conf_info *dev_conf_info);
+
 /**
  * Retrieve the firmware version of a device.
  *
-- 
2.25.1

[dpdk-stable] [PATCH v6 2/2] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

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: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
 app/test-pmd/testpmd.c | 33 +++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h |  2 ++
 app/test-pmd/util.c    | 15 +++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 6cbe9ba3c8..9a78805673 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_dev_conf_info dev_conf_info;
+			int k;
+
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2498,6 +2501,36 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+			/* get offloads */
+			if (0 !=
+				eth_dev_conf_info_get_print_err(pi,
+							&dev_conf_info)) {
+				rte_exit(EXIT_FAILURE,
+				    "rte_eth_dev_conf_info_get() failed\n");
+				return -1;
+			}
+			/* Apply Rx offloads configuration */
+			if (dev_conf_info.rx_offloads !=
+				port->dev_conf.rxmode.offloads) {
+				port->dev_conf.rxmode.offloads =
+					dev_conf_info.rx_offloads;
+				for (k = 0;
+				     k < port->dev_info.max_rx_queues;
+				     k++)
+					port->rx_conf[k].offloads =
+						dev_conf_info.rx_offloads;
+			}
+			/* Apply Tx offloads configuration */
+			if (dev_conf_info.tx_offloads !=
+				port->dev_conf.txmode.offloads) {
+				port->dev_conf.txmode.offloads =
+					dev_conf_info.tx_offloads;
+				for (k = 0;
+				     k < port->dev_info.max_tx_queues;
+				     k++)
+					port->tx_conf[k].offloads =
+						dev_conf_info.tx_offloads;
+			}
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 16a3598e48..4cf69a17a3 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -950,6 +950,8 @@ void show_gro(portid_t port_id);
 void setup_gso(const char *mode, portid_t port_id);
 int eth_dev_info_get_print_err(uint16_t port_id,
 			struct rte_eth_dev_info *dev_info);
+int eth_dev_conf_info_get_print_err(uint16_t port_id,
+			struct rte_eth_dev_conf_info *dev_info);
 void eth_set_promisc_mode(uint16_t port_id, int enable);
 void eth_set_allmulticast_mode(uint16_t port, int enable);
 int eth_link_get_nowait_print_err(uint16_t port_id, struct rte_eth_link *link);
diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
index 5dd7157947..c6992fba7a 100644
--- a/app/test-pmd/util.c
+++ b/app/test-pmd/util.c
@@ -440,6 +440,21 @@ eth_dev_info_get_print_err(uint16_t port_id,
 	return ret;
 }
 
+int
+eth_dev_conf_info_get_print_err(uint16_t port_id,
+				struct rte_eth_dev_conf_info *dev_conf_info)
+{
+	int ret;
+
+	ret = rte_eth_dev_conf_info_get(port_id, dev_conf_info);
+	if (ret != 0)
+		fprintf(stderr,
+			"Error during getting device config (port %u) info: %s\n",
+			port_id, strerror(-ret));
+
+	return ret;
+}
+
 void
 eth_set_promisc_mode(uint16_t port, int enable)
 {
-- 
2.25.1

[dpdk-stable] [PATCH v6 2/2] app/testpmd: fix testpmd doesn't show RSS hash offload

[Jie Wang]

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: stable@dpdk.org

Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
 app/test-pmd/testpmd.c | 33 +++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.h |  2 ++
 app/test-pmd/util.c    | 15 +++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 6cbe9ba3c8..9a78805673 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_dev_conf_info dev_conf_info;
+			int k;
+
 			port->need_reconfig = 0;
 
 			if (flow_isolate_all) {
@@ -2498,6 +2501,36 @@ start_port(portid_t pid)
 				port->need_reconfig = 1;
 				return -1;
 			}
+			/* get offloads */
+			if (0 !=
+				eth_dev_conf_info_get_print_err(pi,
+							&dev_conf_info)) {
+				rte_exit(EXIT_FAILURE,
+				    "rte_eth_dev_conf_info_get() failed\n");
+				return -1;
+			}
+			/* Apply Rx offloads configuration */
+			if (dev_conf_info.rx_offloads !=
+				port->dev_conf.rxmode.offloads) {
+				port->dev_conf.rxmode.offloads =
+					dev_conf_info.rx_offloads;
+				for (k = 0;
+				     k < port->dev_info.max_rx_queues;
+				     k++)
+					port->rx_conf[k].offloads =
+						dev_conf_info.rx_offloads;
+			}
+			/* Apply Tx offloads configuration */
+			if (dev_conf_info.tx_offloads !=
+				port->dev_conf.txmode.offloads) {
+				port->dev_conf.txmode.offloads =
+					dev_conf_info.tx_offloads;
+				for (k = 0;
+				     k < port->dev_info.max_tx_queues;
+				     k++)
+					port->tx_conf[k].offloads =
+						dev_conf_info.tx_offloads;
+			}
 		}
 		if (port->need_reconfig_queues > 0) {
 			port->need_reconfig_queues = 0;
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 16a3598e48..4cf69a17a3 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -950,6 +950,8 @@ void show_gro(portid_t port_id);
 void setup_gso(const char *mode, portid_t port_id);
 int eth_dev_info_get_print_err(uint16_t port_id,
 			struct rte_eth_dev_info *dev_info);
+int eth_dev_conf_info_get_print_err(uint16_t port_id,
+			struct rte_eth_dev_conf_info *dev_info);
 void eth_set_promisc_mode(uint16_t port_id, int enable);
 void eth_set_allmulticast_mode(uint16_t port, int enable);
 int eth_link_get_nowait_print_err(uint16_t port_id, struct rte_eth_link *link);
diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
index 5dd7157947..c6992fba7a 100644
--- a/app/test-pmd/util.c
+++ b/app/test-pmd/util.c
@@ -440,6 +440,21 @@ eth_dev_info_get_print_err(uint16_t port_id,
 	return ret;
 }
 
+int
+eth_dev_conf_info_get_print_err(uint16_t port_id,
+				struct rte_eth_dev_conf_info *dev_conf_info)
+{
+	int ret;
+
+	ret = rte_eth_dev_conf_info_get(port_id, dev_conf_info);
+	if (ret != 0)
+		fprintf(stderr,
+			"Error during getting device config (port %u) info: %s\n",
+			port_id, strerror(-ret));
+
+	return ret;
+}
+
 void
 eth_set_promisc_mode(uint16_t port, int enable)
 {
-- 
2.25.1