[PATCH 0/2] bugfix for testpmd

[PATCH 0/2] bugfix for testpmd

[Min Hu (Connor)]

This patch set contains two bugfixes for testpmd.

Huisong Li (1):
  app/testpmd: fix incorrect MTU verification

Min Hu (Connor) (1):
  app/testpmd: fix stats get when display fwd stats

 app/test-pmd/cmdline.c |  4 ---
 app/test-pmd/config.c  | 65 ++++++++++++++++++++++++++++++++++++++++--
 app/test-pmd/testpmd.c | 16 +++++++++--
 3 files changed, 77 insertions(+), 8 deletions(-)

-- 
2.33.0

[PATCH 1/2] app/testpmd: fix stats get when display fwd stats

[Min Hu (Connor)]

In function 'fwd_stats_display', if function 'rte_eth_stats_get' fails,
'stats' is uncertainty value. The display result will be abnormal.

This patch check the return value of 'rte_eth_stats_get' to avoid
display abnormal stats.

Fixes: 53324971a14e ("app/testpmd: display/clear forwarding stats on demand")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test-pmd/config.c  | 10 ++++++++--
 app/test-pmd/testpmd.c | 16 ++++++++++++++--
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index cc8e7aa138..bd689f9f86 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -249,14 +249,20 @@ nic_stats_display(portid_t port_id)
 								diff_ns;
 	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
 	struct rte_eth_stats stats;
-
 	static const char *nic_stats_border = "########################";
+	int ret;
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		print_valid_ports();
 		return;
 	}
-	rte_eth_stats_get(port_id, &stats);
+	ret = rte_eth_stats_get(port_id, &stats);
+	if (ret != 0) {
+		fprintf(stderr,
+			"%s: Error: failed to get stats (port %u): %d",
+			__func__, port_id, ret);
+		return;
+	}
 	printf("\n  %s NIC statistics for port %-2d %s\n",
 	       nic_stats_border, port_id, nic_stats_border);
 
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe2ce19f99..79bb23264b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1982,6 +1982,7 @@ fwd_stats_display(void)
 	struct rte_port *port;
 	streamid_t sm_id;
 	portid_t pt_id;
+	int ret;
 	int i;
 
 	memset(ports_stats, 0, sizeof(ports_stats));
@@ -2013,7 +2014,13 @@ fwd_stats_display(void)
 		pt_id = fwd_ports_ids[i];
 		port = &ports[pt_id];
 
-		rte_eth_stats_get(pt_id, &stats);
+		ret = rte_eth_stats_get(pt_id, &stats);
+		if (ret != 0) {
+			fprintf(stderr,
+				"%s: Error: failed to get stats (port %u): %d",
+				__func__, pt_id, ret);
+			continue;
+		}
 		stats.ipackets -= port->stats.ipackets;
 		stats.opackets -= port->stats.opackets;
 		stats.ibytes -= port->stats.ibytes;
