DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1] app/testpmd: fix dynamic config error for max-pkt-len
@ 2020-12-22  8:13 Steve Yang
  2020-12-23  2:27 ` Li, Xiaoyun
  2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
  0 siblings, 2 replies; 38+ messages in thread
From: Steve Yang @ 2020-12-22  8:13 UTC (permalink / raw)
  To: dev; +Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, Steve Yang

When 'max-pkt-len' value caused the 'rx_offloads' flag change, the all
offloads of rx queues ('rx_conf[qid].offloads') weren't synchronized,
that will cause the offloads check failed with 'rx_queue_offload_capa'
within 'rte_eth_rx_queue_setup'.

Apply rx offloads configuration once it changed when 'max-pkt-len'
command parsed.

Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 app/test-pmd/cmdline.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 2ccbaa039e..d72a40d7de 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1902,7 +1902,23 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 				rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
 			else
 				rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
-			port->dev_conf.rxmode.offloads = rx_offloads;
+
+			if (rx_offloads != port->dev_conf.rxmode.offloads) {
+				uint16_t k;
+				int ret;
+
+				port->dev_conf.rxmode.offloads = rx_offloads;
+				/* Apply Rx offloads configuration */
+				ret = eth_dev_info_get_print_err(pid,
+							&port->dev_info);
+				if (ret != 0)
+					rte_exit(EXIT_FAILURE,
+					    "rte_eth_dev_info_get() failed\n");
+
+				for (k = 0;
+				     k < port->dev_info.nb_rx_queues; k++)
+					port->rx_conf[k].offloads = rx_offloads;
+			}
 		} else {
 			printf("Unknown parameter\n");
 			return;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v1] app/testpmd: fix dynamic config error for max-pkt-len
  2020-12-22  8:13 [dpdk-dev] [PATCH v1] app/testpmd: fix dynamic config error for max-pkt-len Steve Yang
@ 2020-12-23  2:27 ` Li, Xiaoyun
  2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
  1 sibling, 0 replies; 38+ messages in thread
From: Li, Xiaoyun @ 2020-12-23  2:27 UTC (permalink / raw)
  To: Yang, SteveX, dev
  Cc: Lu, Wenzhuo, Xing, Beilei, Iremonger, Bernard, Yang, SteveX

Hi
Comments inline

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Steve Yang
> Sent: Tuesday, December 22, 2020 16:14
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>
> Subject: [dpdk-dev] [PATCH v1] app/testpmd: fix dynamic config error for max-
> pkt-len
> 
> When 'max-pkt-len' value caused the 'rx_offloads' flag change, the all offloads
> of rx queues ('rx_conf[qid].offloads') weren't synchronized, that will cause the
> offloads check failed with 'rx_queue_offload_capa'
> within 'rte_eth_rx_queue_setup'.
> 
> Apply rx offloads configuration once it changed when 'max-pkt-len'
> command parsed.

Grammar and tense inconsistence...
You can phrase like the following:

Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while rx_conf.offloads of each queue still kept the old value.
It would cause the failure of offloads check in ''rte_eth_rx_queue_setup'.

This patch applied rx offloads configuration for each queue once it changed.