@@ -2108,11 +2115,16 @@ fwd_stats_reset(void)
 {
 	streamid_t sm_id;
 	portid_t pt_id;
+	int ret;
 	int i;
 
 	for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
 		pt_id = fwd_ports_ids[i];
-		rte_eth_stats_get(pt_id, &ports[pt_id].stats);
+		ret = rte_eth_stats_get(pt_id, &ports[pt_id].stats);
+		if (ret != 0)
+			fprintf(stderr,
+				"%s: Error: failed to clear stats (port %u):%d",
+				__func__, pt_id, ret);
 	}
 	for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) {
 		struct fwd_stream *fs = fwd_streams[sm_id];
-- 
2.33.0

[PATCH 2/2] app/testpmd: fix incorrect MTU verification

[Min Hu (Connor)]

From: Huisong Li <lihuisong@huawei.com>

The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
used it when execute 'port config mtu 0 xx' cmd. This patch fix it.

Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test-pmd/cmdline.c |  4 ---
 app/test-pmd/config.c  | 55 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 6ffea8e21a..91e4090582 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2050,10 +2050,6 @@ cmd_config_mtu_parsed(void *parsed_result,
 {
 	struct cmd_config_mtu_result *res = parsed_result;
 
-	if (res->value < RTE_ETHER_MIN_LEN) {
-		fprintf(stderr, "mtu cannot be less than %d\n", RTE_ETHER_MIN_LEN);
-		return;
-	}
 	port_mtu_set(res->port_id, res->value);
 }
 
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index bd689f9f86..1b1e738f83 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1254,6 +1254,57 @@ port_reg_set(portid_t port_id, uint32_t reg_off, uint32_t reg_v)
 	display_port_reg_value(port_id, reg_off, reg_v);
 }
 
+static uint32_t
+eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
+{
+	uint32_t overhead_len;
+
+	if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
+		overhead_len = max_rx_pktlen - max_mtu;
+	else
+		overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+
+	return overhead_len;
+}
+
+static int
+eth_dev_validate_mtu(uint16_t port_id, uint16_t mtu)
+{
+	struct rte_eth_dev_info dev_info;
+	uint32_t overhead_len;
+	uint32_t frame_size;
+	int ret;
+
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
+
+	if (mtu < dev_info.min_mtu) {
+		fprintf(stderr,
+			"MTU (%u) < device min MTU (%u) for port_id %u\n",
+			mtu, dev_info.min_mtu, port_id);
+		return -EINVAL;
+	}
+	if (mtu > dev_info.max_mtu) {
+		fprintf(stderr,
+			"MTU (%u) > device max MTU (%u) for port_id %u\n",
+			mtu, dev_info.max_mtu, port_id);
+		return -EINVAL;
+	}
+
+	overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
+			dev_info.max_mtu);
+	frame_size = mtu + overhead_len;
+	if (frame_size > dev_info.max_rx_pktlen) {
+		fprintf(stderr,
+			"Frame size (%u) > device max frame size (%u) for port_id %u\n",
+			frame_size, dev_info.max_rx_pktlen, port_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 void
 port_mtu_set(portid_t port_id, uint16_t mtu)
 {
@@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
 	if (port_id_is_invalid(port_id, ENABLED_WARN))
 		return;
 
+	diag = eth_dev_validate_mtu(port_id, mtu);
+	if (diag != 0)
+		return;
+
 	if (port->need_reconfig == 0) {
 		diag = rte_eth_dev_set_mtu(port_id, mtu);
 		if (diag != 0) {
-- 
2.33.0

Re: [PATCH 0/2] bugfix for testpmd

[Min Hu (Connor)]

Hi, Ferruh,
	what do you think of this patch?

在 2022/4/6 16:45, Min Hu (Connor) 写道:
> This patch set contains two bugfixes for testpmd.
> 
> Huisong Li (1):
>    app/testpmd: fix incorrect MTU verification
> 
> Min Hu (Connor) (1):
>    app/testpmd: fix stats get when display fwd stats
> 
>   app/test-pmd/cmdline.c |  4 ---
>   app/test-pmd/config.c  | 65 ++++++++++++++++++++++++++++++++++++++++--
>   app/test-pmd/testpmd.c | 16 +++++++++--
>   3 files changed, 77 insertions(+), 8 deletions(-)
> 

Re: [PATCH 2/2] app/testpmd: fix incorrect MTU verification

[Singh, Aman Deep]

On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
> used it when execute 'port config mtu 0 xx' cmd. This patch fix it.
>
> Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   app/test-pmd/cmdline.c |  4 ---
>   app/test-pmd/config.c  | 55 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 6ffea8e21a..91e4090582 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2050,10 +2050,6 @@ cmd_config_mtu_parsed(void *parsed_result,
>   {
>   	struct cmd_config_mtu_result *res = parsed_result;
>   
> -	if (res->value < RTE_ETHER_MIN_LEN) {
> -		fprintf(stderr, "mtu cannot be less than %d\n", RTE_ETHER_MIN_LEN);
> -		return;
> -	}
>   	port_mtu_set(res->port_id, res->value);
>   }
>   
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index bd689f9f86..1b1e738f83 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1254,6 +1254,57 @@ port_reg_set(portid_t port_id, uint32_t reg_off, uint32_t reg_v)
>   	display_port_reg_value(port_id, reg_off, reg_v);
>   }
>   
> +static uint32_t
> +eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
> +{
> +	uint32_t overhead_len;
> +
> +	if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
> +		overhead_len = max_rx_pktlen - max_mtu;
> +	else
> +		overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
> +
> +	return overhead_len;
> +}
> +
> +static int
> +eth_dev_validate_mtu(uint16_t port_id, uint16_t mtu)
> +{
> +	struct rte_eth_dev_info dev_info;
> +	uint32_t overhead_len;
> +	uint32_t frame_size;
> +	int ret;
> +
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (mtu < dev_info.min_mtu) {
> +		fprintf(stderr,
> +			"MTU (%u) < device min MTU (%u) for port_id %u\n",
> +			mtu, dev_info.min_mtu, port_id);
> +		return -EINVAL;
> +	}
> +	if (mtu > dev_info.max_mtu) {
> +		fprintf(stderr,
> +			"MTU (%u) > device max MTU (%u) for port_id %u\n",
> +			mtu, dev_info.max_mtu, port_id);
> +		return -EINVAL;
> +	}
> +
> +	overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
> +			dev_info.max_mtu);
> +	frame_size = mtu + overhead_len;
> +	if (frame_size > dev_info.max_rx_pktlen) {
> +		fprintf(stderr,
> +			"Frame size (%u) > device max frame size (%u) for port_id %u\n",
> +			frame_size, dev_info.max_rx_pktlen, port_id);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   void
>   port_mtu_set(portid_t port_id, uint16_t mtu)
>   {
> @@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>   	if (port_id_is_invalid(port_id, ENABLED_WARN))
>   		return;
>   
> +	diag = eth_dev_validate_mtu(port_id, mtu);
> +	if (diag != 0)
> +		return;
> +
>   	if (port->need_reconfig == 0) {
>   		diag = rte_eth_dev_set_mtu(port_id, mtu);
>   		if (diag != 0) {
I just wanted to know if these added functions eth_dev_validate_mtu() &  
eth_dev_get_overhead_len()
are copy of ethdev library API's in file "rte_ethdev.c", which get 
called by rte_eth_dev_set_mtu.
Is our intent, is to call these twice ?

Re: [PATCH 2/2] app/testpmd: fix incorrect MTU verification

[lihuisong (C)]

在 2022/4/26 0:25, Singh, Aman Deep 写道:
>
> On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
>> used it when execute 'port config mtu 0 xx' cmd. This patch fix it.
>>
>> Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c |  4 ---
>>   app/test-pmd/config.c  | 55 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 55 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 6ffea8e21a..91e4090582 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -2050,10 +2050,6 @@ cmd_config_mtu_parsed(void *parsed_result,
>>   {
>>       struct cmd_config_mtu_result *res = parsed_result;
>>   -    if (res->value < RTE_ETHER_MIN_LEN) {
>> -        fprintf(stderr, "mtu cannot be less than %d\n", 
>> RTE_ETHER_MIN_LEN);
>> -        return;
>> -    }
>>       port_mtu_set(res->port_id, res->value);
>>   }
>>   diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index bd689f9f86..1b1e738f83 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -1254,6 +1254,57 @@ port_reg_set(portid_t port_id, uint32_t 
>> reg_off, uint32_t reg_v)
>>       display_port_reg_value(port_id, reg_off, reg_v);
>>   }
>>   +static uint32_t
>> +eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu)
>> +{
>> +    uint32_t overhead_len;
>> +
>> +    if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu)
>> +        overhead_len = max_rx_pktlen - max_mtu;
>> +    else
>> +        overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
>> +
>> +    return overhead_len;
>> +}
>> +
>> +static int
>> +eth_dev_validate_mtu(uint16_t port_id, uint16_t mtu)
>> +{
>> +    struct rte_eth_dev_info dev_info;
>> +    uint32_t overhead_len;
>> +    uint32_t frame_size;
>> +    int ret;
>> +
>> +    ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +    if (ret != 0)
>> +        return ret;
>> +
>> +    if (mtu < dev_info.min_mtu) {
>> +        fprintf(stderr,
>> +            "MTU (%u) < device min MTU (%u) for port_id %u\n",
>> +            mtu, dev_info.min_mtu, port_id);
>> +        return -EINVAL;
>> +    }
>> +    if (mtu > dev_info.max_mtu) {
>> +        fprintf(stderr,
>> +            "MTU (%u) > device max MTU (%u) for port_id %u\n",
>> +            mtu, dev_info.max_mtu, port_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
>> +            dev_info.max_mtu);
>> +    frame_size = mtu + overhead_len;
>> +    if (frame_size > dev_info.max_rx_pktlen) {
>> +        fprintf(stderr,
>> +            "Frame size (%u) > device max frame size (%u) for 
>> port_id %u\n",
>> +            frame_size, dev_info.max_rx_pktlen, port_id);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   void
>>   port_mtu_set(portid_t port_id, uint16_t mtu)
>>   {
>> @@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>>       if (port_id_is_invalid(port_id, ENABLED_WARN))
>>           return;
>>   +    diag = eth_dev_validate_mtu(port_id, mtu);
>> +    if (diag != 0)
>> +        return;
>> +
>>       if (port->need_reconfig == 0) {
>>           diag = rte_eth_dev_set_mtu(port_id, mtu);
>>           if (diag != 0) {
> I just wanted to know if these added functions eth_dev_validate_mtu() 
> &  eth_dev_get_overhead_len()
> are copy of ethdev library API's in file "rte_ethdev.c", which get 
> called by rte_eth_dev_set_mtu.
> Is our intent, is to call these twice ?
If port->need_reconfig is 1, rte_eth_dev_set_mtu doesn't be called, and
MTU value is saved to port->dev_conf.rxmode.mtu. This dev_conf.rxmode.mtu
will be set to driver in dev_configure(). This check is performed to
prevent dev_configure() failure. That's what I think.
> .

Re: [PATCH 1/2] app/testpmd: fix stats get when display fwd stats

[Singh, Aman Deep]

On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
> In function 'fwd_stats_display', if function 'rte_eth_stats_get' fails,
> 'stats' is uncertainty value. The display result will be abnormal.
>
> This patch check the return value of 'rte_eth_stats_get' to avoid
> display abnormal stats.
>
> Fixes: 53324971a14e ("app/testpmd: display/clear forwarding stats on demand")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   app/test-pmd/config.c  | 10 ++++++++--
>   app/test-pmd/testpmd.c | 16 ++++++++++++++--
>   2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index cc8e7aa138..bd689f9f86 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -249,14 +249,20 @@ nic_stats_display(portid_t port_id)
>   								diff_ns;
>   	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
>   	struct rte_eth_stats stats;
> -
>   	static const char *nic_stats_border = "########################";
> +	int ret;
>   
>   	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
>   		print_valid_ports();
>   		return;
>   	}
> -	rte_eth_stats_get(port_id, &stats);
> +	ret = rte_eth_stats_get(port_id, &stats);
> +	if (ret != 0) {
> +		fprintf(stderr,
> +			"%s: Error: failed to get stats (port %u): %d",
> +			__func__, port_id, ret);
> +		return;
> +	}
>   	printf("\n  %s NIC statistics for port %-2d %s\n",
>   	       nic_stats_border, port_id, nic_stats_border);
>   
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index fe2ce19f99..79bb23264b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1982,6 +1982,7 @@ fwd_stats_display(void)
>   	struct rte_port *port;
>   	streamid_t sm_id;
>   	portid_t pt_id;
> +	int ret;
>   	int i;
>   
>   	memset(ports_stats, 0, sizeof(ports_stats));
> @@ -2013,7 +2014,13 @@ fwd_stats_display(void)
>   		pt_id = fwd_ports_ids[i];
>   		port = &ports[pt_id];
>   
> -		rte_eth_stats_get(pt_id, &stats);
> +		ret = rte_eth_stats_get(pt_id, &stats);
> +		if (ret != 0) {
> +			fprintf(stderr,
> +				"%s: Error: failed to get stats (port %u): %d",
> +				__func__, pt_id, ret);
> +			continue;
> +		}
>   		stats.ipackets -= port->stats.ipackets;
>   		stats.opackets -= port->stats.opackets;
>   		stats.ibytes -= port->stats.ibytes;
> @@ -2108,11 +2115,16 @@ fwd_stats_reset(void)
>   {
>   	streamid_t sm_id;
>   	portid_t pt_id;
> +	int ret;
>   	int i;
>   
>   	for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
>   		pt_id = fwd_ports_ids[i];
> -		rte_eth_stats_get(pt_id, &ports[pt_id].stats);
> +		ret = rte_eth_stats_get(pt_id, &ports[pt_id].stats);
> +		if (ret != 0)
> +			fprintf(stderr,
> +				"%s: Error: failed to clear stats (port %u):%d",
> +				__func__, pt_id, ret);
Should we clear "ports[pt_id].stats" in this condition.
>   	}
>   	for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) {
>   		struct fwd_stream *fs = fwd_streams[sm_id];
As such LGTM
Acked-by: Aman Singh <aman.deep.singh@intel.com>

Re: [PATCH 1/2] app/testpmd: fix stats get when display fwd stats

[Min Hu (Connor)]

Hi, Singh,

在 2022/4/27 20:15, Singh, Aman Deep 写道:
> 
> 
> On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
>> In function 'fwd_stats_display', if function 'rte_eth_stats_get' fails,
>> 'stats' is uncertainty value. The display result will be abnormal.
>>
>> This patch check the return value of 'rte_eth_stats_get' to avoid
>> display abnormal stats.
>>
>> Fixes: 53324971a14e ("app/testpmd: display/clear forwarding stats on 
>> demand")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test-pmd/config.c  | 10 ++++++++--
>>   app/test-pmd/testpmd.c | 16 ++++++++++++++--
>>   2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index cc8e7aa138..bd689f9f86 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -249,14 +249,20 @@ nic_stats_display(portid_t port_id)
>>                                   diff_ns;
>>       uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
>>       struct rte_eth_stats stats;
>> -
>>       static const char *nic_stats_border = "########################";
>> +    int ret;
>>       if (port_id_is_invalid(port_id, ENABLED_WARN)) {
>>           print_valid_ports();
>>           return;
>>       }
>> -    rte_eth_stats_get(port_id, &stats);
>> +    ret = rte_eth_stats_get(port_id, &stats);
>> +    if (ret != 0) {
>> +        fprintf(stderr,
>> +            "%s: Error: failed to get stats (port %u): %d",
>> +            __func__, port_id, ret);
>> +        return;
>> +    }
>>       printf("\n  %s NIC statistics for port %-2d %s\n",
>>              nic_stats_border, port_id, nic_stats_border);
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index fe2ce19f99..79bb23264b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -1982,6 +1982,7 @@ fwd_stats_display(void)
>>       struct rte_port *port;
>>       streamid_t sm_id;
>>       portid_t pt_id;
>> +    int ret;
>>       int i;
>>       memset(ports_stats, 0, sizeof(ports_stats));
>> @@ -2013,7 +2014,13 @@ fwd_stats_display(void)
>>           pt_id = fwd_ports_ids[i];
>>           port = &ports[pt_id];
>> -        rte_eth_stats_get(pt_id, &stats);
>> +        ret = rte_eth_stats_get(pt_id, &stats);
>> +        if (ret != 0) {
>> +            fprintf(stderr,
>> +                "%s: Error: failed to get stats (port %u): %d",
>> +                __func__, pt_id, ret);
>> +            continue;
>> +        }
>>           stats.ipackets -= port->stats.ipackets;
>>           stats.opackets -= port->stats.opackets;
>>           stats.ibytes -= port->stats.ibytes;
>> @@ -2108,11 +2115,16 @@ fwd_stats_reset(void)
>>   {
>>       streamid_t sm_id;
>>       portid_t pt_id;
>> +    int ret;
>>       int i;
>>       for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
>>           pt_id = fwd_ports_ids[i];
>> -        rte_eth_stats_get(pt_id, &ports[pt_id].stats);
>> +        ret = rte_eth_stats_get(pt_id, &ports[pt_id].stats);
>> +        if (ret != 0)
>> +            fprintf(stderr,
>> +                "%s: Error: failed to clear stats (port %u):%d",
>> +                __func__, pt_id, ret);
> Should we clear "ports[pt_id].stats" in this condition.
when done 'rte_eth_stats_get',
if success, 'ports[pt_id].stats' will be cleared in this function.
if failed, no need to clear 'ports[pt_id].stats' as the log said.

>>       }
>>       for (sm_id = 0; sm_id < cur_fwd_config.nb_fwd_streams; sm_id++) {
>>           struct fwd_stream *fs = fwd_streams[sm_id];
> As such LGTM
Thanks for your comment.
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> .

Re: [PATCH 2/2] app/testpmd: fix incorrect MTU verification

[Ferruh Yigit]

On 4/26/2022 2:38 AM, lihuisong (C) wrote:
> 
> 在 2022/4/26 0:25, Singh, Aman Deep 写道:
>>
>> On 4/6/2022 2:15 PM, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> The macro RTE_ETHER_MIN_LEN isn't the minimum value of MTU. But testpmd
>>> used it when execute 'port config mtu 0 xx' cmd. This patch fix it.
>>>
>>> Fixes: 1bb4a528c41f ("ethdev: fix max Rx packet length")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>

<...>

>>> @@ -1263,6 +1314,10 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>>>       if (port_id_is_invalid(port_id, ENABLED_WARN))
>>>           return;
>>>   +    diag = eth_dev_validate_mtu(port_id, mtu);
>>> +    if (diag != 0)
>>> +        return;
>>> +
>>>       if (port->need_reconfig == 0) {
>>>           diag = rte_eth_dev_set_mtu(port_id, mtu);
>>>           if (diag != 0) {
>> I just wanted to know if these added functions eth_dev_validate_mtu() 
>> &  eth_dev_get_overhead_len()
>> are copy of ethdev library API's in file "rte_ethdev.c", which get 
>> called by rte_eth_dev_set_mtu.
>> Is our intent, is to call these twice ?
> If port->need_reconfig is 1, rte_eth_dev_set_mtu doesn't be called, and
> MTU value is saved to port->dev_conf.rxmode.mtu. This dev_conf.rxmode.mtu
> will be set to driver in dev_configure(). This check is performed to
> prevent dev_configure() failure. That's what I think.
>> .

Testpmd stores the MTU value and process it during start_port(), which 
may postpone any possible error in rte_eth_dev_configure(), so I think
it is OK to duplicate the check in app level.

Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>

Re: [PATCH 0/2] bugfix for testpmd

[Ferruh Yigit]

On 4/25/2022 7:52 AM, Min Hu (Connor) wrote:
> Hi, Ferruh,
>      what do you think of this patch?
> 
> 在 2022/4/6 16:45, Min Hu (Connor) 写道:
>> This patch set contains two bugfixes for testpmd.
>>
>> Huisong Li (1):
>>    app/testpmd: fix incorrect MTU verification
>>
>> Min Hu (Connor) (1):
>>    app/testpmd: fix stats get when display fwd stats
>>

Series applied to dpdk-next-net/main, thanks.