> 
> Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>  app/test-pmd/cmdline.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 2ccbaa039e..d72a40d7de 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1902,7 +1902,23 @@ cmd_config_max_pkt_len_parsed(void
> *parsed_result,
>  				rx_offloads |=
> DEV_RX_OFFLOAD_JUMBO_FRAME;
>  			else
>  				rx_offloads &=
> ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> -			port->dev_conf.rxmode.offloads = rx_offloads;

I understand what you're doing here. But I think there's a better place to do this.
This config cmd will call init_port_config() later. And rxtx_port_config() will be called in it.
I think you should do this in rxtx_port_config().
Check if rx_conf  is equal to dev_conf, and if it's not consistent, apply dev_conf.

Although if you insist on your way doing this, there're some issues too. See below.

> +
> +			if (rx_offloads != port->dev_conf.rxmode.offloads) {
> +				uint16_t k;
> +				int ret;
> +
> +				port->dev_conf.rxmode.offloads = rx_offloads;
> +				/* Apply Rx offloads configuration */
> +				ret = eth_dev_info_get_print_err(pid,
> +							&port->dev_info);
> +				if (ret != 0)
> +					rte_exit(EXIT_FAILURE,
> +					    "rte_eth_dev_info_get() failed\n");

rte_exit if for the main process of the application not for cmdline.
Because rte_exit will just terminate the application and return to the shell. You wouldn't want that.
You only needs to 'return;' or maybe printf a error message and return.

> +
> +				for (k = 0;

Why are you using 'k'? There's no problem of this just looks a bit weird. There's no 'i' used in this function so why not just use 'i'.

> +				     k < port->dev_info.nb_rx_queues; k++)
> +					port->rx_conf[k].offloads =
> rx_offloads;
> +			}
>  		} else {
>  			printf("Unknown parameter\n");
>  			return;
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error
  2020-12-22  8:13 [dpdk-dev] [PATCH v1] app/testpmd: fix dynamic config error for max-pkt-len Steve Yang
  2020-12-23  2:27 ` Li, Xiaoyun
@ 2020-12-23  8:51 ` Steve Yang
  2020-12-23  9:00   ` Li, Xiaoyun
                     ` (3 more replies)
  1 sibling, 4 replies; 38+ messages in thread
From: Steve Yang @ 2020-12-23  8:51 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, xiaoyun.li,
	qiming.yang, Steve Yang

The offloads of 'tx/rx_conf' didn't keep up with the corresponding
offloads of 'dev_conf', it would cause the configuration invalid.

For example:
Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
rx_conf.offloads of each queue still kept the old value.
It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.

This patch applied tx/rx offloads configuration for each queue
once it changed.

Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
v2:
 * moved the update logic to 'rxtx_port_config';
 * added the 'tx_conf' part;
 * optimized the 'default' assignment;
---
 app/test-pmd/testpmd.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 33a060dffd..6ee28e3797 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3288,9 +3288,11 @@ rxtx_port_config(struct rte_port *port)
 
 	for (qid = 0; qid < nb_rxq; qid++) {
 		offloads = port->rx_conf[qid].offloads;
-		port->rx_conf[qid] = port->dev_info.default_rxconf;
-		if (offloads != 0)
-			port->rx_conf[qid].offloads = offloads;
+		if (offloads != port->dev_conf.rxmode.offloads)
+			port->rx_conf[qid].offloads =
+				port->dev_conf.rxmode.offloads;
+		if (!offloads)
+			port->rx_conf[qid] = port->dev_info.default_rxconf;
 
 		/* Check if any Rx parameters have been passed */
 		if (rx_pthresh != RTE_PMD_PARAM_UNSET)
@@ -3313,9 +3315,11 @@ rxtx_port_config(struct rte_port *port)
 
 	for (qid = 0; qid < nb_txq; qid++) {
 		offloads = port->tx_conf[qid].offloads;
-		port->tx_conf[qid] = port->dev_info.default_txconf;
-		if (offloads != 0)
-			port->tx_conf[qid].offloads = offloads;
+		if (offloads != port->dev_conf.txmode.offloads)
+			port->tx_conf[qid].offloads =
+				port->dev_conf.txmode.offloads;
+		if (!offloads)
+			port->tx_conf[qid] = port->dev_info.default_txconf;
 
 		/* Check if any Tx parameters have been passed */
 		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error
  2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
@ 2020-12-23  9:00   ` Li, Xiaoyun
  2021-01-13  8:13   ` Chen, BoX C
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 38+ messages in thread
From: Li, Xiaoyun @ 2020-12-23  9:00 UTC (permalink / raw)
  To: Yang, SteveX, dev
  Cc: Lu, Wenzhuo, Xing, Beilei, Iremonger, Bernard, Yang, Qiming,
	Yang, SteveX

Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

> -----Original Message-----
> From: Steve Yang <stevex.yang@intel.com>
> Sent: Wednesday, December 23, 2020 16:52
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>
> Subject: [PATCH v2] app/testpmd: fix dynamic config error
> 
> The offloads of 'tx/rx_conf' didn't keep up with the corresponding offloads of
> 'dev_conf', it would cause the configuration invalid.
> 
> For example:
> Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
> rx_conf.offloads of each queue still kept the old value.
> It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.
> 
> This patch applied tx/rx offloads configuration for each queue once it changed.
> 
> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
> v2:
>  * moved the update logic to 'rxtx_port_config';
>  * added the 'tx_conf' part;
>  * optimized the 'default' assignment;
> ---
>  app/test-pmd/testpmd.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 33a060dffd..6ee28e3797 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3288,9 +3288,11 @@ rxtx_port_config(struct rte_port *port)
> 
>  	for (qid = 0; qid < nb_rxq; qid++) {
>  		offloads = port->rx_conf[qid].offloads;
> -		port->rx_conf[qid] = port->dev_info.default_rxconf;
> -		if (offloads != 0)
> -			port->rx_conf[qid].offloads = offloads;
> +		if (offloads != port->dev_conf.rxmode.offloads)
> +			port->rx_conf[qid].offloads =
> +				port->dev_conf.rxmode.offloads;
> +		if (!offloads)
> +			port->rx_conf[qid] = port->dev_info.default_rxconf;
> 
>  		/* Check if any Rx parameters have been passed */
>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -3313,9
> +3315,11 @@ rxtx_port_config(struct rte_port *port)
> 
>  	for (qid = 0; qid < nb_txq; qid++) {
>  		offloads = port->tx_conf[qid].offloads;
> -		port->tx_conf[qid] = port->dev_info.default_txconf;
> -		if (offloads != 0)
> -			port->tx_conf[qid].offloads = offloads;
> +		if (offloads != port->dev_conf.txmode.offloads)
> +			port->tx_conf[qid].offloads =
> +				port->dev_conf.txmode.offloads;
> +		if (!offloads)
> +			port->tx_conf[qid] = port->dev_info.default_txconf;
> 
>  		/* Check if any Tx parameters have been passed */
>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error
  2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
  2020-12-23  9:00   ` Li, Xiaoyun
@ 2021-01-13  8:13   ` Chen, BoX C
  2021-01-19 15:44   ` Ferruh Yigit
  2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
  3 siblings, 0 replies; 38+ messages in thread
From: Chen, BoX C @ 2021-01-13  8:13 UTC (permalink / raw)
  To: Yang, SteveX, dev
  Cc: Lu, Wenzhuo, Xing, Beilei, Iremonger, Bernard, Li, Xiaoyun, Yang,
	Qiming, Yang, SteveX

Tested-by:  Chen, BoX C <BoX.C.Chen@intel.com>

Regards,
Chen Bo

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Steve Yang
> Sent: December 23, 2020 16:52
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; Yang,
> Qiming <qiming.yang@intel.com>; Yang, SteveX <stevex.yang@intel.com>
> Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error
> 
> The offloads of 'tx/rx_conf' didn't keep up with the corresponding offloads
> of 'dev_conf', it would cause the configuration invalid.
> 
> For example:
> Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
> rx_conf.offloads of each queue still kept the old value.
> It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.
> 
> This patch applied tx/rx offloads configuration for each queue once it
> changed.
> 
> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
> v2:
>  * moved the update logic to 'rxtx_port_config';
>  * added the 'tx_conf' part;
>  * optimized the 'default' assignment;
> ---
>  app/test-pmd/testpmd.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 33a060dffd..6ee28e3797 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3288,9 +3288,11 @@ rxtx_port_config(struct rte_port *port)
> 
>  	for (qid = 0; qid < nb_rxq; qid++) {
>  		offloads = port->rx_conf[qid].offloads;
> -		port->rx_conf[qid] = port->dev_info.default_rxconf;
> -		if (offloads != 0)
> -			port->rx_conf[qid].offloads = offloads;
> +		if (offloads != port->dev_conf.rxmode.offloads)
> +			port->rx_conf[qid].offloads =
> +				port->dev_conf.rxmode.offloads;
> +		if (!offloads)
> +			port->rx_conf[qid] = port->dev_info.default_rxconf;
> 
>  		/* Check if any Rx parameters have been passed */
>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -3313,9
> +3315,11 @@ rxtx_port_config(struct rte_port *port)
> 
>  	for (qid = 0; qid < nb_txq; qid++) {
>  		offloads = port->tx_conf[qid].offloads;
> -		port->tx_conf[qid] = port->dev_info.default_txconf;
> -		if (offloads != 0)
> -			port->tx_conf[qid].offloads = offloads;
> +		if (offloads != port->dev_conf.txmode.offloads)
> +			port->tx_conf[qid].offloads =
> +				port->dev_conf.txmode.offloads;
> +		if (!offloads)
> +			port->tx_conf[qid] = port->dev_info.default_txconf;
> 
>  		/* Check if any Tx parameters have been passed */
>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error
  2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
  2020-12-23  9:00   ` Li, Xiaoyun
  2021-01-13  8:13   ` Chen, BoX C
@ 2021-01-19 15:44   ` Ferruh Yigit
  2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
  3 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-19 15:44 UTC (permalink / raw)
  To: Steve Yang, dev
  Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, xiaoyun.li, qiming.yang

On 12/23/2020 8:51 AM, Steve Yang wrote:
> The offloads of 'tx/rx_conf' didn't keep up with the corresponding
> offloads of 'dev_conf', it would cause the configuration invalid.
> 
> For example:
> Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
> rx_conf.offloads of each queue still kept the old value.
> It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.
> 

Can you please give some details how can I reproduce the issue?

> This patch applied tx/rx offloads configuration for each queue
> once it changed.
> 
> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
> v2:
>   * moved the update logic to 'rxtx_port_config';
>   * added the 'tx_conf' part;
>   * optimized the 'default' assignment;
> ---
>   app/test-pmd/testpmd.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 33a060dffd..6ee28e3797 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3288,9 +3288,11 @@ rxtx_port_config(struct rte_port *port)
>   
>   	for (qid = 0; qid < nb_rxq; qid++) {
>   		offloads = port->rx_conf[qid].offloads;
> -		port->rx_conf[qid] = port->dev_info.default_rxconf;
> -		if (offloads != 0)
> -			port->rx_conf[qid].offloads = offloads;
> +		if (offloads != port->dev_conf.rxmode.offloads)
> +			port->rx_conf[qid].offloads =
> +				port->dev_conf.rxmode.offloads;

Isn't 'rxmode.offloads' for the port, but the 'rx_conf[qid].offloads' for the 
queue, can this cause problem if the queue level offloads used?

> +		if (!offloads)
> +			port->rx_conf[qid] = port->dev_info.default_rxconf;

Can you please explain this, why the default config is used, only if the 
'offload' is zero?
The original code is always using the default config, but overwriting the 
'offloads' if needed.

>   
>   		/* Check if any Rx parameters have been passed */
>   		if (rx_pthresh != RTE_PMD_PARAM_UNSET)
> @@ -3313,9 +3315,11 @@ rxtx_port_config(struct rte_port *port)
>   
>   	for (qid = 0; qid < nb_txq; qid++) {
>   		offloads = port->tx_conf[qid].offloads;
> -		port->tx_conf[qid] = port->dev_info.default_txconf;
> -		if (offloads != 0)
> -			port->tx_conf[qid].offloads = offloads;
> +		if (offloads != port->dev_conf.txmode.offloads)
> +			port->tx_conf[qid].offloads =
> +				port->dev_conf.txmode.offloads;
> +		if (!offloads)
> +			port->tx_conf[qid] = port->dev_info.default_txconf;
>   
>   		/* Check if any Tx parameters have been passed */
>   		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors
  2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
                     ` (2 preceding siblings ...)
  2021-01-19 15:44   ` Ferruh Yigit
@ 2021-01-22  9:01   ` Steve Yang
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
                       ` (3 more replies)
  3 siblings, 4 replies; 38+ messages in thread
From: Steve Yang @ 2021-01-22  9:01 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, stevex.yang, qiming.yang

Here fixed 3 issues for 'max-pkt-len':
1. When cmdline option '--max-pkt-len' set the value less then
   '1500 + overhead', the app/testpmd will force to resize the 'max-pkt-len'
   to '1500 + overhead'. However, the user really want to configure
   'max-pkt-len' to a specified value (< 1500 + overhead);

2. If the large value of '--max-pkt-len' gave (e.g.: 8000), and user want to
   reset the value to a small one (e.g.: 1400), it will became invalid due to
   JUMBO_FRAME offload state doesn't change before port started;

3. When rx/tx queue offloads capabilities aren't specified, the rx/tx queue
   setup will be failed once the port offloads changed.

---
v3:
 * rebased code to latest;
 * splited to 3 commits;

v2:
 * moved the update logic to 'rxtx_port_config';
 * added the 'tx_conf' part;
 * optimized the 'default' assignment;
---

Steve Yang (3):
  ethdev: fix MTU doesn't update when jumbo frame disabled
  app/testpmd: fix max-pkt-len option invalid
  app/testpmd: fix dynamic config error

 app/test-pmd/cmdline.c         |  1 +
 app/test-pmd/parameters.c      |  1 +
 app/test-pmd/testpmd.c         | 59 +++++++++++++++++++++++-----------
 app/test-pmd/testpmd.h         |  2 ++
 lib/librte_ethdev/rte_ethdev.c |  8 ++---
 5 files changed, 48 insertions(+), 23 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled
  2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
@ 2021-01-22  9:01     ` Steve Yang
  2021-01-25  7:12       ` Huisong Li
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: fix max-pkt-len option invalid Steve Yang
                       ` (2 subsequent siblings)
  3 siblings, 1 reply; 38+ messages in thread
From: Steve Yang @ 2021-01-22  9:01 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, stevex.yang, qiming.yang

The MTU value should be updated to 'max_rx_pkt_len - overhead'
no matter if the JUMBO FRAME offload enabled. If not update this MTU,
use will get the wrong MTU info via some command.
E.g.: 'show port info all' in testpmd tool.

Actually, the 'max_rx_pkt_len' has been used for other purposes in many
places now, even though the 'max_rx_pkt_len' is expected 'Only used if
JUMBO_FRAME enabled'.

For examples,
'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.

Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index daf5f24f7e..42857e3b67 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			ret = -EINVAL;
 			goto rollback;
 		}
-
-		/* Scale the MTU size to adapt max_rx_pkt_len */
-		dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
-				overhead_len;
 	} else {
 		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
 		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
@@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 						RTE_ETHER_MTU + overhead_len;
 	}
 
+	/* Scale the MTU size to adapt max_rx_pkt_len */
+	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
+				overhead_len;
+
 	/*
 	 * If LRO is enabled, check that the maximum aggregated packet
 	 * size is supported by the configured device.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v3 2/3] app/testpmd: fix max-pkt-len option invalid
  2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
@ 2021-01-22  9:01     ` Steve Yang
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error Steve Yang
  2021-01-25  8:32     ` [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors Steve Yang
  3 siblings, 0 replies; 38+ messages in thread
From: Steve Yang @ 2021-01-22  9:01 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, stevex.yang, qiming.yang

Moved the setting of 'DEV_RX_OFFLOAD_JUMBO_FRAME' from
'cmd_config_max_pkt_len_parsed()' to 'init_config()' caused fail the case
where 'max_rx_pkt_len' is changed from the command line via
"port config all max-pkt-len".

The 'init_config()' function is only called when testpmd is started,
but the DEV_RX_OFFLOAD_JUMBO_FRAME setting needs to be recomputed whenever
'max_rx_pkt_len' changes.

Define the 'update_jumbo_frame_offload()' function for both 'init_config()'
and 'cmd_config_max_pkt_len_parsed()', and detect if 'max_rx_pkt_len'
should be update to 1500 + overhead as default configuration.

Fixes: 761c4d6690 ("app/testpmd: fix max Rx packet length for VLAN packets")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 app/test-pmd/cmdline.c    |  1 +
 app/test-pmd/parameters.c |  1 +
 app/test-pmd/testpmd.c    | 47 +++++++++++++++++++++++++--------------
 app/test-pmd/testpmd.h    |  2 ++
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 89034c8b72..600e0f8943 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1901,6 +1901,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 			printf("Unknown parameter\n");
 			return;
 		}
+		update_jumbo_frame_offload(port, false);
 	}
 
 	init_port_config();
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index df5eb10d84..1c63156ddd 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -833,6 +833,7 @@ launch_args_parse(int argc, char** argv)
 						 "total-num-mbufs should be > 1024\n");
 			}
 			if (!strcmp(lgopts[opt_idx].name, "max-pkt-len")) {
+				default_max_pktlen = false;
 				n = atoi(optarg);
 				if (n >= RTE_ETHER_MIN_LEN)
 					rx_mode.max_rx_pkt_len = (uint32_t) n;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c256e719ae..a2c9aad960 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -531,6 +531,8 @@ uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN;
 /* Holds the registered mbuf dynamic flags names. */
 char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
 
+bool default_max_pktlen = true;
+
 /*
  * Helper function to check if socket is already discovered.
  * If yes, return positive value. If not, return zero.
@@ -1410,7 +1412,6 @@ init_config(void)
 	struct rte_gro_param gro_param;
 	uint32_t gso_types;
 	uint16_t data_size;
-	uint16_t eth_overhead;
 	bool warning = 0;
 	int k;
 	int ret;
@@ -1447,22 +1448,7 @@ init_config(void)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_dev_info_get() failed\n");
 
-		/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
-		if (port->dev_info.max_mtu != UINT16_MAX &&
-		    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
-			eth_overhead = port->dev_info.max_rx_pktlen -
-				port->dev_info.max_mtu;
-		else
-			eth_overhead =
-				RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
-
-		if (port->dev_conf.rxmode.max_rx_pkt_len <=
-			(uint32_t)(RTE_ETHER_MTU + eth_overhead))
-			port->dev_conf.rxmode.max_rx_pkt_len =
-					RTE_ETHER_MTU + eth_overhead;
-		else
-			port->dev_conf.rxmode.offloads |=
-					DEV_RX_OFFLOAD_JUMBO_FRAME;
+		update_jumbo_frame_offload(port, default_max_pktlen);
 
 		if (!(port->dev_info.tx_offload_capa &
 		      DEV_TX_OFFLOAD_MBUF_FAST_FREE))
@@ -3358,6 +3344,33 @@ rxtx_port_config(struct rte_port *port)
 	}
 }
 
+void
+update_jumbo_frame_offload(struct rte_port *port, bool def_max_pktlen)
+{
+	uint16_t eth_overhead;
+
+	/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
+	if (port->dev_info.max_mtu != UINT16_MAX &&
+	    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
+		eth_overhead = port->dev_info.max_rx_pktlen -
+			       port->dev_info.max_mtu;
+	else
+		eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+	if (port->dev_conf.rxmode.max_rx_pkt_len <=
+	    (uint32_t)(RTE_ETHER_MTU + eth_overhead)) {
+		/*
+		 * If command line option doesn't include --max-pkt-len,
+		 * the default max_rx_pkt_len should be set to 1500 + overhead.
+		 */
+		if (def_max_pktlen)
+			port->dev_conf.rxmode.max_rx_pkt_len =
+						RTE_ETHER_MTU + eth_overhead;
+		port->dev_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+	} else
+		port->dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+}
+
 void
 init_port_config(void)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 5f23162107..aac7b69735 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -304,6 +304,7 @@ extern cmdline_parse_inst_t cmd_show_set_raw_all;
 
 extern uint16_t mempool_flags;
 
+extern bool default_max_pktlen;
 /**
  * Forwarding Configuration
  *
@@ -1005,6 +1006,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id, __rte_unused uint16_t queue,
 			 __rte_unused void *user_param);
 void add_tx_dynf_callback(portid_t portid);
 void remove_tx_dynf_callback(portid_t portid);
+void update_jumbo_frame_offload(struct rte_port *port, bool def_max_pktlen);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error
  2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: fix max-pkt-len option invalid Steve Yang
@ 2021-01-22  9:01     ` Steve Yang
  2021-01-22 17:04       ` Ferruh Yigit
  2021-01-25  8:32     ` [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors Steve Yang
  3 siblings, 1 reply; 38+ messages in thread
From: Steve Yang @ 2021-01-22  9:01 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, stevex.yang, qiming.yang

The offloads of 'tx/rx_conf' didn't keep up with the corresponding
offloads of 'dev_conf' if rx queue capability was 0, it would cause
the configuration invalid.

For example:
Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
rx_conf.offloads of each queue still kept the old value.
It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.

This patch applied tx/rx offloads configuration for each queue
once it changed and corresponding tx/rx queue capability was 0.

Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 app/test-pmd/testpmd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index a2c9aad960..8307c7f9e9 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3296,7 +3296,11 @@ rxtx_port_config(struct rte_port *port)
 	for (qid = 0; qid < nb_rxq; qid++) {
 		offloads = port->rx_conf[qid].offloads;
 		port->rx_conf[qid] = port->dev_info.default_rxconf;
-		if (offloads != 0)
+		if (port->dev_info.rx_queue_offload_capa == 0 &&
+		    offloads != port->dev_conf.rxmode.offloads)
+			port->rx_conf[qid].offloads =
+				port->dev_conf.rxmode.offloads;
+		else if (offloads != 0)
 			port->rx_conf[qid].offloads = offloads;
 
 		/* Check if any Rx parameters have been passed */
@@ -3321,7 +3325,11 @@ rxtx_port_config(struct rte_port *port)
 	for (qid = 0; qid < nb_txq; qid++) {
 		offloads = port->tx_conf[qid].offloads;
 		port->tx_conf[qid] = port->dev_info.default_txconf;
-		if (offloads != 0)
+		if (port->dev_info.tx_queue_offload_capa == 0 &&
+		    offloads != port->dev_conf.txmode.offloads)
+			port->tx_conf[qid].offloads =
+				port->dev_conf.txmode.offloads;
+		else if (offloads != 0)
 			port->tx_conf[qid].offloads = offloads;
 
 		/* Check if any Tx parameters have been passed */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error Steve Yang
@ 2021-01-22 17:04       ` Ferruh Yigit
  2021-01-22 17:15         ` Ferruh Yigit
  0 siblings, 1 reply; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-22 17:04 UTC (permalink / raw)
  To: Steve Yang, dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas,
	andrew.rybchenko, qiming.yang

On 1/22/2021 9:01 AM, Steve Yang wrote:
> The offloads of 'tx/rx_conf' didn't keep up with the corresponding
> offloads of 'dev_conf' if rx queue capability was 0, it would cause
> the configuration invalid.
> 
> For example:
> Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
> rx_conf.offloads of each queue still kept the old value.
> It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.
> 
> This patch applied tx/rx offloads configuration for each queue
> once it changed and corresponding tx/rx queue capability was 0.
> 
> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>   app/test-pmd/testpmd.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index a2c9aad960..8307c7f9e9 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3296,7 +3296,11 @@ rxtx_port_config(struct rte_port *port)
>   	for (qid = 0; qid < nb_rxq; qid++) {
>   		offloads = port->rx_conf[qid].offloads;
>   		port->rx_conf[qid] = port->dev_info.default_rxconf;
> -		if (offloads != 0)
> +		if (port->dev_info.rx_queue_offload_capa == 0 &&
> +		    offloads != port->dev_conf.rxmode.offloads)
> +			port->rx_conf[qid].offloads =
> +				port->dev_conf.rxmode.offloads;
> +		else if (offloads != 0)
>   			port->rx_conf[qid].offloads = offloads;

I am still concerned to use port offloads to set the queue specific offloads, 
this may lead unexpected result.

Below is what Steve provided as reproduce steps [1], I think that is application 
(testpmd) miss-configuration to update 'max-pkt-len' but not update the 
JUMBO_FRAME offload flag and MTU accordingly.

What do you think update the JUMBO_FRAME offload flag (both for port and queues) 
according and set MTU according on the testpmd command where 'max-pkt-len' is 
set? This is more like your first version.



[1]
----------------------------------------------
# x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i 
--max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
testpmd>  set verbose 3
testpmd>  start
testpmd>  stop
testpmd>  port stop all
testpmd>  port config all max-pkt-len 1518
testpmd> port start all
Configuring Port 0 (socket 1)
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be within 
per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
Fail to configure port 0 rx queues//<-- Fail error info;
------------------------------------------------


>   
>   		/* Check if any Rx parameters have been passed */
> @@ -3321,7 +3325,11 @@ rxtx_port_config(struct rte_port *port)
>   	for (qid = 0; qid < nb_txq; qid++) {
>   		offloads = port->tx_conf[qid].offloads;
>   		port->tx_conf[qid] = port->dev_info.default_txconf;
> -		if (offloads != 0)
> +		if (port->dev_info.tx_queue_offload_capa == 0 &&
> +		    offloads != port->dev_conf.txmode.offloads)
> +			port->tx_conf[qid].offloads =
> +				port->dev_conf.txmode.offloads;
> +		else if (offloads != 0)
>   			port->tx_conf[qid].offloads = offloads;
>   
>   		/* Check if any Tx parameters have been passed */
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error
  2021-01-22 17:04       ` Ferruh Yigit
@ 2021-01-22 17:15         ` Ferruh Yigit
  0 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-22 17:15 UTC (permalink / raw)
  To: Steve Yang, dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas,
	andrew.rybchenko, qiming.yang

On 1/22/2021 5:04 PM, Ferruh Yigit wrote:
> On 1/22/2021 9:01 AM, Steve Yang wrote:
>> The offloads of 'tx/rx_conf' didn't keep up with the corresponding
>> offloads of 'dev_conf' if rx queue capability was 0, it would cause
>> the configuration invalid.
>>
>> For example:
>> Configuring 'max-pkt-len' would change 'rx_offloads' in dev_conf while
>> rx_conf.offloads of each queue still kept the old value.
>> It would cause the failure of offloads check in 'rte_eth_rx_queue_setup'.
>>
>> This patch applied tx/rx offloads configuration for each queue
>> once it changed and corresponding tx/rx queue capability was 0.
>>
>> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
>>
>> Signed-off-by: Steve Yang <stevex.yang@intel.com>
>> ---
>>   app/test-pmd/testpmd.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index a2c9aad960..8307c7f9e9 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3296,7 +3296,11 @@ rxtx_port_config(struct rte_port *port)
>>       for (qid = 0; qid < nb_rxq; qid++) {
>>           offloads = port->rx_conf[qid].offloads;
>>           port->rx_conf[qid] = port->dev_info.default_rxconf;
>> -        if (offloads != 0)
>> +        if (port->dev_info.rx_queue_offload_capa == 0 &&
>> +            offloads != port->dev_conf.rxmode.offloads)
>> +            port->rx_conf[qid].offloads =
>> +                port->dev_conf.rxmode.offloads;
>> +        else if (offloads != 0)
>>               port->rx_conf[qid].offloads = offloads;
> 
> I am still concerned to use port offloads to set the queue specific offloads, 
> this may lead unexpected result.
> 
> Below is what Steve provided as reproduce steps [1], I think that is application 
> (testpmd) miss-configuration to update 'max-pkt-len' but not update the 
> JUMBO_FRAME offload flag and MTU accordingly.
> 
> What do you think update the JUMBO_FRAME offload flag (both for port and queues) 
> according and set MTU according on the testpmd command where 'max-pkt-len' is 
> set? This is more like your first version.
> 
> 
> 
> [1]
> ----------------------------------------------
> # x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i 
> --max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  start
> testpmd>  stop
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518
> testpmd> port start all
> Configuring Port 0 (socket 1)
> Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be within 
> per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
> Fail to configure port 0 rx queues//<-- Fail error info;
> ------------------------------------------------
> 

Indeed first patch of this series solves the MTU problem with "port config all 
max-pkt-len" command (although it ignores 'max_rx_pkt_len' is only valid when 
JUMBO_FRAME flag is set)

And second patch updates the JUMBO_FRAME flag in the command function, same as I 
suggested above.
Only if second patch extended to update JUMBO_FRAME flag for all queues in 
'update_jumbo_frame_offload()', this patch can be dropped, what do you think?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
@ 2021-01-25  7:12       ` Huisong Li
       [not found]         ` <DM6PR11MB43628A600BAAEC75FFF1343BF9BD9@DM6PR11MB4362.namprd11.prod.outlook.com>
  0 siblings, 1 reply; 38+ messages in thread
From: Huisong Li @ 2021-01-25  7:12 UTC (permalink / raw)
  To: Steve Yang, dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, qiming.yang, oulijun, huangdaode

Hi Steve,

In the current modification, the MTU is updated based on 
'max_rx_pkt_len' regardless of whether jumbo frame is enabled.

Now, MTU is correct when jumbo frmae is disabled. However, when jumbo 
frame is enabled, the MTU value may be inconsistent with

the definition of the enabled jumbo frame. Like:

1/ DEV_RX_OFFLOAD_JUMBO_FRAME is set;

2/ max_rx_pkt_len = 1200

3/ dev->data->mtu = 1200 - overhead_len(18) = 1182


In rte_eth_dev_configure API, the check for 'max_rx_pkt_len' is as follows:

if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {  //jumbo 
frame enabled
         if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) {
             xxxx
             goto rollback;
         } else if (*dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN*) {
             xxxx
             goto rollback;
         }
} else { //jumbo frame disabled

         if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
                     pktlen > RTE_ETHER_MTU + overhead_len)
                         /* Use default value */
dev->data->dev_conf.rxmode.max_rx_pkt_len =
                                                 RTE_ETHER_MTU + 
overhead_len;

}

Since the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME to enable jumbo 
frame, and the framework API needs to update

the MTU based on 'max_rx_pkt_len', but the framework API uses 
*RTE_ETHER_MIN_LEN(64)* to verify the boundary value of

'max_rx_pkt_len', instead of "RTE_ETHER_MTU + overhead_len".  As far as 
I know, if the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME

and 'max_rx_pkt_len' is 1200, the framework API or driver should return 
a failure. As mentioned in this patch set, the jumbo frame

offload is set only when 'max_rx_pkt_len' requested is greater than 
"RTE_ETHER_MTU + eth_overhead" in testpmd.

I really don't understand it.  How do you understand this behavior?

Thanks.


在 2021/1/22 17:01, Steve Yang 写道:
> The MTU value should be updated to 'max_rx_pkt_len - overhead'
> no matter if the JUMBO FRAME offload enabled. If not update this MTU,
> use will get the wrong MTU info via some command.
> E.g.: 'show port info all' in testpmd tool.
>
> Actually, the 'max_rx_pkt_len' has been used for other purposes in many
> places now, even though the 'max_rx_pkt_len' is expected 'Only used if
> JUMBO_FRAME enabled'.
>
> For examples,
> 'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.
>
> Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index daf5f24f7e..42857e3b67 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   			ret = -EINVAL;
>   			goto rollback;
>   		}
> -
> -		/* Scale the MTU size to adapt max_rx_pkt_len */
> -		dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> -				overhead_len;
>   	} else {
>   		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
>   		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
> @@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   						RTE_ETHER_MTU + overhead_len;
>   	}
>   
> +	/* Scale the MTU size to adapt max_rx_pkt_len */
> +	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> +				overhead_len;
> +
>   	/*
>   	 * If LRO is enabled, check that the maximum aggregated packet
>   	 * size is supported by the configured device.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors
  2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
                       ` (2 preceding siblings ...)
  2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error Steve Yang
@ 2021-01-25  8:32     ` Steve Yang
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
                         ` (2 more replies)
  3 siblings, 3 replies; 38+ messages in thread
From: Steve Yang @ 2021-01-25  8:32 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, qiming.yang, Steve Yang

Here fixed 3 issues for 'max-pkt-len':
1. When cmdline option '--max-pkt-len' set the value less then
   '1500 + overhead', the app/testpmd will force to resize the 'max-pkt-len'
   to '1500 + overhead'. However, the user really want to configure
   'max-pkt-len' to a specified value (< 1500 + overhead);

2. If the large value of '--max-pkt-len' gave (e.g.: 8000), and user want to
   reset the value to a small one (e.g.: 1400), it will became invalid due to
   JUMBO_FRAME offload state doesn't change before port started;

3. When rx/tx queue offloads capabilities aren't specified, the rx/tx queue
   setup will be failed once the port offloads changed.

---
v4:
 * combined testpmd patches;
 * updated the commit log for patch 2;
v3:
 * rebased code to latest;
 * splited to 3 commits;

v2:
 * moved the update logic to 'rxtx_port_config';
 * added the 'tx_conf' part;
 * optimized the 'default' assignment;
---

Steve Yang (2):
  ethdev: fix MTU doesn't update when jumbo frame disabled
  app/testpmd: fix max-pkt-len option invalid

 app/test-pmd/cmdline.c         |  1 +
 app/test-pmd/parameters.c      |  1 +
 app/test-pmd/testpmd.c         | 63 +++++++++++++++++++++++++---------
 app/test-pmd/testpmd.h         |  2 ++
 lib/librte_ethdev/rte_ethdev.c |  8 ++---
 5 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled
  2021-01-25  8:32     ` [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors Steve Yang
@ 2021-01-25  8:32       ` Steve Yang
  2021-01-25 12:41         ` Ferruh Yigit
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid Steve Yang
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
  2 siblings, 1 reply; 38+ messages in thread
From: Steve Yang @ 2021-01-25  8:32 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, qiming.yang, Steve Yang

The MTU value should be updated to 'max_rx_pkt_len - overhead'
no matter if the JUMBO FRAME offload enabled. If not update this MTU,
use will get the wrong MTU info via some command.
E.g.: 'show port info all' in testpmd tool.

Actually, the 'max_rx_pkt_len' has been used for other purposes in many
places now, even though the 'max_rx_pkt_len' is expected 'Only used if
JUMBO_FRAME enabled'.

For examples,
'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.

Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index daf5f24f7e..42857e3b67 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			ret = -EINVAL;
 			goto rollback;
 		}
-
-		/* Scale the MTU size to adapt max_rx_pkt_len */
-		dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
-				overhead_len;
 	} else {
 		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
 		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
@@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 						RTE_ETHER_MTU + overhead_len;
 	}
 
+	/* Scale the MTU size to adapt max_rx_pkt_len */
+	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
+				overhead_len;
+
 	/*
 	 * If LRO is enabled, check that the maximum aggregated packet
 	 * size is supported by the configured device.
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid
  2021-01-25  8:32     ` [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors Steve Yang
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
@ 2021-01-25  8:32       ` Steve Yang
  2021-01-25 14:45         ` Ferruh Yigit
  2021-01-25 15:46         ` Lance Richardson
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
  2 siblings, 2 replies; 38+ messages in thread
From: Steve Yang @ 2021-01-25  8:32 UTC (permalink / raw)
  To: dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas, ferruh.yigit,
	andrew.rybchenko, qiming.yang, Steve Yang

Moved the setting of 'DEV_RX_OFFLOAD_JUMBO_FRAME' from
'cmd_config_max_pkt_len_parsed()' to 'init_config()' caused fail the case
where 'max_rx_pkt_len' is changed from the command line via
"port config all max-pkt-len".

The 'init_config()' function is only called when testpmd is started,
but the DEV_RX_OFFLOAD_JUMBO_FRAME setting needs to be recomputed whenever
'max_rx_pkt_len' changes.

Define the 'update_jumbo_frame_offload()' function for both 'init_config()'
and 'cmd_config_max_pkt_len_parsed()', and detect if 'max_rx_pkt_len'
should be update to 1500 + overhead as default configuration. At the same
time, the offloads of rx queue also should update the value once port
offloads changed (e.g.: disabled JUMBO_FRAME offload via changed
max-pkt-len, reproduce steps [1]), otherwise, it would cause unexpected
result.

[1]
--------------------------------------------------------------------------
./x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i
    --max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
testpmd>  set verbose 3
testpmd>  port stop all
testpmd>  port config all max-pkt-len 1518  port start all

// Got fail error info without this patch
Configuring Port 0 (socket 1)
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
Fail to configure port 0 rx queues //<-- Fail error info;
--------------------------------------------------------------------------

Fixes: 761c4d6690 ("app/testpmd: fix max Rx packet length for VLAN packets")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 app/test-pmd/cmdline.c    |  1 +
 app/test-pmd/parameters.c |  1 +
 app/test-pmd/testpmd.c    | 63 ++++++++++++++++++++++++++++-----------
 app/test-pmd/testpmd.h    |  2 ++
 4 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 89034c8b72..8b6b7b6206 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1901,6 +1901,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 			printf("Unknown parameter\n");
 			return;
 		}
+		update_jumbo_frame_offload(pid, false);
 	}
 
 	init_port_config();
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index df5eb10d84..1c63156ddd 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -833,6 +833,7 @@ launch_args_parse(int argc, char** argv)
 						 "total-num-mbufs should be > 1024\n");
 			}
 			if (!strcmp(lgopts[opt_idx].name, "max-pkt-len")) {
+				default_max_pktlen = false;
 				n = atoi(optarg);
 				if (n >= RTE_ETHER_MIN_LEN)
 					rx_mode.max_rx_pkt_len = (uint32_t) n;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c256e719ae..f22d2be46d 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -531,6 +531,8 @@ uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN;
 /* Holds the registered mbuf dynamic flags names. */
 char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
 
+bool default_max_pktlen = true;
+
 /*
  * Helper function to check if socket is already discovered.
  * If yes, return positive value. If not, return zero.
@@ -1410,7 +1412,6 @@ init_config(void)
 	struct rte_gro_param gro_param;
 	uint32_t gso_types;
 	uint16_t data_size;
-	uint16_t eth_overhead;
 	bool warning = 0;
 	int k;
 	int ret;
@@ -1447,22 +1448,7 @@ init_config(void)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_dev_info_get() failed\n");
 
-		/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
-		if (port->dev_info.max_mtu != UINT16_MAX &&
-		    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
-			eth_overhead = port->dev_info.max_rx_pktlen -
-				port->dev_info.max_mtu;
-		else
-			eth_overhead =
-				RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
-
-		if (port->dev_conf.rxmode.max_rx_pkt_len <=
-			(uint32_t)(RTE_ETHER_MTU + eth_overhead))
-			port->dev_conf.rxmode.max_rx_pkt_len =
-					RTE_ETHER_MTU + eth_overhead;
-		else
-			port->dev_conf.rxmode.offloads |=
-					DEV_RX_OFFLOAD_JUMBO_FRAME;
+		update_jumbo_frame_offload(pid, default_max_pktlen);
 
 		if (!(port->dev_info.tx_offload_capa &
 		      DEV_TX_OFFLOAD_MBUF_FAST_FREE))
@@ -3358,6 +3344,49 @@ rxtx_port_config(struct rte_port *port)
 	}
 }
 
+void
+update_jumbo_frame_offload(portid_t portid, bool def_max_pktlen)
+{
+	struct rte_port *port = &ports[portid];
+	uint32_t eth_overhead;
+	uint64_t rx_offloads;
+
+	/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
+	if (port->dev_info.max_mtu != UINT16_MAX &&
+	    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
+		eth_overhead = port->dev_info.max_rx_pktlen -
+				port->dev_info.max_mtu;
+	else
+		eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+	rx_offloads = port->dev_conf.rxmode.offloads;
+	if (port->dev_conf.rxmode.max_rx_pkt_len <=
+		RTE_ETHER_MTU + eth_overhead) {
+		/*
+		 * If command line option doesn't include --max-pkt-len,
+		 * the default max_rx_pkt_len should be set to 1500 + overhead.
+		 */
+		if (def_max_pktlen)
+			port->dev_conf.rxmode.max_rx_pkt_len =
+						RTE_ETHER_MTU + eth_overhead;
+		rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+	} else
+		rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+
+	/* Apply Rx offloads configuration to Rx queue(s) */
+	if (rx_offloads != port->dev_conf.rxmode.offloads) {
+		uint16_t qid;
+
+		port->dev_conf.rxmode.offloads = rx_offloads;
+		if (eth_dev_info_get_print_err(portid, &port->dev_info) != 0)
+			rte_exit(EXIT_FAILURE,
+				"rte_eth_dev_info_get() failed\n");
+
+		for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++)
+			port->rx_conf[qid].offloads = rx_offloads;
+	}
+}
+
 void
 init_port_config(void)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 5f23162107..16f6fbefef 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -304,6 +304,7 @@ extern cmdline_parse_inst_t cmd_show_set_raw_all;
 
 extern uint16_t mempool_flags;
 
+extern bool default_max_pktlen;
 /**
  * Forwarding Configuration
  *
@@ -1005,6 +1006,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id, __rte_unused uint16_t queue,
 			 __rte_unused void *user_param);
 void add_tx_dynf_callback(portid_t portid);
 void remove_tx_dynf_callback(portid_t portid);
+void update_jumbo_frame_offload(portid_t portid, bool def_max_pktlen);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.17.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled
       [not found]         ` <DM6PR11MB43628A600BAAEC75FFF1343BF9BD9@DM6PR11MB4362.namprd11.prod.outlook.com>
@ 2021-01-25 12:38           ` Ferruh Yigit
  0 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-25 12:38 UTC (permalink / raw)
  To: Yang, SteveX, Huisong Li, dev
  Cc: Lu, Wenzhuo, Li, Xiaoyun, Iremonger, Bernard, thomas,
	andrew.rybchenko, Yang, Qiming, oulijun, huangdaode, Lijun Ou

On 1/25/2021 9:49 AM, Yang, SteveX wrote:
> Hi Huisong,
> 
> Thanks for your review.
> 
> The validity of the pair <DEV_RX_OFFLOAD_JUMBO_FRAME, max_rx_pkt_len> should be 
> checked from application layer (e.g.: testpmd),
> 
> and the RTE layer should keep open enough to adapt the high-layer requirement.
> 
> I’m not sure if exists some applications/NICs that treat ‘packet size < 1500’ as 
> JUMBO_FRAME. If so, that also can work as expect with current code.
> 
> @Yigit, Ferruh <mailto:ferruh.yigit@intel.com>, please correct me if something 
> understand wrong.
> 

Hi Huisong,

Agree that there is a grey area in the API, the question is if 'JUMBO_FRAME' is 
set, can application set the 'max_rx_pkt_len' less than "RTE_ETHER_MTU + 
overhead_len". Lijun (cc'ed) has the same concern.

The API documentation, and checks in the 'rte_eth_dev_configure()' enables 
setting this for a long time, I am reluctant to add this limitation now.
Although agree that application should set 'JUMBO_FRAME' properly based on 
requested 'MTU' value.


> BTW, there perhaps are some confused problems about jumbo frame and 
> max_rx_pkt_len, and Ferruh has scheduled to re-factor this part at release 21.11.
> 
> If you’re interesting about it, please refer to following link: [RFC,v2] doc: 
> announce max Rx packet len field deprecation - Patchwork (dpdk.org) 
> <http://patches.dpdk.org/patch/84522/>
> 
> Thanks & Regards,
> 
> Steve Yang.
> 
> *From:* Huisong Li <lihuisong@huawei.com>
> *Sent:* Monday, January 25, 2021 3:12 PM
> *To:* Yang, SteveX <stevex.yang@intel.com>; dev@dpdk.org
> *Cc:* Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; 
> Iremonger, Bernard <bernard.iremonger@intel.com>; thomas@monjalon.net; Yigit, 
> Ferruh <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru; Yang, Qiming 
> <qiming.yang@intel.com>; oulijun@huawei.com; huangdaode@huawei.com
> *Subject:* Re: [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when 
> jumbo frame disabled
> 
> Hi Steve,
> 
> In the current modification, the MTU is updated based on 'max_rx_pkt_len' 
> regardless of whether jumbo frame is enabled.
> 
> Now, MTU is correct when jumbo frmae is disabled. However, when jumbo frame is 
> enabled, the MTU value may be inconsistent with
> 
> the definition of the enabled jumbo frame. Like:
> 
> 1/ DEV_RX_OFFLOAD_JUMBO_FRAME is set;
> 
> 2/ max_rx_pkt_len = 1200
> 
> 3/ dev->data->mtu = 1200 - overhead_len(18) = 1182
> 
> In rte_eth_dev_configure API, the check for 'max_rx_pkt_len' is as follows:
> 
> if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {  //jumbo frame enabled
>          if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) {
>              xxxx
>              goto rollback;
>          } else if (*dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN*) {
>              xxxx
>              goto rollback;
>          }
> } else { //jumbo frame disabled
> 
>          if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
>                      pktlen > RTE_ETHER_MTU + overhead_len)
>                          /* Use default value */
>                          dev->data->dev_conf.rxmode.max_rx_pkt_len =
>                                                  RTE_ETHER_MTU + overhead_len;
> 
> }
> 
> Since the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME to enable jumbo frame, and 
> the framework API needs to update
> 
> the MTU based on 'max_rx_pkt_len', but the framework API uses 
> *RTE_ETHER_MIN_LEN(64)* to verify the boundary value of
> 
> 'max_rx_pkt_len', instead of "RTE_ETHER_MTU + overhead_len".  As far as I know, 
> if the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME
> 
> and 'max_rx_pkt_len' is 1200, the framework API or driver should return a 
> failure. As mentioned in this patch set, the jumbo frame
> 
> offload is set only when 'max_rx_pkt_len' requested is greater than 
> "RTE_ETHER_MTU + eth_overhead" in testpmd.
> 
> I really don't understand it.  How do you understand this behavior?
> 
> Thanks.
> 
> 在 2021/1/22 17:01, Steve Yang 写道:
> 
>     The MTU value should be updated to 'max_rx_pkt_len - overhead'
> 
>     no matter if the JUMBO FRAME offload enabled. If not update this MTU,
> 
>     use will get the wrong MTU info via some command.
> 
>     E.g.: 'show port info all' in testpmd tool.
> 
>     Actually, the 'max_rx_pkt_len' has been used for other purposes in many
> 
>     places now, even though the 'max_rx_pkt_len' is expected 'Only used if
> 
>     JUMBO_FRAME enabled'.
> 
>     For examples,
> 
>     'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.
> 
>     Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")
> 
>     Signed-off-by: Steve Yang<stevex.yang@intel.com>  <mailto:stevex.yang@intel.com>
> 
>     ---
> 
>       lib/librte_ethdev/rte_ethdev.c | 8 ++++----
> 
>       1 file changed, 4 insertions(+), 4 deletions(-)
> 
>     diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> 
>     index daf5f24f7e..42857e3b67 100644
> 
>     --- a/lib/librte_ethdev/rte_ethdev.c
> 
>     +++ b/lib/librte_ethdev/rte_ethdev.c
> 
>     @@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> 
>                              ret = -EINVAL;
> 
>                              goto rollback;
> 
>                      }
> 
>     -
> 
>     -               /* Scale the MTU size to adapt max_rx_pkt_len */
> 
>     -               dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> 
>     -                              overhead_len;
> 
>              } else {
> 
>                      uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
> 
>                      if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
> 
>     @@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> 
>                                                     RTE_ETHER_MTU + overhead_len;
> 
>              }
> 
>       
> 
>     +       /* Scale the MTU size to adapt max_rx_pkt_len */
> 
>     +       dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> 
>     +                              overhead_len;
> 
>     +
> 
>              /*
> 
>               * If LRO is enabled, check that the maximum aggregated packet
> 
>               * size is supported by the configured device.
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
@ 2021-01-25 12:41         ` Ferruh Yigit
  0 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-25 12:41 UTC (permalink / raw)
  To: Steve Yang, dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas,
	andrew.rybchenko, qiming.yang

On 1/25/2021 8:32 AM, Steve Yang wrote:
> The MTU value should be updated to 'max_rx_pkt_len - overhead'
> no matter if the JUMBO FRAME offload enabled. If not update this MTU,
> use will get the wrong MTU info via some command.
> E.g.: 'show port info all' in testpmd tool.
> 
> Actually, the 'max_rx_pkt_len' has been used for other purposes in many
> places now, even though the 'max_rx_pkt_len' is expected 'Only used if
> JUMBO_FRAME enabled'.
> 
> For examples,
> 'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.
> 
> Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index daf5f24f7e..42857e3b67 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   			ret = -EINVAL;
>   			goto rollback;
>   		}
> -
> -		/* Scale the MTU size to adapt max_rx_pkt_len */
> -		dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> -				overhead_len;
>   	} else {
>   		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
>   		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
> @@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   						RTE_ETHER_MTU + overhead_len;
>   	}
>   
> +	/* Scale the MTU size to adapt max_rx_pkt_len */
> +	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> +				overhead_len;
> +
>   	/*
>   	 * If LRO is enabled, check that the maximum aggregated packet
>   	 * size is supported by the configured device.
> 

This will cause 'max_rx_pkt_len' taken into account even 'JUMBO_FRAME' is not 
set, this is against the API documentation.

PMD can ignore the 'max_rx_pkt_len' when 'JUMBO_FRAME' is not set, so updating 
the 'max_rx_pkt_len' is relatively harmless, but updating 'MTU' can have affect.

Instead what do you think explicitly call 'rte_eth_dev_set_mtu()' in the testpmd 
when the MTU of the port needs to be changed because of "port config all 
max-pkt-len" command?

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid Steve Yang
@ 2021-01-25 14:45         ` Ferruh Yigit
  2021-01-25 15:46         ` Lance Richardson
  1 sibling, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-25 14:45 UTC (permalink / raw)
  To: Steve Yang, dev
  Cc: wenzhuo.lu, xiaoyun.li, bernard.iremonger, thomas,
	andrew.rybchenko, qiming.yang

On 1/25/2021 8:32 AM, Steve Yang wrote:
> Moved the setting of 'DEV_RX_OFFLOAD_JUMBO_FRAME' from
> 'cmd_config_max_pkt_len_parsed()' to 'init_config()' caused fail the case
> where 'max_rx_pkt_len' is changed from the command line via
> "port config all max-pkt-len".
> 
> The 'init_config()' function is only called when testpmd is started,
> but the DEV_RX_OFFLOAD_JUMBO_FRAME setting needs to be recomputed whenever
> 'max_rx_pkt_len' changes.
> 
> Define the 'update_jumbo_frame_offload()' function for both 'init_config()'
> and 'cmd_config_max_pkt_len_parsed()', and detect if 'max_rx_pkt_len'
> should be update to 1500 + overhead as default configuration. At the same
> time, the offloads of rx queue also should update the value once port
> offloads changed (e.g.: disabled JUMBO_FRAME offload via changed
> max-pkt-len, reproduce steps [1]), otherwise, it would cause unexpected
> result.
> 
> [1]
> --------------------------------------------------------------------------
> ./x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i
>      --max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518  port start all
> 
> // Got fail error info without this patch
> Configuring Port 0 (socket 1)
> Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
> within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
> Fail to configure port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
> 
> Fixes: 761c4d6690 ("app/testpmd: fix max Rx packet length for VLAN packets")
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>   app/test-pmd/cmdline.c    |  1 +
>   app/test-pmd/parameters.c |  1 +
>   app/test-pmd/testpmd.c    | 63 ++++++++++++++++++++++++++++-----------
>   app/test-pmd/testpmd.h    |  2 ++
>   4 files changed, 50 insertions(+), 17 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 89034c8b72..8b6b7b6206 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1901,6 +1901,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
>   			printf("Unknown parameter\n");
>   			return;
>   		}
> +		update_jumbo_frame_offload(pid, false);
>   	}
>   
>   	init_port_config();
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index df5eb10d84..1c63156ddd 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -833,6 +833,7 @@ launch_args_parse(int argc, char** argv)
>   						 "total-num-mbufs should be > 1024\n");
>   			}
>   			if (!strcmp(lgopts[opt_idx].name, "max-pkt-len")) {
> +				default_max_pktlen = false;
>   				n = atoi(optarg);
>   				if (n >= RTE_ETHER_MIN_LEN)
>   					rx_mode.max_rx_pkt_len = (uint32_t) n;
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index c256e719ae..f22d2be46d 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -531,6 +531,8 @@ uint16_t gso_max_segment_size = RTE_ETHER_MAX_LEN - RTE_ETHER_CRC_LEN;
>   /* Holds the registered mbuf dynamic flags names. */
>   char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
>   
> +bool default_max_pktlen = true;
> +
>   /*
>    * Helper function to check if socket is already discovered.
>    * If yes, return positive value. If not, return zero.
> @@ -1410,7 +1412,6 @@ init_config(void)
>   	struct rte_gro_param gro_param;
>   	uint32_t gso_types;
>   	uint16_t data_size;
> -	uint16_t eth_overhead;
>   	bool warning = 0;
>   	int k;
>   	int ret;
> @@ -1447,22 +1448,7 @@ init_config(void)
>   			rte_exit(EXIT_FAILURE,
>   				 "rte_eth_dev_info_get() failed\n");
>   
> -		/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
> -		if (port->dev_info.max_mtu != UINT16_MAX &&
> -		    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
> -			eth_overhead = port->dev_info.max_rx_pktlen -
> -				port->dev_info.max_mtu;
> -		else
> -			eth_overhead =
> -				RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> -
> -		if (port->dev_conf.rxmode.max_rx_pkt_len <=
> -			(uint32_t)(RTE_ETHER_MTU + eth_overhead))
> -			port->dev_conf.rxmode.max_rx_pkt_len =
> -					RTE_ETHER_MTU + eth_overhead;
> -		else
> -			port->dev_conf.rxmode.offloads |=
> -					DEV_RX_OFFLOAD_JUMBO_FRAME;
> +		update_jumbo_frame_offload(pid, default_max_pktlen);
>   
>   		if (!(port->dev_info.tx_offload_capa &
>   		      DEV_TX_OFFLOAD_MBUF_FAST_FREE))
> @@ -3358,6 +3344,49 @@ rxtx_port_config(struct rte_port *port)
>   	}
>   }
>   
> +void
> +update_jumbo_frame_offload(portid_t portid, bool def_max_pktlen)
> +{
> +	struct rte_port *port = &ports[portid];
> +	uint32_t eth_overhead;
> +	uint64_t rx_offloads;
> +
> +	/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
> +	if (port->dev_info.max_mtu != UINT16_MAX &&
> +	    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
> +		eth_overhead = port->dev_info.max_rx_pktlen -
> +				port->dev_info.max_mtu;
> +	else
> +		eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> +
> +	rx_offloads = port->dev_conf.rxmode.offloads;
> +	if (port->dev_conf.rxmode.max_rx_pkt_len <=
> +		RTE_ETHER_MTU + eth_overhead) {

This check is also used to decide if expansion is required, but the check is 
generic, with "port config all max-pkt-len" command this condition always can be 
created.

The target is make testpmd set "RTE_ETHER_MTU + eth_overhead" instead of fixed 
'RTE_ETHER_MAX_LEN' value, so why not set initial '.max_rx_pkt_len' value to 
zero, to say extend it to the default value.

> +		/*
> +		 * If command line option doesn't include --max-pkt-len,
> +		 * the default max_rx_pkt_len should be set to 1500 + overhead.
> +		 */
> +		if (def_max_pktlen)
> +			port->dev_conf.rxmode.max_rx_pkt_len =
> +						RTE_ETHER_MTU + eth_overhead;

What if '--max-pkt-len' is not provided, but "port config all max-pkt-len" 
command issued, won't is discard the command and always set to default?

> +		rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> +	} else
> +		rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> +
> +	/* Apply Rx offloads configuration to Rx queue(s) */
> +	if (rx_offloads != port->dev_conf.rxmode.offloads) {
> +		uint16_t qid;
> +
> +		port->dev_conf.rxmode.offloads = rx_offloads;
> +		if (eth_dev_info_get_print_err(portid, &port->dev_info) != 0)
> +			rte_exit(EXIT_FAILURE,
> +				"rte_eth_dev_info_get() failed\n");
> +
> +		for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++)
> +			port->rx_conf[qid].offloads = rx_offloads;

As said before, there is a concept of queue specific offloads, you can't just 
assume the queue offloads will always be same as the port offloads.
Instead it can be possible to add/remove JUMBO_FRAME offload to queues based on 
condition.

> +	}
> +}
> +
>   void
>   init_port_config(void)
>   {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 5f23162107..16f6fbefef 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -304,6 +304,7 @@ extern cmdline_parse_inst_t cmd_show_set_raw_all;
>   
>   extern uint16_t mempool_flags;
>   
> +extern bool default_max_pktlen;
>   /**
>    * Forwarding Configuration
>    *
> @@ -1005,6 +1006,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id, __rte_unused uint16_t queue,
>   			 __rte_unused void *user_param);
>   void add_tx_dynf_callback(portid_t portid);
>   void remove_tx_dynf_callback(portid_t portid);
> +void update_jumbo_frame_offload(portid_t portid, bool def_max_pktlen);
>   
>   /*
>    * Work-around of a compilation error with ICC on invocations of the
> 


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid Steve Yang
  2021-01-25 14:45         ` Ferruh Yigit
@ 2021-01-25 15:46         ` Lance Richardson
  2021-01-25 17:57           ` Ferruh Yigit
  1 sibling, 1 reply; 38+ messages in thread
From: Lance Richardson @ 2021-01-25 15:46 UTC (permalink / raw)
  To: Steve Yang
  Cc: dev, Lu, Wenzhuo, xiaoyun.li, Iremonger, Bernard,
	Thomas Monjalon, Ferruh Yigit, Andrew Rybchenko, Yang, Qiming

[-- Attachment #1: Type: text/plain, Size: 2649 bytes --]

On Mon, Jan 25, 2021 at 3:35 AM Steve Yang <stevex.yang@intel.com> wrote:
>
> Moved the setting of 'DEV_RX_OFFLOAD_JUMBO_FRAME' from
> 'cmd_config_max_pkt_len_parsed()' to 'init_config()' caused fail the case
> where 'max_rx_pkt_len' is changed from the command line via
> "port config all max-pkt-len".
>
> The 'init_config()' function is only called when testpmd is started,
> but the DEV_RX_OFFLOAD_JUMBO_FRAME setting needs to be recomputed whenever
> 'max_rx_pkt_len' changes.
>
> Define the 'update_jumbo_frame_offload()' function for both 'init_config()'
> and 'cmd_config_max_pkt_len_parsed()', and detect if 'max_rx_pkt_len'
> should be update to 1500 + overhead as default configuration. At the same
> time, the offloads of rx queue also should update the value once port
> offloads changed (e.g.: disabled JUMBO_FRAME offload via changed
> max-pkt-len, reproduce steps [1]), otherwise, it would cause unexpected
> result.
>
> [1]
> --------------------------------------------------------------------------
> ./x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i
>     --max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518  port start all
>
> // Got fail error info without this patch
> Configuring Port 0 (socket 1)
> Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
> within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
> Fail to configure port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
>
> Fixes: 761c4d6690 ("app/testpmd: fix max Rx packet length for VLAN packets")
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>  app/test-pmd/cmdline.c    |  1 +
>  app/test-pmd/parameters.c |  1 +
>  app/test-pmd/testpmd.c    | 63 ++++++++++++++++++++++++++++-----------
>  app/test-pmd/testpmd.h    |  2 ++
>  4 files changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 89034c8b72..8b6b7b6206 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1901,6 +1901,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
>                         printf("Unknown parameter\n");
>                         return;
>                 }
> +               update_jumbo_frame_offload(pid, false);

I'm probably missing something, but why isn't this a matter of simply calling
port_mtu_set() here (with mtu computed from max pkt len) and keeping
init_config() as currently implemented?

>         }
>
>         init_port_config();

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid
  2021-01-25 15:46         ` Lance Richardson
@ 2021-01-25 17:57           ` Ferruh Yigit
  0 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-25 17:57 UTC (permalink / raw)
  To: Lance Richardson, Steve Yang
  Cc: dev, Lu, Wenzhuo, xiaoyun.li, Iremonger, Bernard,
	Thomas Monjalon, Andrew Rybchenko, Yang, Qiming

On 1/25/2021 3:46 PM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 3:35 AM Steve Yang <stevex.yang@intel.com> wrote:
>>
>> Moved the setting of 'DEV_RX_OFFLOAD_JUMBO_FRAME' from
>> 'cmd_config_max_pkt_len_parsed()' to 'init_config()' caused fail the case
>> where 'max_rx_pkt_len' is changed from the command line via
>> "port config all max-pkt-len".
>>
>> The 'init_config()' function is only called when testpmd is started,
>> but the DEV_RX_OFFLOAD_JUMBO_FRAME setting needs to be recomputed whenever
>> 'max_rx_pkt_len' changes.
>>
>> Define the 'update_jumbo_frame_offload()' function for both 'init_config()'
>> and 'cmd_config_max_pkt_len_parsed()', and detect if 'max_rx_pkt_len'
>> should be update to 1500 + overhead as default configuration. At the same
>> time, the offloads of rx queue also should update the value once port
>> offloads changed (e.g.: disabled JUMBO_FRAME offload via changed
>> max-pkt-len, reproduce steps [1]), otherwise, it would cause unexpected
>> result.
>>
>> [1]
>> --------------------------------------------------------------------------
>> ./x86_64-native-linuxapp-gcc/app/dpdk-testpmd  -c 0xf -n 4  -- -i
>>      --max-pkt-len=9000 --tx-offloads=0x8000 --rxq=4 --txq=4 --disable-rss
>> testpmd>  set verbose 3
>> testpmd>  port stop all
>> testpmd>  port config all max-pkt-len 1518  port start all
>>
>> // Got fail error info without this patch
>> Configuring Port 0 (socket 1)
>> Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
>> within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
>> Fail to configure port 0 rx queues //<-- Fail error info;
>> --------------------------------------------------------------------------
>>
>> Fixes: 761c4d6690 ("app/testpmd: fix max Rx packet length for VLAN packets")
>>
>> Signed-off-by: Steve Yang <stevex.yang@intel.com>
>> ---
>>   app/test-pmd/cmdline.c    |  1 +
>>   app/test-pmd/parameters.c |  1 +
>>   app/test-pmd/testpmd.c    | 63 ++++++++++++++++++++++++++++-----------
>>   app/test-pmd/testpmd.h    |  2 ++
>>   4 files changed, 50 insertions(+), 17 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 89034c8b72..8b6b7b6206 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -1901,6 +1901,7 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
>>                          printf("Unknown parameter\n");
>>                          return;
>>                  }
>> +               update_jumbo_frame_offload(pid, false);
> 
> I'm probably missing something, but why isn't this a matter of simply calling
> port_mtu_set() here (with mtu computed from max pkt len) and keeping
> init_config() as currently implemented?
> 

They are very similar, calling the 'port_mtu_set()' can be an option, but it is 
headache to get dev->info first and calculate overhead to be able to call 
'port_mtu_set()';
Since this is already done in 'init_config()', converting it to a function and 
reuse it also looks valid as done here.

I am updating that function and sending a new version soon.


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-25  8:32     ` [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors Steve Yang
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
  2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid Steve Yang
@ 2021-01-25 18:15       ` Ferruh Yigit
  2021-01-25 19:41         ` Lance Richardson
                           ` (4 more replies)
  2 siblings, 5 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-25 18:15 UTC (permalink / raw)
  To: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: Ferruh Yigit, dev, stable, lance.richardson, oulijun, wisamm, lihuisong

From: Steve Yang <stevex.yang@intel.com>

"port config all max-pkt-len" command fails because it doesn't set the
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.

Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
'init_config()' function is only called during testpmd startup, but the
flag status needs to be calculated whenever 'max_rx_pkt_len' changes.

The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
didn't.

Adding the 'update_jumbo_frame_offload()' helper function to update
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
function is called both by 'init_config()' and
'cmd_config_max_pkt_len_parsed()'.

Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
is zero.
If '--max-pkt-len=N' argument provided, it will be used instead.
And with each "port config all max-pkt-len" command, the
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
updated.

[1]
--------------------------------------------------------------------------
dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
	--rxq=4 --txq=4 --disable-rss
testpmd>  set verbose 3
testpmd>  port stop all
testpmd>  port config all max-pkt-len 1518
testpmd>  port start all

// Got fail error info without this patch
Configuring Port 0 (socket 1)
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
Fail to configure port 0 rx queues //<-- Fail error info;
--------------------------------------------------------------------------

Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN packets")
Cc: stable@dpdk.org

Signed-off-by: Steve Yang <stevex.yang@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---

v5:
* 'update_jumbo_frame_offload()' helper updated
  * check zero 'max-pkt-len' value
  * Update how queue offload flags updated
  * Update MTU if JUMBO_FRAME flag is not set
* Default testpmd 'max-pkt-len' value set to zero

Cc: lance.richardson@broadcom.com
Cc: oulijun@huawei.com
Cc: wisamm@mellanox.com
Cc: lihuisong@huawei.com
---
 app/test-pmd/cmdline.c |  13 ++++++
 app/test-pmd/testpmd.c | 102 +++++++++++++++++++++++++++++++++--------
 app/test-pmd/testpmd.h |   1 +
 3 files changed, 97 insertions(+), 19 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 89034c8b7272..9ada4316c6c0 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1877,7 +1877,9 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 				__rte_unused void *data)
 {
 	struct cmd_config_max_pkt_len_result *res = parsed_result;
+	uint32_t max_rx_pkt_len_backup = 0;
 	portid_t pid;
+	int ret;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -1896,7 +1898,18 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 			if (res->value == port->dev_conf.rxmode.max_rx_pkt_len)
 				return;
 
+			ret = eth_dev_info_get_print_err(pid, &port->dev_info);
+			if (ret != 0) {
+				printf("rte_eth_dev_info_get() failed for port %u\n",
+					pid);
+				return;
+			}
+
+			max_rx_pkt_len_backup = port->dev_conf.rxmode.max_rx_pkt_len;
+
 			port->dev_conf.rxmode.max_rx_pkt_len = res->value;
+			if (update_jumbo_frame_offload(pid) != 0)
+				port->dev_conf.rxmode.max_rx_pkt_len = max_rx_pkt_len_backup;
 		} else {
 			printf("Unknown parameter\n");
 			return;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c256e719aea2..b69fcd3fde72 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -443,8 +443,11 @@ lcoreid_t latencystats_lcore_id = -1;
  * Ethernet device configuration.
  */
 struct rte_eth_rxmode rx_mode = {
-	.max_rx_pkt_len = RTE_ETHER_MAX_LEN,
-		/**< Default maximum frame length. */
+	/* Default maximum frame length.
+	 * Zero is converted to "RTE_ETHER_MTU + PMD Ethernet overhead"
+	 * in init_config().
+	 */
+	.max_rx_pkt_len = 0,
 };
 
 struct rte_eth_txmode tx_mode = {
@@ -1410,7 +1413,6 @@ init_config(void)
 	struct rte_gro_param gro_param;
 	uint32_t gso_types;
 	uint16_t data_size;
-	uint16_t eth_overhead;
 	bool warning = 0;
 	int k;
 	int ret;
@@ -1447,22 +1449,10 @@ init_config(void)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_dev_info_get() failed\n");
 
-		/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
-		if (port->dev_info.max_mtu != UINT16_MAX &&
-		    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
-			eth_overhead = port->dev_info.max_rx_pktlen -
-				port->dev_info.max_mtu;
-		else
-			eth_overhead =
-				RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
-
-		if (port->dev_conf.rxmode.max_rx_pkt_len <=
-			(uint32_t)(RTE_ETHER_MTU + eth_overhead))
-			port->dev_conf.rxmode.max_rx_pkt_len =
-					RTE_ETHER_MTU + eth_overhead;
-		else
-			port->dev_conf.rxmode.offloads |=
-					DEV_RX_OFFLOAD_JUMBO_FRAME;
+		ret = update_jumbo_frame_offload(pid);
+		if (ret != 0)
+			printf("Updating jumbo frame offload failed for port %u\n",
+				pid);
 
 		if (!(port->dev_info.tx_offload_capa &
 		      DEV_TX_OFFLOAD_MBUF_FAST_FREE))
@@ -3358,6 +3348,80 @@ rxtx_port_config(struct rte_port *port)
 	}
 }
 
+/*
+ * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
+ * MTU is also aligned if JUMBO_FRAME offload is not set.
+ *
+ * port->dev_info should be get before calling this function.
+ *
+ * return 0 on success, negative on error
+ */
+int
+update_jumbo_frame_offload(portid_t portid)
+{
+	struct rte_port *port = &ports[portid];
+	uint32_t eth_overhead;
+	uint64_t rx_offloads;
+	int ret;
+	bool on;
+
+	/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
+	if (port->dev_info.max_mtu != UINT16_MAX &&
+	    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
+		eth_overhead = port->dev_info.max_rx_pktlen -
+				port->dev_info.max_mtu;
+	else
+		eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+	rx_offloads = port->dev_conf.rxmode.offloads;
+
+	/* Default config value is 0 to use PMD specific overhead */
+	if (port->dev_conf.rxmode.max_rx_pkt_len == 0)
+		port->dev_conf.rxmode.max_rx_pkt_len = RTE_ETHER_MTU + eth_overhead;
+
+	if (port->dev_conf.rxmode.max_rx_pkt_len <= RTE_ETHER_MTU + eth_overhead) {
+		rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+		on = false;
+	} else {
+		if ((port->dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
+			printf("Frame size (%u) is not supported by port %u\n",
+				port->dev_conf.rxmode.max_rx_pkt_len,
+				portid);
+			return -1;
+		}
+		rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+		on = true;
+	}
+
+	if (rx_offloads != port->dev_conf.rxmode.offloads) {
+		uint16_t qid;
+
+		port->dev_conf.rxmode.offloads = rx_offloads;
+
+		/* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
+		for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
+			if (on)
+				port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+			else
+				port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+		}
+	}
+
+	/* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
+	 * if unset do it here
+	 */
+	if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
+		ret = rte_eth_dev_set_mtu(portid,
+				port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
+		if (ret)
+			printf("Failed to set MTU to %u for port %u\n",
+				port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
+				portid);
+	}
+
+	return 0;
+}
+
 void
 init_port_config(void)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 5f2316210726..2f8f5a92e46a 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -1005,6 +1005,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id, __rte_unused uint16_t queue,
 			 __rte_unused void *user_param);
 void add_tx_dynf_callback(portid_t portid);
 void remove_tx_dynf_callback(portid_t portid);
+int update_jumbo_frame_offload(portid_t portid);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.29.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
@ 2021-01-25 19:41         ` Lance Richardson
  2021-01-26  0:44           ` Ferruh Yigit
  2021-01-26  9:02         ` [dpdk-dev] " Wisam Monther
                           ` (3 subsequent siblings)
  4 siblings, 1 reply; 38+ messages in thread
From: Lance Richardson @ 2021-01-25 19:41 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

[-- Attachment #1: Type: text/plain, Size: 3216 bytes --]

On Mon, Jan 25, 2021 at 1:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> From: Steve Yang <stevex.yang@intel.com>
>
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the
> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
>
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
>
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
>
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
> is zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
>
> [1]

<snip>

> +/*
> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
> + * MTU is also aligned if JUMBO_FRAME offload is not set.
> + *
> + * port->dev_info should be get before calling this function.

Should this be "port->dev_info should be set ..." instead?


<snip>

> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
> +               uint16_t qid;
> +
> +               port->dev_conf.rxmode.offloads = rx_offloads;
> +
> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> +                       if (on)
> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> +                       else
> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> +               }

Is it correct to set per-queue offloads that aren't advertised by the PMD
as supported in rx_queue_offload_capa?

> +       }
> +
> +       /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
> +        * if unset do it here
> +        */
> +       if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> +               ret = rte_eth_dev_set_mtu(portid,
> +                               port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
> +               if (ret)
> +                       printf("Failed to set MTU to %u for port %u\n",
> +                               port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
> +                               portid);
> +       }
> +
> +       return 0;
> +}
> +

Applied and tested with a few iterations of configuring max packet size
back and forth between jumbo and non-jumbo sizes, also tried setting
max packet size using the command-line option, all seems good based
on rx offloads and packet forwarding.

Two minor questions above, otherwise LGTM.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-25 19:41         ` Lance Richardson
@ 2021-01-26  0:44           ` Ferruh Yigit
  2021-01-26  3:22             ` Lance Richardson
  2021-01-26  3:45             ` Lance Richardson
  0 siblings, 2 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-26  0:44 UTC (permalink / raw)
  To: Lance Richardson
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

On 1/25/2021 7:41 PM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 1:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> From: Steve Yang <stevex.yang@intel.com>
>>
>> "port config all max-pkt-len" command fails because it doesn't set the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>>
>> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
>> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
>> 'init_config()' function is only called during testpmd startup, but the
>> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
>>
>> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
>> didn't.
>>
>> Adding the 'update_jumbo_frame_offload()' helper function to update
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
>> function is called both by 'init_config()' and
>> 'cmd_config_max_pkt_len_parsed()'.
>>
>> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
>> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
>> is zero.
>> If '--max-pkt-len=N' argument provided, it will be used instead.
>> And with each "port config all max-pkt-len" command, the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
>> updated.
>>
>> [1]
> 
> <snip>
> 
>> +/*
>> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
>> + * MTU is also aligned if JUMBO_FRAME offload is not set.
>> + *
>> + * port->dev_info should be get before calling this function.
> 
> Should this be "port->dev_info should be set ..." instead?
> 

Ack.

> 
> <snip>
> 
>> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
>> +               uint16_t qid;
>> +
>> +               port->dev_conf.rxmode.offloads = rx_offloads;
>> +
>> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>> +                       if (on)
>> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>> +                       else
>> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>> +               }
> 
> Is it correct to set per-queue offloads that aren't advertised by the PMD
> as supported in rx_queue_offload_capa?
> 

'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values 
are reflected to 'port->rx_conf[].offloads' for all queues.

We should set the offload in 'port->rx_conf[].offloads' if it is set in 
'port->dev_conf.rxmode.offloads'.

If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have 
it. And the port level capability is already checked above.

>> +       }
>> +
>> +       /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
>> +        * if unset do it here
>> +        */
>> +       if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
>> +               ret = rte_eth_dev_set_mtu(portid,
>> +                               port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
>> +               if (ret)
>> +                       printf("Failed to set MTU to %u for port %u\n",
>> +                               port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
>> +                               portid);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
> 
> Applied and tested with a few iterations of configuring max packet size
> back and forth between jumbo and non-jumbo sizes, also tried setting
> max packet size using the command-line option, all seems good based
> on rx offloads and packet forwarding.
> 
> Two minor questions above, otherwise LGTM.
> 

Thanks for testing. I will wait for more comments before new version.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-26  0:44           ` Ferruh Yigit
@ 2021-01-26  3:22             ` Lance Richardson
  2021-01-26  3:45             ` Lance Richardson
  1 sibling, 0 replies; 38+ messages in thread
From: Lance Richardson @ 2021-01-26  3:22 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

[-- Attachment #1: Type: text/plain, Size: 4181 bytes --]

Acked-by: Lance Richardson <lance.richardson@broadcom.com>

Thanks,
    Lance

On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 1/25/2021 7:41 PM, Lance Richardson wrote:
> > On Mon, Jan 25, 2021 at 1:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >>
> >> From: Steve Yang <stevex.yang@intel.com>
> >>
> >> "port config all max-pkt-len" command fails because it doesn't set the
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> >>
> >> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> >> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> >> 'init_config()' function is only called during testpmd startup, but the
> >> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
> >>
> >> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> >> didn't.
> >>
> >> Adding the 'update_jumbo_frame_offload()' helper function to update
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> >> function is called both by 'init_config()' and
> >> 'cmd_config_max_pkt_len_parsed()'.
> >>
> >> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> >> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
> >> is zero.
> >> If '--max-pkt-len=N' argument provided, it will be used instead.
> >> And with each "port config all max-pkt-len" command, the
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> >> updated.
> >>
> >> [1]
> >
> > <snip>
> >
> >> +/*
> >> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
> >> + * MTU is also aligned if JUMBO_FRAME offload is not set.
> >> + *
> >> + * port->dev_info should be get before calling this function.
> >
> > Should this be "port->dev_info should be set ..." instead?
> >
>
> Ack.
>
> >
> > <snip>
> >
> >> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
> >> +               uint16_t qid;
> >> +
> >> +               port->dev_conf.rxmode.offloads = rx_offloads;
> >> +
> >> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> >> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> >> +                       if (on)
> >> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> +                       else
> >> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> +               }
> >
> > Is it correct to set per-queue offloads that aren't advertised by the PMD
> > as supported in rx_queue_offload_capa?
> >
>
> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
> are reflected to 'port->rx_conf[].offloads' for all queues.
>
> We should set the offload in 'port->rx_conf[].offloads' if it is set in
> 'port->dev_conf.rxmode.offloads'.
>
> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
> it. And the port level capability is already checked above.
>
> >> +       }
> >> +
> >> +       /* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
> >> +        * if unset do it here
> >> +        */
> >> +       if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> >> +               ret = rte_eth_dev_set_mtu(portid,
> >> +                               port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
> >> +               if (ret)
> >> +                       printf("Failed to set MTU to %u for port %u\n",
> >> +                               port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
> >> +                               portid);
> >> +       }
> >> +
> >> +       return 0;
> >> +}
> >> +
> >
> > Applied and tested with a few iterations of configuring max packet size
> > back and forth between jumbo and non-jumbo sizes, also tried setting
> > max packet size using the command-line option, all seems good based
> > on rx offloads and packet forwarding.
> >
> > Two minor questions above, otherwise LGTM.
> >
>
> Thanks for testing. I will wait for more comments before new version.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-26  0:44           ` Ferruh Yigit
  2021-01-26  3:22             ` Lance Richardson
@ 2021-01-26  3:45             ` Lance Richardson
  2021-01-26  7:54               ` Li, Xiaoyun
  2021-01-26 11:01               ` Ferruh Yigit
  1 sibling, 2 replies; 38+ messages in thread
From: Lance Richardson @ 2021-01-26  3:45 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> >> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
> >> +               uint16_t qid;
> >> +
> >> +               port->dev_conf.rxmode.offloads = rx_offloads;
> >> +
> >> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> >> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> >> +                       if (on)
> >> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> +                       else
> >> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >> +               }
> >
> > Is it correct to set per-queue offloads that aren't advertised by the PMD
> > as supported in rx_queue_offload_capa?
> >
>
> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
> are reflected to 'port->rx_conf[].offloads' for all queues.
>
> We should set the offload in 'port->rx_conf[].offloads' if it is set in
> 'port->dev_conf.rxmode.offloads'.
>
> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
> it. And the port level capability is already checked above.
>

I'm still not 100% clear about the per-queue offload question.

With this patch, and jumbo max packet size configured (on the command
line in this case), I see:

testpmd> show port 0 rx_offload configuration
Rx Offloading Configuration of port 0 :
  Port : JUMBO_FRAME
  Queue[ 0] : JUMBO_FRAME

testpmd> show port 0 rx_offload capabilities
Rx Offloading Capabilities of port 0 :
  Per Queue :
  Per Port  : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
KEEP_CRC OUTER_UDP_CKSUM RSS_HASH

Yet if I configure a jumbo MTU starting with standard max packet size,
jumbo is only enabled at the port level:
testpmd> port config mtu 0 9000
testpmd> port start all

testpmd> show port 0 rx_offload configuration
Rx Offloading Configuration of port 0 :
  Port : JUMBO_FRAME
  Queue[ 0] :

It still seems odd for a per-queue offload to be enabled on a PMD that
doesn't support per-queue receive offloads.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-26  3:45             ` Lance Richardson
@ 2021-01-26  7:54               ` Li, Xiaoyun
  2021-01-26 11:01               ` Ferruh Yigit
  1 sibling, 0 replies; 38+ messages in thread
From: Li, Xiaoyun @ 2021-01-26  7:54 UTC (permalink / raw)
  To: Lance Richardson, Yigit, Ferruh
  Cc: Lu, Wenzhuo, Iremonger, Bernard, Yang, SteveX, dev, stable,
	oulijun, wisamm, lihuisong



> -----Original Message-----
> From: Lance Richardson <lance.richardson@broadcom.com>
> Sent: Tuesday, January 26, 2021 11:45
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>; dev@dpdk.org; stable@dpdk.org;
> oulijun@huawei.com; wisamm@mellanox.com; lihuisong@huawei.com
> Subject: Re: [PATCH v5] app/testpmd: fix setting maximum packet length
> 
> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >
> > >> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
> > >> +               uint16_t qid;
> > >> +
> > >> +               port->dev_conf.rxmode.offloads = rx_offloads;
> > >> +
> > >> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> > >> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> > >> +                       if (on)
> > >> +                               port->rx_conf[qid].offloads |=
> DEV_RX_OFFLOAD_JUMBO_FRAME;
> > >> +                       else
> > >> +                               port->rx_conf[qid].offloads &=
> ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> > >> +               }
> > >
> > > Is it correct to set per-queue offloads that aren't advertised by the PMD
> > > as supported in rx_queue_offload_capa?
> > >
> >
> > 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads'
> values
> > are reflected to 'port->rx_conf[].offloads' for all queues.
> >
> > We should set the offload in 'port->rx_conf[].offloads' if it is set in
> > 'port->dev_conf.rxmode.offloads'.
> >
> > If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can
> have
> > it. And the port level capability is already checked above.
> >
> 
> I'm still not 100% clear about the per-queue offload question.
> 
> With this patch, and jumbo max packet size configured (on the command
> line in this case), I see:
> 
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
>   Port : JUMBO_FRAME
>   Queue[ 0] : JUMBO_FRAME
> 
> testpmd> show port 0 rx_offload capabilities
> Rx Offloading Capabilities of port 0 :
>   Per Queue :
>   Per Port  : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER
> TIMESTAMP
> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
> 
> Yet if I configure a jumbo MTU starting with standard max packet size,
> jumbo is only enabled at the port level:
> testpmd> port config mtu 0 9000
> testpmd> port start all
> 
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
>   Port : JUMBO_FRAME
>   Queue[ 0] :
> 
> It still seems odd for a per-queue offload to be enabled on a PMD that
> doesn't support per-queue receive offloads.

In struct rte_eth_dev_info, rx_offload_capa means All RX offload capabilities including all per-queue ones.
And rx_queue_offload_capa means Device per-queue RX offload capabilities.

The meaning of rx_queue_offload_capa is a bit of confusing between here and driver.
I think here rx_queue_offload_capa means whether a queue supports offloads.

But some drivers like i40e don't use rx_queue_offload_capa, set this as 0 and only use global rx_offload_capa.
I guess it's because the driver doesn't want to support different offloads settings for different queues?
Then rx_queue_offload_capa means differently.

Actually for i40e, it can support different offloads for different queues since there is 'offloads' in struct i40e_rx_queue.
I40e can just set rx_queue_offload_capa as the value for rx_offload_capa.
But maybe some drivers really don't want this? Not sure on this.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
  2021-01-25 19:41         ` Lance Richardson
@ 2021-01-26  9:02         ` Wisam Monther
  2021-01-27  3:04         ` Li, Xiaoyun
                           ` (2 subsequent siblings)
  4 siblings, 0 replies; 38+ messages in thread
From: Wisam Monther @ 2021-01-26  9:02 UTC (permalink / raw)
  To: Ferruh Yigit, Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: dev, stable, lance.richardson, oulijun, lihuisong

Hi,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, January 25, 2021 8:16 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
> <xiaoyun.li@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> lance.richardson@broadcom.com; oulijun@huawei.com; Wisam Monther
> <wisamm@nvidia.com>; lihuisong@huawei.com
> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
> 
> From: Steve Yang <stevex.yang@intel.com>
> 
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> 
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
> 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the flag
> status needs to be calculated whenever 'max_rx_pkt_len' changes.
> 
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
> 
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
> 
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
> zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
> 
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> 	--rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518  port start all
> 
> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure
> port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
> 
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
> packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> 
> v5:
> * 'update_jumbo_frame_offload()' helper updated
>   * check zero 'max-pkt-len' value
>   * Update how queue offload flags updated
>   * Update MTU if JUMBO_FRAME flag is not set
> * Default testpmd 'max-pkt-len' value set to zero
> 
> Cc: lance.richardson@broadcom.com
> Cc: oulijun@huawei.com
> Cc: wisamm@mellanox.com
> Cc: lihuisong@huawei.com
> ---

It fixed this bug indeed: https://bugs.dpdk.org/show_bug.cgi?id=625
Thanks
Acked-by: Wisam Jaddo <wisamm@nvidia.com>


BRs,
Wisam Jaddo

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-26  3:45             ` Lance Richardson
  2021-01-26  7:54               ` Li, Xiaoyun
@ 2021-01-26 11:01               ` Ferruh Yigit
  2021-01-28 21:36                 ` Lance Richardson
  1 sibling, 1 reply; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-26 11:01 UTC (permalink / raw)
  To: Lance Richardson
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

On 1/26/2021 3:45 AM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>>> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
>>>> +               uint16_t qid;
>>>> +
>>>> +               port->dev_conf.rxmode.offloads = rx_offloads;
>>>> +
>>>> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>>>> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>>>> +                       if (on)
>>>> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>> +                       else
>>>> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>> +               }
>>>
>>> Is it correct to set per-queue offloads that aren't advertised by the PMD
>>> as supported in rx_queue_offload_capa?
>>>
>>
>> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
>> are reflected to 'port->rx_conf[].offloads' for all queues.
>>
>> We should set the offload in 'port->rx_conf[].offloads' if it is set in
>> 'port->dev_conf.rxmode.offloads'.
>>
>> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
>> it. And the port level capability is already checked above.
>>
> 
> I'm still not 100% clear about the per-queue offload question.
> 
> With this patch, and jumbo max packet size configured (on the command
> line in this case), I see:
> 
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
>    Port : JUMBO_FRAME
>    Queue[ 0] : JUMBO_FRAME
> 
> testpmd> show port 0 rx_offload capabilities
> Rx Offloading Capabilities of port 0 :
>    Per Queue :
>    Per Port  : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
> 

The port level offload is applied to all queues on the port, testpmd config 
structure reflects this logic in implementation.
If Rx offload X is set for a port, it is set for all Rx queue offloads, this is 
not new behavior and not related to this patch.

In the ethdev, lets assume X & Y are port level offloads,
after X, Y are set via 'rte_eth_dev_configure()'
if user calls 'rte_eth_rx_queue_setup()' with X & Y offload, this is a valid 
call and API will return success, since those offloads already enabled in port 
level means they are enabled for all queues.

Because of above ethdev behavior, testpmd keeps all enabled port level offload 
in the queue level offload too, and display them as enabled offloads for the queue.

To request a queue specific offload, it is added to specific queue's config 
before calling queue setup. Lets say that queue specific offload is Z, after 
setup testpmd config struct will show that specific queue has X, Y & Z offloads.

I hope it is more clear now.

> Yet if I configure a jumbo MTU starting with standard max packet size,
> jumbo is only enabled at the port level:
> testpmd> port config mtu 0 9000
> testpmd> port start all
> 
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
>    Port : JUMBO_FRAME
>    Queue[ 0] :
> 
> It still seems odd for a per-queue offload to be enabled on a PMD that
> doesn't support per-queue receive offloads.
> 

"port config mtu" should take queue offloads into account, it looks wrong right now.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
  2021-01-25 19:41         ` Lance Richardson
  2021-01-26  9:02         ` [dpdk-dev] " Wisam Monther
@ 2021-01-27  3:04         ` Li, Xiaoyun
  2021-01-28  1:57           ` Chen, BoX C
  2021-01-28  9:18         ` Wisam Monther
  2021-01-28 12:07         ` [dpdk-dev] [PATCH v6] " Ferruh Yigit
  4 siblings, 1 reply; 38+ messages in thread
From: Li, Xiaoyun @ 2021-01-27  3:04 UTC (permalink / raw)
  To: Yigit, Ferruh, Lu, Wenzhuo, Iremonger, Bernard, Yang, SteveX
  Cc: dev, stable, lance.richardson, oulijun, wisamm, lihuisong

Except a minor typo inline.
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Tuesday, January 26, 2021 02:16
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Yang, SteveX
> <stevex.yang@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> lance.richardson@broadcom.com; oulijun@huawei.com;
> wisamm@mellanox.com; lihuisong@huawei.com
> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
> 
> From: Steve Yang <stevex.yang@intel.com>
> 
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> 
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the flag status
> needs to be calculated whenever 'max_rx_pkt_len' changes.
> 
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
reproduced

> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it didn't.
> 
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and 'cmd_config_max_pkt_len_parsed()'.
> 
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
> zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
> 
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> 	--rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518  port start all
> 
> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-queue
> offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure port 0 rx
> queues //<-- Fail error info;
> --------------------------------------------------------------------------
> 
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> 
> v5:
> * 'update_jumbo_frame_offload()' helper updated
>   * check zero 'max-pkt-len' value
>   * Update how queue offload flags updated
>   * Update MTU if JUMBO_FRAME flag is not set
> * Default testpmd 'max-pkt-len' value set to zero
> 
> Cc: lance.richardson@broadcom.com
> Cc: oulijun@huawei.com
> Cc: wisamm@mellanox.com
> Cc: lihuisong@huawei.com
> ---
>  app/test-pmd/cmdline.c |  13 ++++++
>  app/test-pmd/testpmd.c | 102 +++++++++++++++++++++++++++++++++--------
>  app/test-pmd/testpmd.h |   1 +
>  3 files changed, 97 insertions(+), 19 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> 89034c8b7272..9ada4316c6c0 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1877,7 +1877,9 @@ cmd_config_max_pkt_len_parsed(void
> *parsed_result,
>  				__rte_unused void *data)
>  {
>  	struct cmd_config_max_pkt_len_result *res = parsed_result;
> +	uint32_t max_rx_pkt_len_backup = 0;
>  	portid_t pid;
> +	int ret;
> 
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n"); @@ -1896,7 +1898,18 @@
> cmd_config_max_pkt_len_parsed(void *parsed_result,
>  			if (res->value == port-
> >dev_conf.rxmode.max_rx_pkt_len)
>  				return;
> 
> +			ret = eth_dev_info_get_print_err(pid, &port->dev_info);
> +			if (ret != 0) {
> +				printf("rte_eth_dev_info_get() failed for
> port %u\n",
> +					pid);
> +				return;
> +			}
> +
> +			max_rx_pkt_len_backup = port-
> >dev_conf.rxmode.max_rx_pkt_len;
> +
>  			port->dev_conf.rxmode.max_rx_pkt_len = res->value;
> +			if (update_jumbo_frame_offload(pid) != 0)
> +				port->dev_conf.rxmode.max_rx_pkt_len =
> max_rx_pkt_len_backup;
>  		} else {
>  			printf("Unknown parameter\n");
>  			return;
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> c256e719aea2..b69fcd3fde72 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -443,8 +443,11 @@ lcoreid_t latencystats_lcore_id = -1;
>   * Ethernet device configuration.
>   */
>  struct rte_eth_rxmode rx_mode = {
> -	.max_rx_pkt_len = RTE_ETHER_MAX_LEN,
> -		/**< Default maximum frame length. */
> +	/* Default maximum frame length.
> +	 * Zero is converted to "RTE_ETHER_MTU + PMD Ethernet overhead"
> +	 * in init_config().
> +	 */
> +	.max_rx_pkt_len = 0,
>  };
> 
>  struct rte_eth_txmode tx_mode = {
> @@ -1410,7 +1413,6 @@ init_config(void)
>  	struct rte_gro_param gro_param;
>  	uint32_t gso_types;
>  	uint16_t data_size;
> -	uint16_t eth_overhead;
>  	bool warning = 0;
>  	int k;
>  	int ret;
> @@ -1447,22 +1449,10 @@ init_config(void)
>  			rte_exit(EXIT_FAILURE,
>  				 "rte_eth_dev_info_get() failed\n");
> 
> -		/* Update the max_rx_pkt_len to have MTU as
> RTE_ETHER_MTU */
> -		if (port->dev_info.max_mtu != UINT16_MAX &&
> -		    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
> -			eth_overhead = port->dev_info.max_rx_pktlen -
> -				port->dev_info.max_mtu;
> -		else
> -			eth_overhead =
> -				RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> -
> -		if (port->dev_conf.rxmode.max_rx_pkt_len <=
> -			(uint32_t)(RTE_ETHER_MTU + eth_overhead))
> -			port->dev_conf.rxmode.max_rx_pkt_len =
> -					RTE_ETHER_MTU + eth_overhead;
> -		else
> -			port->dev_conf.rxmode.offloads |=
> -					DEV_RX_OFFLOAD_JUMBO_FRAME;
> +		ret = update_jumbo_frame_offload(pid);
> +		if (ret != 0)
> +			printf("Updating jumbo frame offload failed for
> port %u\n",
> +				pid);
> 
>  		if (!(port->dev_info.tx_offload_capa &
>  		      DEV_TX_OFFLOAD_MBUF_FAST_FREE)) @@ -3358,6
> +3348,80 @@ rxtx_port_config(struct rte_port *port)
>  	}
>  }
> 
> +/*
> + * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME
> +offload,
> + * MTU is also aligned if JUMBO_FRAME offload is not set.
> + *
> + * port->dev_info should be get before calling this function.
> + *
> + * return 0 on success, negative on error  */ int
> +update_jumbo_frame_offload(portid_t portid) {
> +	struct rte_port *port = &ports[portid];
> +	uint32_t eth_overhead;
> +	uint64_t rx_offloads;
> +	int ret;
> +	bool on;
> +
> +	/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
> +	if (port->dev_info.max_mtu != UINT16_MAX &&
> +	    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
> +		eth_overhead = port->dev_info.max_rx_pktlen -
> +				port->dev_info.max_mtu;
> +	else
> +		eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> +
> +	rx_offloads = port->dev_conf.rxmode.offloads;
> +
> +	/* Default config value is 0 to use PMD specific overhead */
> +	if (port->dev_conf.rxmode.max_rx_pkt_len == 0)
> +		port->dev_conf.rxmode.max_rx_pkt_len = RTE_ETHER_MTU +
> eth_overhead;
> +
> +	if (port->dev_conf.rxmode.max_rx_pkt_len <= RTE_ETHER_MTU +
> eth_overhead) {
> +		rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> +		on = false;
> +	} else {
> +		if ((port->dev_info.rx_offload_capa &
> DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> +			printf("Frame size (%u) is not supported by port %u\n",
> +				port->dev_conf.rxmode.max_rx_pkt_len,
> +				portid);
> +			return -1;
> +		}
> +		rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> +		on = true;
> +	}
> +
> +	if (rx_offloads != port->dev_conf.rxmode.offloads) {
> +		uint16_t qid;
> +
> +		port->dev_conf.rxmode.offloads = rx_offloads;
> +
> +		/* Apply JUMBO_FRAME offload configuration to Rx queue(s)
> */
> +		for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> +			if (on)
> +				port->rx_conf[qid].offloads |=
> DEV_RX_OFFLOAD_JUMBO_FRAME;
> +			else
> +				port->rx_conf[qid].offloads &=
> ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> +		}
> +	}
> +
> +	/* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
> +	 * if unset do it here
> +	 */
> +	if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
> +		ret = rte_eth_dev_set_mtu(portid,
> +				port->dev_conf.rxmode.max_rx_pkt_len -
> eth_overhead);
> +		if (ret)
> +			printf("Failed to set MTU to %u for port %u\n",
> +				port->dev_conf.rxmode.max_rx_pkt_len -
> eth_overhead,
> +				portid);
> +	}
> +
> +	return 0;
> +}
> +
>  void
>  init_port_config(void)
>  {
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> 5f2316210726..2f8f5a92e46a 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -1005,6 +1005,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id,
> __rte_unused uint16_t queue,
>  			 __rte_unused void *user_param);
>  void add_tx_dynf_callback(portid_t portid);  void
> remove_tx_dynf_callback(portid_t portid);
> +int update_jumbo_frame_offload(portid_t portid);
> 
>  /*
>   * Work-around of a compilation error with ICC on invocations of the
> --
> 2.29.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-27  3:04         ` Li, Xiaoyun
@ 2021-01-28  1:57           ` Chen, BoX C
  0 siblings, 0 replies; 38+ messages in thread
From: Chen, BoX C @ 2021-01-28  1:57 UTC (permalink / raw)
  To: Li, Xiaoyun, Yigit, Ferruh, Lu, Wenzhuo, Iremonger, Bernard,
	Yang, SteveX
  Cc: dev, stable, lance.richardson, oulijun, wisamm, lihuisong

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Li, Xiaoyun
> Sent: January 27, 2021 11:05
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Yang, SteveX <stevex.yang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; lance.richardson@broadcom.com;
> oulijun@huawei.com; wisamm@mellanox.com; lihuisong@huawei.com
> Subject: Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum
> packet length
> 
> Except a minor typo inline.
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> 

Tested-by: Chen, BoX C <box.c.chen@intel.com>


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
                           ` (2 preceding siblings ...)
  2021-01-27  3:04         ` Li, Xiaoyun
@ 2021-01-28  9:18         ` Wisam Monther
  2021-01-28  9:26           ` Ferruh Yigit
  2021-01-28 12:07         ` [dpdk-dev] [PATCH v6] " Ferruh Yigit
  4 siblings, 1 reply; 38+ messages in thread
From: Wisam Monther @ 2021-01-28  9:18 UTC (permalink / raw)
  To: Ferruh Yigit, Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: dev, stable, lance.richardson, oulijun, lihuisong

Hi Ferruh,


> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, January 25, 2021 8:16 PM
> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
> <xiaoyun.li@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> lance.richardson@broadcom.com; oulijun@huawei.com; Wisam Monther
> <wisamm@nvidia.com>; lihuisong@huawei.com
> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
> 
> From: Steve Yang <stevex.yang@intel.com>
> 
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> 
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
> 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the flag
> status needs to be calculated whenever 'max_rx_pkt_len' changes.
> 
> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
> 
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
> 
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
> zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
> 
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> 	--rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518  port start all
> 
> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure
> port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
> 
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
> packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> 
> v5:
> * 'update_jumbo_frame_offload()' helper updated
>   * check zero 'max-pkt-len' value
>   * Update how queue offload flags updated
>   * Update MTU if JUMBO_FRAME flag is not set
> * Default testpmd 'max-pkt-len' value set to zero
> 
> Cc: lance.richardson@broadcom.com
> Cc: oulijun@huawei.com
> Cc: wisamm@mellanox.com
> Cc: lihuisong@huawei.com
> ---

I think we need to have https://bugs.dpdk.org/show_bug.cgi?id=625 ID in the commit log as fix,
In order to allow the scripts to close related bugs directly from Bugzilla.

BRs,
Wisam Jaddo

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-28  9:18         ` Wisam Monther
@ 2021-01-28  9:26           ` Ferruh Yigit
  2021-01-28 11:08             ` Wisam Monther
  0 siblings, 1 reply; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-28  9:26 UTC (permalink / raw)
  To: Wisam Monther, Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: dev, stable, lance.richardson, oulijun, lihuisong

On 1/28/2021 9:18 AM, Wisam Monther wrote:
> Hi Ferruh,
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, January 25, 2021 8:16 PM
>> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
>> <xiaoyun.li@intel.com>; Bernard Iremonger
>> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
>> lance.richardson@broadcom.com; oulijun@huawei.com; Wisam Monther
>> <wisamm@nvidia.com>; lihuisong@huawei.com
>> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
>>
>> From: Steve Yang <stevex.yang@intel.com>
>>
>> "port config all max-pkt-len" command fails because it doesn't set the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
>>
>> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
>> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
>> 'init_config()'.
>> 'init_config()' function is only called during testpmd startup, but the flag
>> status needs to be calculated whenever 'max_rx_pkt_len' changes.
>>
>> The issue can be reproduce as [1], where the 'max-pkt-len' reduced and
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
>> didn't.
>>
>> Adding the 'update_jumbo_frame_offload()' helper function to update
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
>> function is called both by 'init_config()' and
>> 'cmd_config_max_pkt_len_parsed()'.
>>
>> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
>> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it is
>> zero.
>> If '--max-pkt-len=N' argument provided, it will be used instead.
>> And with each "port config all max-pkt-len" command, the
>> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
>> updated.
>>
>> [1]
>> --------------------------------------------------------------------------
>> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
>> 	--rxq=4 --txq=4 --disable-rss
>> testpmd>  set verbose 3
>> testpmd>  port stop all
>> testpmd>  port config all max-pkt-len 1518  port start all
>>
>> // Got fail error info without this patch Configuring Port 0 (socket 1) Ethdev
>> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
>> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to configure
>> port 0 rx queues //<-- Fail error info;
>> --------------------------------------------------------------------------
>>
>> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
>> packets")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Steve Yang <stevex.yang@intel.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>
>> v5:
>> * 'update_jumbo_frame_offload()' helper updated
>>    * check zero 'max-pkt-len' value
>>    * Update how queue offload flags updated
>>    * Update MTU if JUMBO_FRAME flag is not set
>> * Default testpmd 'max-pkt-len' value set to zero
>>
>> Cc: lance.richardson@broadcom.com
>> Cc: oulijun@huawei.com
>> Cc: wisamm@mellanox.com
>> Cc: lihuisong@huawei.com
>> ---
> 
> I think we need to have https://bugs.dpdk.org/show_bug.cgi?id=625 ID in the commit log as fix,


Sure, I will send a new version with suggested updates, most probably today, I 
can add the Bugzilla information too.

> In order to allow the scripts to close related bugs directly from Bugzilla.
> 

Scripts? Do we have scripts that close defects automatically, I wasn't aware of 
it, where does it run?


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-28  9:26           ` Ferruh Yigit
@ 2021-01-28 11:08             ` Wisam Monther
  0 siblings, 0 replies; 38+ messages in thread
From: Wisam Monther @ 2021-01-28 11:08 UTC (permalink / raw)
  To: Ferruh Yigit, Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: dev, stable, lance.richardson, oulijun, lihuisong



> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 28, 2021 11:27 AM
> To: Wisam Monther <wisamm@nvidia.com>; Wenzhuo Lu
> <wenzhuo.lu@intel.com>; Xiaoyun Li <xiaoyun.li@intel.com>; Bernard
> Iremonger <bernard.iremonger@intel.com>; Steve Yang
> <stevex.yang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org; lance.richardson@broadcom.com;
> oulijun@huawei.com; lihuisong@huawei.com
> Subject: Re: [PATCH v5] app/testpmd: fix setting maximum packet length
> 
> On 1/28/2021 9:18 AM, Wisam Monther wrote:
> > Hi Ferruh,
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Monday, January 25, 2021 8:16 PM
> >> To: Wenzhuo Lu <wenzhuo.lu@intel.com>; Xiaoyun Li
> >> <xiaoyun.li@intel.com>; Bernard Iremonger
> >> <bernard.iremonger@intel.com>; Steve Yang <stevex.yang@intel.com>
> >> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org;
> >> stable@dpdk.org; lance.richardson@broadcom.com;
> oulijun@huawei.com;
> >> Wisam Monther <wisamm@nvidia.com>; lihuisong@huawei.com
> >> Subject: [PATCH v5] app/testpmd: fix setting maximum packet length
> >>
> >> From: Steve Yang <stevex.yang@intel.com>
> >>
> >> "port config all max-pkt-len" command fails because it doesn't set
> >> the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> >>
> >> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME'
> >> offload flag update from 'cmd_config_max_pkt_len_parsed()' to
> >> 'init_config()'.
> >> 'init_config()' function is only called during testpmd startup, but
> >> the flag status needs to be calculated whenever 'max_rx_pkt_len'
> changes.
> >>
> >> The issue can be reproduce as [1], where the 'max-pkt-len' reduced
> >> and 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared
> but
> >> it didn't.
> >>
> >> Adding the 'update_jumbo_frame_offload()' helper function to update
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'.
> This
> >> function is called both by 'init_config()' and
> >> 'cmd_config_max_pkt_len_parsed()'.
> >>
> >> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> >> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when
> >> it is zero.
> >> If '--max-pkt-len=N' argument provided, it will be used instead.
> >> And with each "port config all max-pkt-len" command, the
> >> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU
> is
> >> updated.
> >>
> >> [1]
> >> ---------------------------------------------------------------------
> >> ----- dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000
> >> --tx-offloads=0x8000
> >> 	--rxq=4 --txq=4 --disable-rss
> >> testpmd>  set verbose 3
> >> testpmd>  port stop all
> >> testpmd>  port config all max-pkt-len 1518  port start all
> >>
> >> // Got fail error info without this patch Configuring Port 0 (socket
> >> 1) Ethdev
> >> port_id=0 rx_queue_id=0, new added offloads 0x800 must be within per-
> >> queue offload capabilities 0x0 in rte_eth_rx_queue_setup() Fail to
> >> configure port 0 rx queues //<-- Fail error info;
> >> ---------------------------------------------------------------------
> >> -----
> >>
> >> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN
> >> packets")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >>
> >> v5:
> >> * 'update_jumbo_frame_offload()' helper updated
> >>    * check zero 'max-pkt-len' value
> >>    * Update how queue offload flags updated
> >>    * Update MTU if JUMBO_FRAME flag is not set
> >> * Default testpmd 'max-pkt-len' value set to zero
> >>
> >> Cc: lance.richardson@broadcom.com
> >> Cc: oulijun@huawei.com
> >> Cc: wisamm@mellanox.com
> >> Cc: lihuisong@huawei.com
> >> ---
> >
> > I think we need to have https://bugs.dpdk.org/show_bug.cgi?id=625 ID
> > in the commit log as fix,
> 
> 
> Sure, I will send a new version with suggested updates, most probably today,
> I can add the Bugzilla information too.
> 
> > In order to allow the scripts to close related bugs directly from Bugzilla.
> >
> 
> Scripts? Do we have scripts that close defects automatically, I wasn't aware of
> it, where does it run?

It's something Thomas using and run it every time to time, it's not published yet.
But it closes merged bugs, adding a link to the commit as comment.



^ permalink raw reply	[flat|nested] 38+ messages in thread

* [dpdk-dev] [PATCH v6] app/testpmd: fix setting maximum packet length
  2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
                           ` (3 preceding siblings ...)
  2021-01-28  9:18         ` Wisam Monther
@ 2021-01-28 12:07         ` Ferruh Yigit
  2021-01-29  9:34           ` Ferruh Yigit
  4 siblings, 1 reply; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-28 12:07 UTC (permalink / raw)
  To: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: Ferruh Yigit, dev, stable, Lance Richardson, Wisam Jaddo,
	Bo Chen, oulijun, wisamm, lihuisong

From: Steve Yang <stevex.yang@intel.com>

"port config all max-pkt-len" command fails because it doesn't set the
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.

Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
'init_config()' function is only called during testpmd startup, but the
flag status needs to be calculated whenever 'max_rx_pkt_len' changes.

The issue can be reproduced as [1], where the 'max-pkt-len' reduced and
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
didn't.

Adding the 'update_jumbo_frame_offload()' helper function to update
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
function is called both by 'init_config()' and
'cmd_config_max_pkt_len_parsed()'.

Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
is zero.
If '--max-pkt-len=N' argument provided, it will be used instead.
And with each "port config all max-pkt-len" command, the
'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
updated.

[1]
--------------------------------------------------------------------------
dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
	--rxq=4 --txq=4 --disable-rss
testpmd>  set verbose 3
testpmd>  port stop all
testpmd>  port config all max-pkt-len 1518
testpmd>  port start all

// Got fail error info without this patch
Configuring Port 0 (socket 1)
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
Fail to configure port 0 rx queues //<-- Fail error info;
--------------------------------------------------------------------------

Bugzilla ID: 625
Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN packets")
Cc: stable@dpdk.org

Signed-off-by: Steve Yang <stevex.yang@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Lance Richardson <lance.richardson@broadcom.com>
Acked-by: Wisam Jaddo <wisamm@nvidia.com>
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
Tested-by: Bo Chen <box.c.chen@intel.com>
---
v5:
* 'update_jumbo_frame_offload()' helper updated
  * check zero 'max-pkt-len' value
  * Update how queue offload flags updated
  * Update MTU if JUMBO_FRAME flag is not set
* Default testpmd 'max-pkt-len' value set to zero

Cc: lance.richardson@broadcom.com
Cc: oulijun@huawei.com
Cc: wisamm@mellanox.com
Cc: lihuisong@huawei.com

v6:
* Comment wording updated
* Bugzilla id added to the commit log
---
 app/test-pmd/cmdline.c |  13 ++++++
 app/test-pmd/testpmd.c | 102 +++++++++++++++++++++++++++++++++--------
 app/test-pmd/testpmd.h |   1 +
 3 files changed, 97 insertions(+), 19 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index c6ace2dd2546..b338f5fe6333 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1877,7 +1877,9 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 				__rte_unused void *data)
 {
 	struct cmd_config_max_pkt_len_result *res = parsed_result;
+	uint32_t max_rx_pkt_len_backup = 0;
 	portid_t pid;
+	int ret;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -1896,7 +1898,18 @@ cmd_config_max_pkt_len_parsed(void *parsed_result,
 			if (res->value == port->dev_conf.rxmode.max_rx_pkt_len)
 				return;
 
+			ret = eth_dev_info_get_print_err(pid, &port->dev_info);
+			if (ret != 0) {
+				printf("rte_eth_dev_info_get() failed for port %u\n",
+					pid);
+				return;
+			}
+
+			max_rx_pkt_len_backup = port->dev_conf.rxmode.max_rx_pkt_len;
+
 			port->dev_conf.rxmode.max_rx_pkt_len = res->value;
+			if (update_jumbo_frame_offload(pid) != 0)
+				port->dev_conf.rxmode.max_rx_pkt_len = max_rx_pkt_len_backup;
 		} else {
 			printf("Unknown parameter\n");
 			return;
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index c256e719aea2..555852ae5e42 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -443,8 +443,11 @@ lcoreid_t latencystats_lcore_id = -1;
  * Ethernet device configuration.
  */
 struct rte_eth_rxmode rx_mode = {
-	.max_rx_pkt_len = RTE_ETHER_MAX_LEN,
-		/**< Default maximum frame length. */
+	/* Default maximum frame length.
+	 * Zero is converted to "RTE_ETHER_MTU + PMD Ethernet overhead"
+	 * in init_config().
+	 */
+	.max_rx_pkt_len = 0,
 };
 
 struct rte_eth_txmode tx_mode = {
@@ -1410,7 +1413,6 @@ init_config(void)
 	struct rte_gro_param gro_param;
 	uint32_t gso_types;
 	uint16_t data_size;
-	uint16_t eth_overhead;
 	bool warning = 0;
 	int k;
 	int ret;
@@ -1447,22 +1449,10 @@ init_config(void)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_dev_info_get() failed\n");
 
-		/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
-		if (port->dev_info.max_mtu != UINT16_MAX &&
-		    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
-			eth_overhead = port->dev_info.max_rx_pktlen -
-				port->dev_info.max_mtu;
-		else
-			eth_overhead =
-				RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
-
-		if (port->dev_conf.rxmode.max_rx_pkt_len <=
-			(uint32_t)(RTE_ETHER_MTU + eth_overhead))
-			port->dev_conf.rxmode.max_rx_pkt_len =
-					RTE_ETHER_MTU + eth_overhead;
-		else
-			port->dev_conf.rxmode.offloads |=
-					DEV_RX_OFFLOAD_JUMBO_FRAME;
+		ret = update_jumbo_frame_offload(pid);
+		if (ret != 0)
+			printf("Updating jumbo frame offload failed for port %u\n",
+				pid);
 
 		if (!(port->dev_info.tx_offload_capa &
 		      DEV_TX_OFFLOAD_MBUF_FAST_FREE))
@@ -3358,6 +3348,80 @@ rxtx_port_config(struct rte_port *port)
 	}
 }
 
+/*
+ * Helper function to arrange max_rx_pktlen value and JUMBO_FRAME offload,
+ * MTU is also aligned if JUMBO_FRAME offload is not set.
+ *
+ * port->dev_info should be set before calling this function.
+ *
+ * return 0 on success, negative on error
+ */
+int
+update_jumbo_frame_offload(portid_t portid)
+{
+	struct rte_port *port = &ports[portid];
+	uint32_t eth_overhead;
+	uint64_t rx_offloads;
+	int ret;
+	bool on;
+
+	/* Update the max_rx_pkt_len to have MTU as RTE_ETHER_MTU */
+	if (port->dev_info.max_mtu != UINT16_MAX &&
+	    port->dev_info.max_rx_pktlen > port->dev_info.max_mtu)
+		eth_overhead = port->dev_info.max_rx_pktlen -
+				port->dev_info.max_mtu;
+	else
+		eth_overhead = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+	rx_offloads = port->dev_conf.rxmode.offloads;
+
+	/* Default config value is 0 to use PMD specific overhead */
+	if (port->dev_conf.rxmode.max_rx_pkt_len == 0)
+		port->dev_conf.rxmode.max_rx_pkt_len = RTE_ETHER_MTU + eth_overhead;
+
+	if (port->dev_conf.rxmode.max_rx_pkt_len <= RTE_ETHER_MTU + eth_overhead) {
+		rx_offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+		on = false;
+	} else {
+		if ((port->dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
+			printf("Frame size (%u) is not supported by port %u\n",
+				port->dev_conf.rxmode.max_rx_pkt_len,
+				portid);
+			return -1;
+		}
+		rx_offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+		on = true;
+	}
+
+	if (rx_offloads != port->dev_conf.rxmode.offloads) {
+		uint16_t qid;
+
+		port->dev_conf.rxmode.offloads = rx_offloads;
+
+		/* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
+		for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
+			if (on)
+				port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
+			else
+				port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
+		}
+	}
+
+	/* If JUMBO_FRAME is set MTU conversion done by ethdev layer,
+	 * if unset do it here
+	 */
+	if ((rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) {
+		ret = rte_eth_dev_set_mtu(portid,
+				port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead);
+		if (ret)
+			printf("Failed to set MTU to %u for port %u\n",
+				port->dev_conf.rxmode.max_rx_pkt_len - eth_overhead,
+				portid);
+	}
+
+	return 0;
+}
+
 void
 init_port_config(void)
 {
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 5f2316210726..2f8f5a92e46a 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -1005,6 +1005,7 @@ uint16_t tx_pkt_set_dynf(uint16_t port_id, __rte_unused uint16_t queue,
 			 __rte_unused void *user_param);
 void add_tx_dynf_callback(portid_t portid);
 void remove_tx_dynf_callback(portid_t portid);
+int update_jumbo_frame_offload(portid_t portid);
 
 /*
  * Work-around of a compilation error with ICC on invocations of the
-- 
2.29.2


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-26 11:01               ` Ferruh Yigit
@ 2021-01-28 21:36                 ` Lance Richardson
  2021-01-28 22:12                   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
  0 siblings, 1 reply; 38+ messages in thread
From: Lance Richardson @ 2021-01-28 21:36 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

[-- Attachment #1: Type: text/plain, Size: 3342 bytes --]

On Tue, Jan 26, 2021 at 6:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 1/26/2021 3:45 AM, Lance Richardson wrote:
> > On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >>
> >>>> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
> >>>> +               uint16_t qid;
> >>>> +
> >>>> +               port->dev_conf.rxmode.offloads = rx_offloads;
> >>>> +
> >>>> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
> >>>> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
> >>>> +                       if (on)
> >>>> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> >>>> +                       else
> >>>> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
> >>>> +               }
> >>>
> >>> Is it correct to set per-queue offloads that aren't advertised by the PMD
> >>> as supported in rx_queue_offload_capa?
> >>>
> >>
> >> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
> >> are reflected to 'port->rx_conf[].offloads' for all queues.
> >>
> >> We should set the offload in 'port->rx_conf[].offloads' if it is set in
> >> 'port->dev_conf.rxmode.offloads'.
> >>
> >> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
> >> it. And the port level capability is already checked above.
> >>
> >
> > I'm still not 100% clear about the per-queue offload question.
> >
> > With this patch, and jumbo max packet size configured (on the command
> > line in this case), I see:
> >
> > testpmd> show port 0 rx_offload configuration
> > Rx Offloading Configuration of port 0 :
> >    Port : JUMBO_FRAME
> >    Queue[ 0] : JUMBO_FRAME
> >
> > testpmd> show port 0 rx_offload capabilities
> > Rx Offloading Capabilities of port 0 :
> >    Per Queue :
> >    Per Port  : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> > OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
> > KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
> >
>
> The port level offload is applied to all queues on the port, testpmd config
> structure reflects this logic in implementation.
> If Rx offload X is set for a port, it is set for all Rx queue offloads, this is
> not new behavior and not related to this patch.
>
OK, is this purely for display purposes within testpmd? I ask because
it appears that all PMDs supporting per-queue offload configuration already
take care of combining port-level and per-queue offloads within their
tx_queue_setup()/rx_queue_setup() functions and then track the combined
set of offloads within a per-queue field, e.g. this line is common to
e1000/i40e/ionic/ixgbe/octeontx2/thunderx/txgbe rx_queue_setup()
implementations:
    offloads = rx_conf->offloads | dev->data->dev_conf.rxmode.offloads;

And rte_ethdev.h says:
   No need to repeat any bit in rx_conf->offloads which has already been
   enabled in rte_eth_dev_configure() at port level. An offloading enabled
  at port level can't be disabled at queue level.

Which I suppose confirms that if testpmd is combining per-port and per-
queue offloads, it's just for the purposes of testpmd.

Apologies for worrying at this even more, I just wanted to be sure that
I understand what the PMD is expected to do.

Regards,
    Lance

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v5] app/testpmd: fix setting maximum packet length
  2021-01-28 21:36                 ` Lance Richardson
@ 2021-01-28 22:12                   ` Ferruh Yigit
  0 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-28 22:12 UTC (permalink / raw)
  To: Lance Richardson
  Cc: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang, dev,
	stable, oulijun, wisamm, lihuisong

On 1/28/2021 9:36 PM, Lance Richardson wrote:
> On Tue, Jan 26, 2021 at 6:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 1/26/2021 3:45 AM, Lance Richardson wrote:
>>> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>>
>>>>>> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
>>>>>> +               uint16_t qid;
>>>>>> +
>>>>>> +               port->dev_conf.rxmode.offloads = rx_offloads;
>>>>>> +
>>>>>> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>>>>>> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>>>>>> +                       if (on)
>>>>>> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>>>> +                       else
>>>>>> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>>>> +               }
>>>>>
>>>>> Is it correct to set per-queue offloads that aren't advertised by the PMD
>>>>> as supported in rx_queue_offload_capa?
>>>>>
>>>>
>>>> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
>>>> are reflected to 'port->rx_conf[].offloads' for all queues.
>>>>
>>>> We should set the offload in 'port->rx_conf[].offloads' if it is set in
>>>> 'port->dev_conf.rxmode.offloads'.
>>>>
>>>> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
>>>> it. And the port level capability is already checked above.
>>>>
>>>
>>> I'm still not 100% clear about the per-queue offload question.
>>>
>>> With this patch, and jumbo max packet size configured (on the command
>>> line in this case), I see:
>>>
>>> testpmd> show port 0 rx_offload configuration
>>> Rx Offloading Configuration of port 0 :
>>>     Port : JUMBO_FRAME
>>>     Queue[ 0] : JUMBO_FRAME
>>>
>>> testpmd> show port 0 rx_offload capabilities
>>> Rx Offloading Capabilities of port 0 :
>>>     Per Queue :
>>>     Per Port  : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
>>> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
>>> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
>>>
>>
>> The port level offload is applied to all queues on the port, testpmd config
>> structure reflects this logic in implementation.
>> If Rx offload X is set for a port, it is set for all Rx queue offloads, this is
>> not new behavior and not related to this patch.
>>
> OK, is this purely for display purposes within testpmd? I ask because
> it appears that all PMDs supporting per-queue offload configuration already
> take care of combining port-level and per-queue offloads within their
> tx_queue_setup()/rx_queue_setup() functions and then track the combined
> set of offloads within a per-queue field, e.g. this line is common to
> e1000/i40e/ionic/ixgbe/octeontx2/thunderx/txgbe rx_queue_setup()
> implementations:
>      offloads = rx_conf->offloads | dev->data->dev_conf.rxmode.offloads;
> 
> And rte_ethdev.h says:
>     No need to repeat any bit in rx_conf->offloads which has already been
>     enabled in rte_eth_dev_configure() at port level. An offloading enabled
>    at port level can't be disabled at queue level.
> 
> Which I suppose confirms that if testpmd is combining per-port and per-
> queue offloads, it's just for the purposes of testpmd.
> 

Yes it is just for purposed of testpmd (application), but doesn't need to be 
just limited to display purpose, testpmd keeps the config locally for any need.

> Apologies for worrying at this even more, I just wanted to be sure that
> I understand what the PMD is expected to do.
> 

PMDs should not be getting these offloads in queue setup, since ethdev layer 
strips the port level offloads, I mean:

if port level offloads: X, Y

And applications provide following for queue_setup():
a) X
b) Y
c) X, Y

For all three PMD receives empty queue offload request.

d) X, Y, Z

PMD gets Z if it is in drivers queue specific capabilities.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [dpdk-dev] [PATCH v6] app/testpmd: fix setting maximum packet length
  2021-01-28 12:07         ` [dpdk-dev] [PATCH v6] " Ferruh Yigit
@ 2021-01-29  9:34           ` Ferruh Yigit
  0 siblings, 0 replies; 38+ messages in thread
From: Ferruh Yigit @ 2021-01-29  9:34 UTC (permalink / raw)
  To: Wenzhuo Lu, Xiaoyun Li, Bernard Iremonger, Steve Yang
  Cc: dev, stable, Lance Richardson, Wisam Jaddo, Bo Chen, oulijun,
	wisamm, lihuisong

On 1/28/2021 12:07 PM, Ferruh Yigit wrote:
> From: Steve Yang <stevex.yang@intel.com>
> 
> "port config all max-pkt-len" command fails because it doesn't set the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag properly.
> 
> Commit in the fixes line moved the 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload
> flag update from 'cmd_config_max_pkt_len_parsed()' to 'init_config()'.
> 'init_config()' function is only called during testpmd startup, but the
> flag status needs to be calculated whenever 'max_rx_pkt_len' changes.
> 
> The issue can be reproduced as [1], where the 'max-pkt-len' reduced and
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag should be cleared but it
> didn't.
> 
> Adding the 'update_jumbo_frame_offload()' helper function to update
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag and 'max_rx_pkt_len'. This
> function is called both by 'init_config()' and
> 'cmd_config_max_pkt_len_parsed()'.
> 
> Default 'max-pkt-len' value set to zero, 'update_jumbo_frame_offload()'
> updates it to "RTE_ETHER_MTU + PMD specific Ethernet overhead" when it
> is zero.
> If '--max-pkt-len=N' argument provided, it will be used instead.
> And with each "port config all max-pkt-len" command, the
> 'DEV_RX_OFFLOAD_JUMBO_FRAME' offload flag, 'max-pkt-len' and MTU is
> updated.
> 
> [1]
> --------------------------------------------------------------------------
> dpdk-testpmd -c 0xf -n 4 -- -i --max-pkt-len=9000 --tx-offloads=0x8000
> 	--rxq=4 --txq=4 --disable-rss
> testpmd>  set verbose 3
> testpmd>  port stop all
> testpmd>  port config all max-pkt-len 1518
> testpmd>  port start all
> 
> // Got fail error info without this patch
> Configuring Port 0 (socket 1)
> Ethdev port_id=0 rx_queue_id=0, new added offloads 0x800 must be
> within per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()
> Fail to configure port 0 rx queues //<-- Fail error info;
> --------------------------------------------------------------------------
> 
> Bugzilla ID: 625
> Fixes: 761c4d66900f ("app/testpmd: fix max Rx packet length for VLAN packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Lance Richardson <lance.richardson@broadcom.com>
> Acked-by: Wisam Jaddo <wisamm@nvidia.com>
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> Tested-by: Bo Chen <box.c.chen@intel.com>

Applied to dpdk-next-net/main, thanks.


^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2021-01-29  9:34 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-22  8:13 [dpdk-dev] [PATCH v1] app/testpmd: fix dynamic config error for max-pkt-len Steve Yang
2020-12-23  2:27 ` Li, Xiaoyun
2020-12-23  8:51 ` [dpdk-dev] [PATCH v2] app/testpmd: fix dynamic config error Steve Yang
2020-12-23  9:00   ` Li, Xiaoyun
2021-01-13  8:13   ` Chen, BoX C
2021-01-19 15:44   ` Ferruh Yigit
2021-01-22  9:01   ` [dpdk-dev] [PATCH v3 0/3] fix 'max-pkt-len' errors Steve Yang
2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
2021-01-25  7:12       ` Huisong Li
     [not found]         ` <DM6PR11MB43628A600BAAEC75FFF1343BF9BD9@DM6PR11MB4362.namprd11.prod.outlook.com>
2021-01-25 12:38           ` Ferruh Yigit
2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: fix max-pkt-len option invalid Steve Yang
2021-01-22  9:01     ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: fix dynamic config error Steve Yang
2021-01-22 17:04       ` Ferruh Yigit
2021-01-22 17:15         ` Ferruh Yigit
2021-01-25  8:32     ` [dpdk-dev] [PATCH v4 0/2] fix 'max-pkt-len' errors Steve Yang
2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 1/2] ethdev: fix MTU doesn't update when jumbo frame disabled Steve Yang
2021-01-25 12:41         ` Ferruh Yigit
2021-01-25  8:32       ` [dpdk-dev] [PATCH v4 2/2] app/testpmd: fix max-pkt-len option invalid Steve Yang
2021-01-25 14:45         ` Ferruh Yigit
2021-01-25 15:46         ` Lance Richardson
2021-01-25 17:57           ` Ferruh Yigit
2021-01-25 18:15       ` [dpdk-dev] [PATCH v5] app/testpmd: fix setting maximum packet length Ferruh Yigit
2021-01-25 19:41         ` Lance Richardson
2021-01-26  0:44           ` Ferruh Yigit
2021-01-26  3:22             ` Lance Richardson
2021-01-26  3:45             ` Lance Richardson
2021-01-26  7:54               ` Li, Xiaoyun
2021-01-26 11:01               ` Ferruh Yigit
2021-01-28 21:36                 ` Lance Richardson
2021-01-28 22:12                   ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2021-01-26  9:02         ` [dpdk-dev] " Wisam Monther
2021-01-27  3:04         ` Li, Xiaoyun
2021-01-28  1:57           ` Chen, BoX C
2021-01-28  9:18         ` Wisam Monther
2021-01-28  9:26           ` Ferruh Yigit
2021-01-28 11:08             ` Wisam Monther
2021-01-28 12:07         ` [dpdk-dev] [PATCH v6] " Ferruh Yigit
2021-01-29  9:34           ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git