[dpdk-dev] [PATCH 0/4] minor fixes for testpmd

[dpdk-dev] [PATCH 0/4] minor fixes for testpmd

[Wei Hu (Xavier)]

This series are minor fixes for testpmd application.

Chengchang Tang (3):
  app/testpmd: fix missing verification of port id
  app/testpmd: fix VLAN offload configuration when config fail
  app/testpmd: fix packet header in txonly mode

Huisong Li (1):
  app/testpmd: fix displaying Rx Tx queues information

 app/test-pmd/cmdline.c |  9 +++++
 app/test-pmd/config.c  | 80 +++++++++++++++++++++++++++++++-----------
 app/test-pmd/txonly.c  | 32 +++++++++++++++++
 3 files changed, 100 insertions(+), 21 deletions(-)

-- 
2.27.0

[dpdk-dev] [PATCH 1/4] app/testpmd: fix missing verification of port id

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

To set Tx vlan offloads, it is required to stop port firstly. But before
checking whether the port is stopped, the port id entered by the user
is not checked for validity. When the port id is illegal, it would lead
to a segmentation fault since it attempts to access a member of
non-existent port.

This patch adds verification of port id in tx vlan offloads.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/cmdline.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 0a6ed85f3..8377f8401 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4268,6 +4268,9 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4322,6 +4325,9 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4435,6 +4441,9 @@ cmd_tx_vlan_reset_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_reset_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
-- 
2.27.0

[dpdk-dev] [PATCH 2/4] app/testpmd: fix VLAN offload configuration when config fail

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

When failing to configure VLAN offloads after the port was started, there
is no need to update the port configuration. Currently, when user
configure an unsupported VLAN offloads and fails, and then restart the
port, it will fails since the configuration has been refreshed.

This patch makes the function return directly insead of refreshing the
configuration when execution fails.

Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 30bee3324..6e8e05ab1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3390,9 +3390,11 @@ vlan_extend_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_extend_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3417,9 +3419,11 @@ rx_vlan_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_strip_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3458,9 +3462,11 @@ rx_vlan_filter_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_filter_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3485,9 +3491,11 @@ rx_vlan_qinq_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("%s(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", __func__, port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
-- 
2.27.0

[dpdk-dev] [PATCH 3/4] app/testpmd: fix packet header in txonly mode

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

In txonly forward mode, the packet header is fixed by the initial
setting, including the packet length and checksum. So when the packets
varies, this may cause a packet header error. Currently, there are two
methods in txonly mode to randomly change the packets.
1. Set txsplit random and txpkts (x[,y]*), the number of segments
   each packets will be a random value between 1 and total number of
   segments determined by txpkts settings.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --rxd xxxx --txd xxxx
     b) set fwd txonly
     c) set txsplit rand
     d) set txpkts 2048,2048,2048,2048
     e) start
The nb_segs of the packets sent by testpmd will be 1~4. The real packet
length will be 2048, 4096, 6144 and 8192. But in fact the packet length
in ip header and udp header will be fixed by 8178 and 8158.

2. Set txonly-multi-flow. the ip address will be varied to generate
   multiple flow.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --txonly-multi-flow
     b) set fwd txonly
     c) start
The ip address of each pkts will change randomly, but since the header
is fixed, the checksum may be a error value.

Therefore, this patch adds a funciton to update the packet length and
check sum in the pkts header when the txsplit mode is setted to rand or
multi-flow is setted.

Fixes: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple flows")
Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/txonly.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 3bae367ee..ad3b0a765 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -156,6 +156,34 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
 }
 
+static inline void
+update_pkt_header(struct rte_mbuf *pkt, uint32_t total_pkt_len)
+{
+	struct rte_ipv4_hdr *ip_hdr;
+	struct rte_udp_hdr *udp_hdr;
+	uint16_t pkt_data_len;
+	uint16_t pkt_len;
+
+	pkt_data_len = (uint16_t) (total_pkt_len - (
+					sizeof(struct rte_ether_hdr) +
+					sizeof(struct rte_ipv4_hdr) +
+					sizeof(struct rte_udp_hdr)));
+	/* updata udp pkt length */
+	udp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_udp_hdr *,
+				sizeof(struct rte_ether_hdr) +
+				sizeof(struct rte_ipv4_hdr));
+	pkt_len = (uint16_t) (pkt_data_len + sizeof(struct rte_udp_hdr));
+	udp_hdr->dgram_len = RTE_CPU_TO_BE_16(pkt_len);
+
+	/* updata ip pkt length and csum */
+	ip_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_ipv4_hdr *,
+				sizeof(struct rte_ether_hdr));
+	ip_hdr->hdr_checksum = 0;
+	pkt_len = (uint16_t) (pkt_len + sizeof(struct rte_ipv4_hdr));
+	ip_hdr->total_length = RTE_CPU_TO_BE_16(pkt_len);
+	ip_hdr->hdr_checksum = rte_ipv4_cksum(ip_hdr);
+}
+
 static inline bool
 pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 		struct rte_ether_hdr *eth_hdr, const uint16_t vlan_tci,
@@ -223,6 +251,10 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 	copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt,
 			sizeof(struct rte_ether_hdr) +
 			sizeof(struct rte_ipv4_hdr));
+
+	if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND) || txonly_multi_flow)
+		update_pkt_header(pkt, pkt_len);
+
 	if (unlikely(timestamp_enable)) {
 		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
 		struct {
-- 
2.27.0

[dpdk-dev] [PATCH 4/4] app/testpmd: fix displaying Rx Tx queues information

[Wei Hu (Xavier)]

From: Huisong Li <lihuisong@huawei.com>

Currently, the information of Rx/Tx queues from PMD driver is not displayed
exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
and "ports[pid].tx_conf" maintained in testpmd application may be not the
value actually used by PMD driver. For instance, user does not set a field,
but PMD driver has to use the default value.

This patch fixes rxtx_config_display so that the information of Rx/Tx
queues can be really displayed for the PMD driver that implement
.rxq_info_get and .txq_info_get ops callback function.

Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 64 +++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6e8e05ab1..3ab24ebd2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2089,10 +2089,17 @@ rxtx_config_display(void)
 		struct rte_eth_txconf *tx_conf = &ports[pid].tx_conf[0];
 		uint16_t *nb_rx_desc = &ports[pid].nb_rx_desc[0];
 		uint16_t *nb_tx_desc = &ports[pid].nb_tx_desc[0];
-		uint16_t nb_rx_desc_tmp;
-		uint16_t nb_tx_desc_tmp;
 		struct rte_eth_rxq_info rx_qinfo;
 		struct rte_eth_txq_info tx_qinfo;
+		uint16_t rx_free_thresh_tmp;
+		uint16_t tx_free_thresh_tmp;
+		uint16_t tx_rs_thresh_tmp;
+		uint16_t nb_rx_desc_tmp;
+		uint16_t nb_tx_desc_tmp;
+		uint64_t offloads_tmp;
+		uint8_t pthresh_tmp;
+		uint8_t hthresh_tmp;
+		uint8_t wthresh_tmp;
 		int32_t rc;
 
 		/* per port config */
@@ -2106,41 +2113,64 @@ rxtx_config_display(void)
 		/* per rx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_rx_queue_info_get(pid, qid, &rx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_rx_desc_tmp = nb_rx_desc[qid];
-			else
+				rx_free_thresh_tmp =
+					rx_conf[qid].rx_free_thresh;
+				pthresh_tmp = rx_conf[qid].rx_thresh.pthresh;
+				hthresh_tmp = rx_conf[qid].rx_thresh.hthresh;
+				wthresh_tmp = rx_conf[qid].rx_thresh.wthresh;
+				offloads_tmp = rx_conf[qid].offloads;
+			} else {
 				nb_rx_desc_tmp = rx_qinfo.nb_desc;
+				rx_free_thresh_tmp =
+						rx_qinfo.conf.rx_free_thresh;
+				pthresh_tmp = rx_qinfo.conf.rx_thresh.pthresh;
+				hthresh_tmp = rx_qinfo.conf.rx_thresh.hthresh;
+				wthresh_tmp = rx_qinfo.conf.rx_thresh.wthresh;
+				offloads_tmp = rx_qinfo.conf.offloads;
+			}
 
 			printf("    RX queue: %d\n", qid);
 			printf("      RX desc=%d - RX free threshold=%d\n",
-				nb_rx_desc_tmp, rx_conf[qid].rx_free_thresh);
+				nb_rx_desc_tmp, rx_free_thresh_tmp);
 			printf("      RX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				rx_conf[qid].rx_thresh.pthresh,
-				rx_conf[qid].rx_thresh.hthresh,
-				rx_conf[qid].rx_thresh.wthresh);
-			printf("      RX Offloads=0x%"PRIx64"\n",
-				rx_conf[qid].offloads);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
+			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
 		}
 
 		/* per tx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_tx_queue_info_get(pid, qid, &tx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_tx_desc_tmp = nb_tx_desc[qid];
-			else
+				tx_free_thresh_tmp =
+					tx_conf[qid].tx_free_thresh;
+				pthresh_tmp = tx_conf[qid].tx_thresh.pthresh;
+				hthresh_tmp = tx_conf[qid].tx_thresh.hthresh;
+				wthresh_tmp = tx_conf[qid].tx_thresh.wthresh;
+				offloads_tmp = tx_conf[qid].offloads;
+				tx_rs_thresh_tmp = tx_conf[qid].tx_rs_thresh;
+			} else {
 				nb_tx_desc_tmp = tx_qinfo.nb_desc;
+				tx_free_thresh_tmp =
+						tx_qinfo.conf.tx_free_thresh;
+				pthresh_tmp = tx_qinfo.conf.tx_thresh.pthresh;
+				hthresh_tmp = tx_qinfo.conf.tx_thresh.hthresh;
+				wthresh_tmp = tx_qinfo.conf.tx_thresh.wthresh;
+				offloads_tmp = tx_qinfo.conf.offloads;
+				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
+			}
 
 			printf("    TX queue: %d\n", qid);
 			printf("      TX desc=%d - TX free threshold=%d\n",
-				nb_tx_desc_tmp, tx_conf[qid].tx_free_thresh);
+				nb_tx_desc_tmp, tx_free_thresh_tmp);
 			printf("      TX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				tx_conf[qid].tx_thresh.pthresh,
-				tx_conf[qid].tx_thresh.hthresh,
-				tx_conf[qid].tx_thresh.wthresh);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
 			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
-				tx_conf[qid].offloads, tx_conf->tx_rs_thresh);
+				offloads_tmp, tx_rs_thresh_tmp);
 		}
 	}
 }
-- 
2.27.0

[dpdk-dev] [PATCH v2 0/4] minor fixes for testpmd

[Wei Hu (Xavier)]

This series are minor fixes for testpmd application.

Chengchang Tang (3):
  app/testpmd: fix missing verification of port id
  app/testpmd: fix VLAN offload configuration when config fail
  app/testpmd: fix packet header in txonly mode

Huisong Li (1):
  app/testpmd: fix displaying Rx Tx queues information

 app/test-pmd/cmdline.c |  9 +++++
 app/test-pmd/config.c  | 80 +++++++++++++++++++++++++++++++-----------
 app/test-pmd/txonly.c  | 32 +++++++++++++++++
 3 files changed, 100 insertions(+), 21 deletions(-)

-- 
2.27.0

[dpdk-dev] [PATCH v2 1/4] app/testpmd: fix missing verification of port id

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

To set Tx vlan offloads, it is required to stop port firstly. But before
checking whether the port is stopped, the port id entered by the user
is not checked for validity. When the port id is illegal, it would lead
to a segmentation fault since it attempts to access a member of
non-existent port.

This patch adds verification of port id in tx vlan offloads.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/cmdline.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 0a6ed85f3..8377f8401 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4268,6 +4268,9 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4322,6 +4325,9 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4435,6 +4441,9 @@ cmd_tx_vlan_reset_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_reset_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
-- 
2.27.0

[dpdk-dev] [PATCH v2 2/4] app/testpmd: fix VLAN offload configuration when config fail

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

When failing to configure VLAN offloads after the port was started, there
is no need to update the port configuration. Currently, when user
configure an unsupported VLAN offloads and fails, and then restart the
port, it will fails since the configuration has been refreshed.

This patch makes the function return directly insead of refreshing the
configuration when execution fails.

Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 30bee3324..6e8e05ab1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3390,9 +3390,11 @@ vlan_extend_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_extend_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3417,9 +3419,11 @@ rx_vlan_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_strip_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3458,9 +3462,11 @@ rx_vlan_filter_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_filter_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3485,9 +3491,11 @@ rx_vlan_qinq_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("%s(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", __func__, port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
-- 
2.27.0

[dpdk-dev] [PATCH v2 3/4] app/testpmd: fix packet header in txonly mode

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

In txonly forward mode, the packet header is fixed by the initial
setting, including the packet length and checksum. So when the packets
varies, this may cause a packet header error. Currently, there are two
methods in txonly mode to randomly change the packets.
1. Set txsplit random and txpkts (x[,y]*), the number of segments
   each packets will be a random value between 1 and total number of
   segments determined by txpkts settings.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --rxd xxxx --txd xxxx
     b) set fwd txonly
     c) set txsplit rand
     d) set txpkts 2048,2048,2048,2048
     e) start
The nb_segs of the packets sent by testpmd will be 1~4. The real packet
length will be 2048, 4096, 6144 and 8192. But in fact the packet length
in ip header and udp header will be fixed by 8178 and 8158.

2. Set txonly-multi-flow. the ip address will be varied to generate
   multiple flow.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --txonly-multi-flow
     b) set fwd txonly
     c) start
The ip address of each pkts will change randomly, but since the header
is fixed, the checksum may be a error value.

Therefore, this patch adds a function to update the packet length and
check sum in the pkts header when the txsplit mode is set to rand or
multi-flow is set.

Fixes: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple flows")
Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
v1 -> v2: fix TYPO_SPELLING warning in the commit log.
---
 app/test-pmd/txonly.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 3bae367ee..ad3b0a765 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -156,6 +156,34 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
 }
 
+static inline void
+update_pkt_header(struct rte_mbuf *pkt, uint32_t total_pkt_len)
+{
+	struct rte_ipv4_hdr *ip_hdr;
+	struct rte_udp_hdr *udp_hdr;
+	uint16_t pkt_data_len;
+	uint16_t pkt_len;
+
+	pkt_data_len = (uint16_t) (total_pkt_len - (
+					sizeof(struct rte_ether_hdr) +
+					sizeof(struct rte_ipv4_hdr) +
+					sizeof(struct rte_udp_hdr)));
+	/* updata udp pkt length */
+	udp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_udp_hdr *,
+				sizeof(struct rte_ether_hdr) +
+				sizeof(struct rte_ipv4_hdr));
+	pkt_len = (uint16_t) (pkt_data_len + sizeof(struct rte_udp_hdr));
+	udp_hdr->dgram_len = RTE_CPU_TO_BE_16(pkt_len);
+
+	/* updata ip pkt length and csum */
+	ip_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_ipv4_hdr *,
+				sizeof(struct rte_ether_hdr));
+	ip_hdr->hdr_checksum = 0;
+	pkt_len = (uint16_t) (pkt_len + sizeof(struct rte_ipv4_hdr));
+	ip_hdr->total_length = RTE_CPU_TO_BE_16(pkt_len);
+	ip_hdr->hdr_checksum = rte_ipv4_cksum(ip_hdr);
+}
+
 static inline bool
 pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 		struct rte_ether_hdr *eth_hdr, const uint16_t vlan_tci,
@@ -223,6 +251,10 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 	copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt,
 			sizeof(struct rte_ether_hdr) +
 			sizeof(struct rte_ipv4_hdr));
+
+	if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND) || txonly_multi_flow)
+		update_pkt_header(pkt, pkt_len);
+
 	if (unlikely(timestamp_enable)) {
 		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
 		struct {
-- 
2.27.0

[dpdk-dev] [PATCH v2 4/4] app/testpmd: fix displaying Rx Tx queues information

[Wei Hu (Xavier)]

From: Huisong Li <lihuisong@huawei.com>

Currently, the information of Rx/Tx queues from PMD driver is not displayed
exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
and "ports[pid].tx_conf" maintained in testpmd application may be not the
value actually used by PMD driver. For instance, user does not set a field,
but PMD driver has to use the default value.

This patch fixes rxtx_config_display so that the information of Rx/Tx
queues can be really displayed for the PMD driver that implement
.rxq_info_get and .txq_info_get ops callback function.

Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 64 +++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6e8e05ab1..3ab24ebd2 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2089,10 +2089,17 @@ rxtx_config_display(void)
 		struct rte_eth_txconf *tx_conf = &ports[pid].tx_conf[0];
 		uint16_t *nb_rx_desc = &ports[pid].nb_rx_desc[0];
 		uint16_t *nb_tx_desc = &ports[pid].nb_tx_desc[0];
-		uint16_t nb_rx_desc_tmp;
-		uint16_t nb_tx_desc_tmp;
 		struct rte_eth_rxq_info rx_qinfo;
 		struct rte_eth_txq_info tx_qinfo;
+		uint16_t rx_free_thresh_tmp;
+		uint16_t tx_free_thresh_tmp;
+		uint16_t tx_rs_thresh_tmp;
+		uint16_t nb_rx_desc_tmp;
+		uint16_t nb_tx_desc_tmp;
+		uint64_t offloads_tmp;
+		uint8_t pthresh_tmp;
+		uint8_t hthresh_tmp;
+		uint8_t wthresh_tmp;
 		int32_t rc;
 
 		/* per port config */
@@ -2106,41 +2113,64 @@ rxtx_config_display(void)
 		/* per rx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_rx_queue_info_get(pid, qid, &rx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_rx_desc_tmp = nb_rx_desc[qid];
-			else
+				rx_free_thresh_tmp =
+					rx_conf[qid].rx_free_thresh;
+				pthresh_tmp = rx_conf[qid].rx_thresh.pthresh;
+				hthresh_tmp = rx_conf[qid].rx_thresh.hthresh;
+				wthresh_tmp = rx_conf[qid].rx_thresh.wthresh;
+				offloads_tmp = rx_conf[qid].offloads;
+			} else {
 				nb_rx_desc_tmp = rx_qinfo.nb_desc;
+				rx_free_thresh_tmp =
+						rx_qinfo.conf.rx_free_thresh;
+				pthresh_tmp = rx_qinfo.conf.rx_thresh.pthresh;
+				hthresh_tmp = rx_qinfo.conf.rx_thresh.hthresh;
+				wthresh_tmp = rx_qinfo.conf.rx_thresh.wthresh;
+				offloads_tmp = rx_qinfo.conf.offloads;
+			}
 
 			printf("    RX queue: %d\n", qid);
 			printf("      RX desc=%d - RX free threshold=%d\n",
-				nb_rx_desc_tmp, rx_conf[qid].rx_free_thresh);
+				nb_rx_desc_tmp, rx_free_thresh_tmp);
 			printf("      RX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				rx_conf[qid].rx_thresh.pthresh,
-				rx_conf[qid].rx_thresh.hthresh,
-				rx_conf[qid].rx_thresh.wthresh);
-			printf("      RX Offloads=0x%"PRIx64"\n",
-				rx_conf[qid].offloads);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
+			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
 		}
 
 		/* per tx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_tx_queue_info_get(pid, qid, &tx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_tx_desc_tmp = nb_tx_desc[qid];
-			else
+				tx_free_thresh_tmp =
+					tx_conf[qid].tx_free_thresh;
+				pthresh_tmp = tx_conf[qid].tx_thresh.pthresh;
+				hthresh_tmp = tx_conf[qid].tx_thresh.hthresh;
+				wthresh_tmp = tx_conf[qid].tx_thresh.wthresh;
+				offloads_tmp = tx_conf[qid].offloads;
+				tx_rs_thresh_tmp = tx_conf[qid].tx_rs_thresh;
+			} else {
 				nb_tx_desc_tmp = tx_qinfo.nb_desc;
+				tx_free_thresh_tmp =
+						tx_qinfo.conf.tx_free_thresh;
+				pthresh_tmp = tx_qinfo.conf.tx_thresh.pthresh;
+				hthresh_tmp = tx_qinfo.conf.tx_thresh.hthresh;
+				wthresh_tmp = tx_qinfo.conf.tx_thresh.wthresh;
+				offloads_tmp = tx_qinfo.conf.offloads;
+				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
+			}
 
 			printf("    TX queue: %d\n", qid);
 			printf("      TX desc=%d - TX free threshold=%d\n",
-				nb_tx_desc_tmp, tx_conf[qid].tx_free_thresh);
+				nb_tx_desc_tmp, tx_free_thresh_tmp);
 			printf("      TX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				tx_conf[qid].tx_thresh.pthresh,
-				tx_conf[qid].tx_thresh.hthresh,
-				tx_conf[qid].tx_thresh.wthresh);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
 			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
-				tx_conf[qid].offloads, tx_conf->tx_rs_thresh);
+				offloads_tmp, tx_rs_thresh_tmp);
 		}
 	}
 }
-- 
2.27.0

Re: [dpdk-dev] [PATCH v2 0/4] minor fixes for testpmd

[Wei Hu (Xavier)]

Hi, all

   Are there any comment?

Thanks

Xavier

On 2020/8/20 9:42, Wei Hu (Xavier) wrote:
> This series are minor fixes for testpmd application.
>
> Chengchang Tang (3):
>    app/testpmd: fix missing verification of port id
>    app/testpmd: fix VLAN offload configuration when config fail
>    app/testpmd: fix packet header in txonly mode
>
> Huisong Li (1):
>    app/testpmd: fix displaying Rx Tx queues information
>
>   app/test-pmd/cmdline.c |  9 +++++
>   app/test-pmd/config.c  | 80 +++++++++++++++++++++++++++++++-----------
>   app/test-pmd/txonly.c  | 32 +++++++++++++++++
>   3 files changed, 100 insertions(+), 21 deletions(-)
>

Re: [dpdk-dev] [PATCH v2 1/4] app/testpmd: fix missing verification of port id

[Ferruh Yigit]

On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> To set Tx vlan offloads, it is required to stop port firstly. But before
> checking whether the port is stopped, the port id entered by the user
> is not checked for validity. When the port id is illegal, it would lead
> to a segmentation fault since it attempts to access a member of
> non-existent port.
> 
> This patch adds verification of port id in tx vlan offloads.
> 
> Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0a6ed85f3..8377f8401 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -4268,6 +4268,9 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
>  {
>  	struct cmd_tx_vlan_set_result *res = parsed_result;
>  
> +	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> +		return;
> +
>  	if (!port_is_stopped(res->port_id)) {
>  		printf("Please stop port %d first\n", res->port_id);
>  		return;

These functions are wrappers to some testpmd functions, and those
functions already have invalid port check, like 'tx_vlan_set()'

Agree that check should be in these functions, but to prevent the
duplicated checks, what do you think to remove checks from internal
functions? ('tx_vlan_set', 'tx_qinq_set' & 'tx_vlan_reset').

Re: [dpdk-dev] [PATCH v2 2/4] app/testpmd: fix VLAN offload configuration when config fail

[Ferruh Yigit]

On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> When failing to configure VLAN offloads after the port was started, there
> is no need to update the port configuration. Currently, when user
> configure an unsupported VLAN offloads and fails, and then restart the
> port, it will fails since the configuration has been refreshed.
> 
> This patch makes the function return directly insead of refreshing the
> configuration when execution fails.
> 
> Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v2 3/4] app/testpmd: fix packet header in txonly mode

[Ferruh Yigit]

On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> In txonly forward mode, the packet header is fixed by the initial
> setting, including the packet length and checksum. So when the packets
> varies, this may cause a packet header error. Currently, there are two
> methods in txonly mode to randomly change the packets.
> 1. Set txsplit random and txpkts (x[,y]*), the number of segments
>    each packets will be a random value between 1 and total number of
>    segments determined by txpkts settings.
>    The step as follows:
>      a) ./testpmd -w xxx -l xx -n 4 -- -i --rxd xxxx --txd xxxx
>      b) set fwd txonly
>      c) set txsplit rand
>      d) set txpkts 2048,2048,2048,2048
>      e) start
> The nb_segs of the packets sent by testpmd will be 1~4. The real packet
> length will be 2048, 4096, 6144 and 8192. But in fact the packet length
> in ip header and udp header will be fixed by 8178 and 8158.

Although I confirm the patch fixes the ip & udp header packet size for
"txsplit=rand" config,
I am always getting actual packet size wrong, independent from
'txsplit', and it is always first segment size. Am I doing something wrong?


And not related to this patch but why setting 'txpkts' requires "--rxd
xxxx --txd xxxx" options explicitly set? If you are already there can
you also remove this restriction?

> 
> 2. Set txonly-multi-flow. the ip address will be varied to generate
>    multiple flow.
>    The step as follows:
>      a) ./testpmd -w xxx -l xx -n 4 -- -i --txonly-multi-flow
>      b) set fwd txonly
>      c) start
> The ip address of each pkts will change randomly, but since the header
> is fixed, the checksum may be a error value.

+1, I confirm fixing the checksum error.

> 
> Therefore, this patch adds a function to update the packet length and
> check sum in the pkts header when the txsplit mode is set to rand or
> multi-flow is set.
> 
> Fixes: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple flows")
> Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

<...>

Re: [dpdk-dev] [PATCH v2 4/4] app/testpmd: fix displaying Rx Tx queues information

[Ferruh Yigit]

On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, the information of Rx/Tx queues from PMD driver is not displayed
> exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
> and "ports[pid].tx_conf" maintained in testpmd application may be not the
> value actually used by PMD driver. For instance, user does not set a field,
> but PMD driver has to use the default value.

Overall the question is why testpmd maintains the config values itself?
If this is testpmd implementation problem that is no big deal, but if
our APIs are forcing applications to maintain local copies that is
something to fix I think, which may lead the differences in application
copy and more troubles as you are fixing here.

> 
> This patch fixes rxtx_config_display so that the information of Rx/Tx
> queues can be really displayed for the PMD driver that implement
> .rxq_info_get and .txq_info_get ops callback function.
> 
> Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
> Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v2 4/4] app/testpmd: fix displaying Rx Tx queues information

[Wei Hu (Xavier)]

Hi, Ferruh Yigit

On 2020/9/15 0:31, Ferruh Yigit wrote:
> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, the information of Rx/Tx queues from PMD driver is not displayed
>> exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
>> and "ports[pid].tx_conf" maintained in testpmd application may be not the
>> value actually used by PMD driver. For instance, user does not set a field,
>> but PMD driver has to use the default value.
> Overall the question is why testpmd maintains the config values itself?
> If this is testpmd implementation problem that is no big deal, but if
> our APIs are forcing applications to maintain local copies that is
> something to fix I think, which may lead the differences in application
> copy and more troubles as you are fixing here.

I think it is a minor problem about displaying some information about 
testpmd.

And it has no impact on the usage of application.


Thanks,

Xavier

>> This patch fixes rxtx_config_display so that the information of Rx/Tx
>> queues can be really displayed for the PMD driver that implement
>> .rxq_info_get and .txq_info_get ops callback function.
>>
>> Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
>> Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>

Re: [dpdk-dev] [PATCH v2 4/4] app/testpmd: fix displaying Rx Tx queues information

[Ferruh Yigit]

On 9/16/2020 10:23 AM, Wei Hu (Xavier) wrote:
> Hi, Ferruh Yigit
> 
> On 2020/9/15 0:31, Ferruh Yigit wrote:
>> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>>> From: Huisong Li<lihuisong@huawei.com>
>>>
>>> Currently, the information of Rx/Tx queues from PMD driver is not displayed
>>> exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
>>> and "ports[pid].tx_conf" maintained in testpmd application may be not the
>>> value actually used by PMD driver. For instance, user does not set a field,
>>> but PMD driver has to use the default value.
>> Overall the question is why testpmd maintains the config values itself?
>> If this is testpmd implementation problem that is no big deal, but if
>> our APIs are forcing applications to maintain local copies that is
>> something to fix I think, which may lead the differences in application
>> copy and more troubles as you are fixing here.
> 
> I think it is a minor problem about displaying some information about 
> testpmd.
> 
> And it has no impact on the usage of application >

Agree this specific issue is minor problem, I already give the review tag.

And if the issue only concerns testpmd, still no big issue.

But I was questioning if this is sign of a design problem in DPDK APIs, 
which matters.

A simple question, why testpmd maintains the local version of the 
configuration?

And follow up can be,
Will it cause any functional problem if application always request 
configs from DPDK when needed instead of maintaining local copies? Are 
we missing APIs to do this?

> 
> Thanks,
> 
> Xavier
> 
>>> This patch fixes rxtx_config_display so that the information of Rx/Tx
>>> queues can be really displayed for the PMD driver that implement
>>> .rxq_info_get and .txq_info_get ops callback function.
>>>
>>> Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
>>> Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
>>> Cc:stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li<lihuisong@huawei.com>
>>> Signed-off-by: Wei Hu (Xavier)<xavier.huwei@huawei.com>
>> Reviewed-by: Ferruh Yigit<ferruh.yigit@intel.com>
>>

Re: [dpdk-dev] [PATCH v2 3/4] app/testpmd: fix packet header in txonly mode

[Chengchang Tang]

On 2020/9/15 0:23, Ferruh Yigit wrote:
> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> In txonly forward mode, the packet header is fixed by the initial
>> setting, including the packet length and checksum. So when the packets
>> varies, this may cause a packet header error. Currently, there are two
>> methods in txonly mode to randomly change the packets.
>> 1. Set txsplit random and txpkts (x[,y]*), the number of segments
>>    each packets will be a random value between 1 and total number of
>>    segments determined by txpkts settings.
>>    The step as follows:
>>      a) ./testpmd -w xxx -l xx -n 4 -- -i --rxd xxxx --txd xxxx
>>      b) set fwd txonly
>>      c) set txsplit rand
>>      d) set txpkts 2048,2048,2048,2048
>>      e) start
>> The nb_segs of the packets sent by testpmd will be 1~4. The real packet
>> length will be 2048, 4096, 6144 and 8192. But in fact the packet length
>> in ip header and udp header will be fixed by 8178 and 8158.
> 
> Although I confirm the patch fixes the ip & udp header packet size for
> "txsplit=rand" config,
> I am always getting actual packet size wrong, independent from
> 'txsplit', and it is always first segment size. Am I doing something wrong?

Yes, I miss it. If the txsplit is not set and the txpkts is set, the txonly
fwd engine will only send single segment packets, and there will be a payload
error for the data_len will be refreshed by the sum of the segment size.

May be the data_len should be the first segment size if the txsplit is not set?
I will fix it in the next version.
> 
> And not related to this patch but why setting 'txpkts' requires "--rxd
> xxxx --txd xxxx" options explicitly set? If you are already there can
> you also remove this restriction?

OK, I will fix it in next version.
> <...> 
> .
> 

Re: [dpdk-dev] [PATCH v2 3/4] app/testpmd: fix packet header in txonly mode

[Ferruh Yigit]

On 9/17/2020 8:10 AM, Chengchang Tang wrote:
> 
> 
> On 2020/9/15 0:23, Ferruh Yigit wrote:
>> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>
>>> In txonly forward mode, the packet header is fixed by the initial
>>> setting, including the packet length and checksum. So when the packets
>>> varies, this may cause a packet header error. Currently, there are two
>>> methods in txonly mode to randomly change the packets.
>>> 1. Set txsplit random and txpkts (x[,y]*), the number of segments
>>>     each packets will be a random value between 1 and total number of
>>>     segments determined by txpkts settings.
>>>     The step as follows:
>>>       a) ./testpmd -w xxx -l xx -n 4 -- -i --rxd xxxx --txd xxxx
>>>       b) set fwd txonly
>>>       c) set txsplit rand
>>>       d) set txpkts 2048,2048,2048,2048
>>>       e) start
>>> The nb_segs of the packets sent by testpmd will be 1~4. The real packet
>>> length will be 2048, 4096, 6144 and 8192. But in fact the packet length
>>> in ip header and udp header will be fixed by 8178 and 8158.
>>
>> Although I confirm the patch fixes the ip & udp header packet size for
>> "txsplit=rand" config,
>> I am always getting actual packet size wrong, independent from
>> 'txsplit', and it is always first segment size. Am I doing something wrong?
> 
> Yes, I miss it. If the txsplit is not set and the txpkts is set, the txonly
> fwd engine will only send single segment packets, and there will be a payload
> error for the data_len will be refreshed by the sum of the segment size.
> 

I am getting the size error when 'txsplit' is 'on' or 'rand'. In that 
case packet header are correct (after your patch) but wireshark 
complains the size of the packet on wire is different than the values in 
headers. And the size of the actual packet is reported by wireshark as 
the size of the first segment.

> May be the data_len should be the first segment size if the txsplit is not set?
> I will fix it in the next version.
>>
>> And not related to this patch but why setting 'txpkts' requires "--rxd
>> xxxx --txd xxxx" options explicitly set? If you are already there can
>> you also remove this restriction?
> 
> OK, I will fix it in next version.

Thanks, appreciated.

Re: [dpdk-dev] [PATCH v2 3/4] app/testpmd: fix packet header in txonly mode

[Chengchang Tang]

On 2020/9/17 19:16, Ferruh Yigit wrote:
> On 9/17/2020 8:10 AM, Chengchang Tang wrote:
>>
>>
>> On 2020/9/15 0:23, Ferruh Yigit wrote:
>>> On 8/20/2020 2:42 AM, Wei Hu (Xavier) wrote:
>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>
>>>> In txonly forward mode, the packet header is fixed by the initial
>>>> setting, including the packet length and checksum. So when the packets
>>>> varies, this may cause a packet header error. Currently, there are two
>>>> methods in txonly mode to randomly change the packets.
>>>> 1. Set txsplit random and txpkts (x[,y]*), the number of segments
>>>>     each packets will be a random value between 1 and total number of
>>>>     segments determined by txpkts settings.
>>>>     The step as follows:
>>>>       a) ./testpmd -w xxx -l xx -n 4 -- -i --rxd xxxx --txd xxxx
>>>>       b) set fwd txonly
>>>>       c) set txsplit rand
>>>>       d) set txpkts 2048,2048,2048,2048
>>>>       e) start
>>>> The nb_segs of the packets sent by testpmd will be 1~4. The real packet
>>>> length will be 2048, 4096, 6144 and 8192. But in fact the packet length
>>>> in ip header and udp header will be fixed by 8178 and 8158.
>>>
>>> Although I confirm the patch fixes the ip & udp header packet size for
>>> "txsplit=rand" config,
>>> I am always getting actual packet size wrong, independent from
>>> 'txsplit', and it is always first segment size. Am I doing something wrong?
>>
>> Yes, I miss it. If the txsplit is not set and the txpkts is set, the txonly
>> fwd engine will only send single segment packets, and there will be a payload
>> error for the data_len will be refreshed by the sum of the segment size.
>>
> 
> I am getting the size error when 'txsplit' is 'on' or 'rand'. In that case packet header are correct (after your patch) but wireshark complains the size of the packet on wire is different than the values in headers. And the size of the actual packet is reported by wireshark as the size of the first segment.

Maybe it should be configured with multi_segs offload. If the 'txsplit' is 'on' or 'rand',
the txonly engine will send a multi segments packets. I tested it on hns3 PMD, which
support a multi_segs by default configuration. So, I miss to add these information to
the commit log. For some PMDs, the multi_segs is not support as default.

>> May be the data_len should be the first segment size if the txsplit is not set?
>> I will fix it in the next version.
>>>
<...>
> 
> 
> .
> 

[dpdk-dev] [PATCH v3 0/6] minor fixes for testpmd

[Wei Hu (Xavier)]

This series are minor fixes for testpmd application.

Chengchang Tang (5):
  app/testpmd: fix missing verification of port id
  app/testpmd: fix VLAN offload configuration when config fail
  app/testpmd: remove restriction on txpkts set
  app/testpmd: fix packet header in txonly mode
  app/testpmd: fix valid desc id check

Huisong Li (1):
  app/testpmd: fix displaying Rx Tx queues information

 app/test-pmd/cmdline.c |   9 +++
 app/test-pmd/config.c  | 181 ++++++++++++++++++++++++++++++++++++++-----------
 app/test-pmd/txonly.c  |  32 +++++++++
 3 files changed, 181 insertions(+), 41 deletions(-)

-- 
2.9.5

[dpdk-dev] [PATCH v3 1/6] app/testpmd: fix missing verification of port id

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

To set Tx vlan offloads, it is required to stop port firstly. But before
checking whether the port is stopped, the port id entered by the user
is not checked for validity. When the port id is illegal, it would lead
to a segmentation fault since it attempts to access a member of
non-existent port.

This patch adds verification of port id in tx vlan offloads and remove
duplicated check.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
v1 -> v2: remove the duplicated check.
---
 app/test-pmd/cmdline.c | 9 +++++++++
 app/test-pmd/config.c  | 6 ------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 5f93409..43b7c2f 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4291,6 +4291,9 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4345,6 +4348,9 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4458,6 +4464,9 @@ cmd_tx_vlan_reset_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_reset_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b6eb2a5..2bdec25 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3545,8 +3545,6 @@ tx_vlan_set(portid_t port_id, uint16_t vlan_id)
 	struct rte_eth_dev_info dev_info;
 	int ret;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
 	if (vlan_id_is_invalid(vlan_id))
 		return;
 
@@ -3577,8 +3575,6 @@ tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer)
 	struct rte_eth_dev_info dev_info;
 	int ret;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
 	if (vlan_id_is_invalid(vlan_id))
 		return;
 	if (vlan_id_is_invalid(vlan_id_outer))
@@ -3604,8 +3600,6 @@ tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer)
 void
 tx_vlan_reset(portid_t port_id)
 {
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
 	ports[port_id].dev_conf.txmode.offloads &=
 				~(DEV_TX_OFFLOAD_VLAN_INSERT |
 				  DEV_TX_OFFLOAD_QINQ_INSERT);
-- 
2.9.5

[dpdk-dev] [PATCH v3 2/6] app/testpmd: fix VLAN offload configuration when config fail

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

When failing to configure VLAN offloads after the port was started, there
is no need to update the port configuration. Currently, when user
configure an unsupported VLAN offloads and fails, and then restart the
port, it will fails since the configuration has been refreshed.

This patch makes the function return directly insead of refreshing the
configuration when execution fails.

Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/config.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 2bdec25..4e33208 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3390,9 +3390,11 @@ vlan_extend_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_extend_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3417,9 +3419,11 @@ rx_vlan_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_strip_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3458,9 +3462,11 @@ rx_vlan_filter_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_filter_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3485,9 +3491,11 @@ rx_vlan_qinq_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("%s(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", __func__, port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
-- 
2.9.5

[dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

Currently, if nb_txd is not set, the txpkts is not allowed to be set
because the nb_txd is used to avoid the numer of segments exceed the Tx
ring size and the default value of nb_txd is 0. And there is a bug that
nb_txd is the global configuration for Tx ring size and the ring size
could be changed by some command per queue. So these valid check is
unreliable and introduced unnecessary constraints.

This patch adds a valid check function to use the real Tx ring size to
check the validity of txpkts.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 4e33208..882de2d 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
 	printf("Split packet: %s\n", split);
 }
 
+static bool
+nb_segs_is_invalid(unsigned int nb_segs)
+{
+	uint16_t port_id;
+
+	RTE_ETH_FOREACH_DEV(port_id) {
+		struct rte_port *port = &ports[port_id];
+		uint16_t ring_size;
+		uint16_t queue_id;
+
+		/*
+		 * When configure the txq by rte_eth_tx_queue_setup with
+		 * nb_tx_desc being 0, it will use a default value provided by
+		 * PMDs to setup this txq. If the default value is 0, it will
+		 * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
+		 */
+		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
+			if (port->nb_tx_desc[queue_id])
+				ring_size = port->nb_tx_desc[queue_id];
+			else if (port->dev_info.default_txportconf.ring_size)
+				ring_size =
+				    port->dev_info.default_txportconf.ring_size;
+			else
+				ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+
+			if (ring_size < nb_segs) {
+				printf("nb segments per TX packets=%u >= TX "
+				       "queue(%u) ring_size=%u - ignored\n",
+					nb_segs, queue_id, ring_size);
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
 void
 set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
 {
 	uint16_t tx_pkt_len;
 	unsigned i;
 
-	if (nb_segs >= (unsigned) nb_txd) {
-		printf("nb segments per TX packets=%u >= nb_txd=%u - ignored\n",
-		       nb_segs, (unsigned int) nb_txd);
+	if (nb_segs_is_invalid(nb_segs))
 		return;
-	}
 
 	/*
 	 * Check that each segment length is greater or equal than
-- 
2.9.5

[dpdk-dev] [PATCH v3 4/6] app/testpmd: fix packet header in txonly mode

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

In txonly forward mode, the packet header is fixed by the initial
setting, including the packet length and checksum. So when the packets
varies, this may cause a packet header error. Currently, there are two
methods in txonly mode to randomly change the packets.
1. Set txsplit random and txpkts (x[,y]*), the number of segments
   each packets will be a random value between 1 and total number of
   segments determined by txpkts settings.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --disable-device-start
     b) port config 0 tx_offload multi_segs on
     c) set fwd txonly
     d) set txsplit rand
     e) set txpkts 2048,2048,2048,2048
     f) start
The nb_segs of the packets sent by testpmd will be 1~4. The real packet
length will be 2048, 4096, 6144 and 8192. But in fact the packet length
in ip header and udp header will be fixed by 8178 and 8158.

2. Set txonly-multi-flow. the ip address will be varied to generate
   multiple flow.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --txonly-multi-flow
     b) set fwd txonly
     c) start
The ip address of each pkts will change randomly, but since the header
is fixed, the checksum may be a error value.

Therefore, this patch adds a function to update the packet length and
check sum in the pkts header when the txsplit mode is set to rand or
multi-flow is set.

Fixes: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple flows")
Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/txonly.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 45def72..d55ee7c 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -156,6 +156,34 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
 }
 
+static inline void
+update_pkt_header(struct rte_mbuf *pkt, uint32_t total_pkt_len)
+{
+	struct rte_ipv4_hdr *ip_hdr;
+	struct rte_udp_hdr *udp_hdr;
+	uint16_t pkt_data_len;
+	uint16_t pkt_len;
+
+	pkt_data_len = (uint16_t) (total_pkt_len - (
+					sizeof(struct rte_ether_hdr) +
+					sizeof(struct rte_ipv4_hdr) +
+					sizeof(struct rte_udp_hdr)));
+	/* updata udp pkt length */
+	udp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_udp_hdr *,
+				sizeof(struct rte_ether_hdr) +
+				sizeof(struct rte_ipv4_hdr));
+	pkt_len = (uint16_t) (pkt_data_len + sizeof(struct rte_udp_hdr));
+	udp_hdr->dgram_len = RTE_CPU_TO_BE_16(pkt_len);
+
+	/* updata ip pkt length and csum */
+	ip_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_ipv4_hdr *,
+				sizeof(struct rte_ether_hdr));
+	ip_hdr->hdr_checksum = 0;
+	pkt_len = (uint16_t) (pkt_len + sizeof(struct rte_ipv4_hdr));
+	ip_hdr->total_length = RTE_CPU_TO_BE_16(pkt_len);
+	ip_hdr->hdr_checksum = rte_ipv4_cksum(ip_hdr);
+}
+
 static inline bool
 pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 		struct rte_ether_hdr *eth_hdr, const uint16_t vlan_tci,
@@ -223,6 +251,10 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 	copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt,
 			sizeof(struct rte_ether_hdr) +
 			sizeof(struct rte_ipv4_hdr));
+
+	if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND) || txonly_multi_flow)
+		update_pkt_header(pkt, pkt_len);
+
 	if (unlikely(timestamp_enable)) {
 		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
 		struct {
-- 
2.9.5

[dpdk-dev] [PATCH v3 5/6] app/testpmd: fix valid desc id check

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

The number of desc is a per queue configuration. But in the check function,
nb_txd & nb_rxd are used to check whether the desc_id is valid. nb_txd &
nb_rxd are the global configuration of number of desc. If the queue
configuration is changed by cmdline liks: "port config xx txq xx ring_size
xxx", the real value will be changed.

This patch use the real value to check whether the desc_id is valid. And if
these are not configured by user. It will use the default value to check
it, since the rte_eth_rx_queue_setup & rte_eth_tx_queue_setup will use a
default value to confiure the queue if nb_rx_desc or nb_tx_desc is zero.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/config.c | 53 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 882de2d..b7851c7 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1891,22 +1891,55 @@ tx_queue_id_is_invalid(queueid_t txq_id)
 }
 
 static int
-rx_desc_id_is_invalid(uint16_t rxdesc_id)
+rx_desc_id_is_invalid(portid_t port_id, queueid_t rxq_id, uint16_t rxdesc_id)
 {
-	if (rxdesc_id < nb_rxd)
+	struct rte_port *port = &ports[port_id];
+	uint16_t ring_size;
+
+	/*
+	 * When configure the rxq by rte_eth_rx_queue_setup with nb_rx_desc
+	 * being 0, it will use a default value provided by PMDs to setup this
+	 * rxq. If the default value is 0, it will use the
+	 * RTE_ETH_DEV_FALLBACK_RX_RINGSIZE to setup this rxq.
+	 */
+	if (port->nb_rx_desc[rxq_id])
+		ring_size = port->nb_rx_desc[rxq_id];
+	else if (port->dev_info.default_rxportconf.ring_size)
+		ring_size = port->dev_info.default_rxportconf.ring_size;
+	else
+		ring_size =  RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+
+	if (rxdesc_id < ring_size)
 		return 0;
-	printf("Invalid RX descriptor %d (must be < nb_rxd=%d)\n",
-	       rxdesc_id, nb_rxd);
+	printf("Invalid RX descriptor %d (must be < ring_size=%d)\n",
+	       rxdesc_id, ring_size);
 	return 1;
 }
 
 static int
-tx_desc_id_is_invalid(uint16_t txdesc_id)
+tx_desc_id_is_invalid(portid_t port_id, queueid_t txq_id, uint16_t txdesc_id)
 {
-	if (txdesc_id < nb_txd)
+	struct rte_port *port = &ports[port_id];
+	uint16_t ring_size;
+
+	/*
+	 * When configure the txq by rte_eth_tx_queue_setup with nb_tx_desc
+	 * being 0, it will use a default value provided by PMDs to setup this
+	 * txq. If the default value is 0, it will use the
+	 * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
+	 */
+	if (port->nb_tx_desc[txq_id])
+		ring_size = port->nb_tx_desc[txq_id];
+	else if (port->dev_info.default_txportconf.ring_size)
+		ring_size = port->dev_info.default_txportconf.ring_size;
+	else
+		ring_size =  RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+
+	if (txdesc_id < ring_size)
 		return 0;
-	printf("Invalid TX descriptor %d (must be < nb_txd=%d)\n",
-	       txdesc_id, nb_txd);
+
+	printf("Invalid TX descriptor %d (must be < ring_size=%d)\n",
+	       txdesc_id, ring_size);
 	return 1;
 }
 
@@ -2031,7 +2064,7 @@ rx_ring_desc_display(portid_t port_id, queueid_t rxq_id, uint16_t rxd_id)
 		return;
 	if (rx_queue_id_is_invalid(rxq_id))
 		return;
-	if (rx_desc_id_is_invalid(rxd_id))
+	if (rx_desc_id_is_invalid(port_id, rxq_id, rxd_id))
 		return;
 	rx_mz = ring_dma_zone_lookup("rx_ring", port_id, rxq_id);
 	if (rx_mz == NULL)
@@ -2048,7 +2081,7 @@ tx_ring_desc_display(portid_t port_id, queueid_t txq_id, uint16_t txd_id)
 		return;
 	if (tx_queue_id_is_invalid(txq_id))
 		return;
-	if (tx_desc_id_is_invalid(txd_id))
+	if (tx_desc_id_is_invalid(port_id, txq_id, txd_id))
 		return;
 	tx_mz = ring_dma_zone_lookup("tx_ring", port_id, txq_id);
 	if (tx_mz == NULL)
-- 
2.9.5

[dpdk-dev] [PATCH v3 6/6] app/testpmd: fix displaying Rx Tx queues information

[Wei Hu (Xavier)]

From: Huisong Li <lihuisong@huawei.com>

Currently, the information of Rx/Tx queues from PMD driver is not displayed
exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
and "ports[pid].tx_conf" maintained in testpmd application may be not the
value actually used by PMD driver. For instance, user does not set a field,
but PMD driver has to use the default value.

This patch fixes rxtx_config_display so that the information of Rx/Tx
queues can be really displayed for the PMD driver that implement
.rxq_info_get and .txq_info_get ops callback function.

Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/config.c | 64 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b7851c7..03cd671 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2122,10 +2122,17 @@ rxtx_config_display(void)
 		struct rte_eth_txconf *tx_conf = &ports[pid].tx_conf[0];
 		uint16_t *nb_rx_desc = &ports[pid].nb_rx_desc[0];
 		uint16_t *nb_tx_desc = &ports[pid].nb_tx_desc[0];
-		uint16_t nb_rx_desc_tmp;
-		uint16_t nb_tx_desc_tmp;
 		struct rte_eth_rxq_info rx_qinfo;
 		struct rte_eth_txq_info tx_qinfo;
+		uint16_t rx_free_thresh_tmp;
+		uint16_t tx_free_thresh_tmp;
+		uint16_t tx_rs_thresh_tmp;
+		uint16_t nb_rx_desc_tmp;
+		uint16_t nb_tx_desc_tmp;
+		uint64_t offloads_tmp;
+		uint8_t pthresh_tmp;
+		uint8_t hthresh_tmp;
+		uint8_t wthresh_tmp;
 		int32_t rc;
 
 		/* per port config */
@@ -2139,41 +2146,64 @@ rxtx_config_display(void)
 		/* per rx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_rx_queue_info_get(pid, qid, &rx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_rx_desc_tmp = nb_rx_desc[qid];
-			else
+				rx_free_thresh_tmp =
+					rx_conf[qid].rx_free_thresh;
+				pthresh_tmp = rx_conf[qid].rx_thresh.pthresh;
+				hthresh_tmp = rx_conf[qid].rx_thresh.hthresh;
+				wthresh_tmp = rx_conf[qid].rx_thresh.wthresh;
+				offloads_tmp = rx_conf[qid].offloads;
+			} else {
 				nb_rx_desc_tmp = rx_qinfo.nb_desc;
+				rx_free_thresh_tmp =
+						rx_qinfo.conf.rx_free_thresh;
+				pthresh_tmp = rx_qinfo.conf.rx_thresh.pthresh;
+				hthresh_tmp = rx_qinfo.conf.rx_thresh.hthresh;
+				wthresh_tmp = rx_qinfo.conf.rx_thresh.wthresh;
+				offloads_tmp = rx_qinfo.conf.offloads;
+			}
 
 			printf("    RX queue: %d\n", qid);
 			printf("      RX desc=%d - RX free threshold=%d\n",
-				nb_rx_desc_tmp, rx_conf[qid].rx_free_thresh);
+				nb_rx_desc_tmp, rx_free_thresh_tmp);
 			printf("      RX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				rx_conf[qid].rx_thresh.pthresh,
-				rx_conf[qid].rx_thresh.hthresh,
-				rx_conf[qid].rx_thresh.wthresh);
-			printf("      RX Offloads=0x%"PRIx64"\n",
-				rx_conf[qid].offloads);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
+			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
 		}
 
 		/* per tx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_tx_queue_info_get(pid, qid, &tx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_tx_desc_tmp = nb_tx_desc[qid];
-			else
+				tx_free_thresh_tmp =
+					tx_conf[qid].tx_free_thresh;
+				pthresh_tmp = tx_conf[qid].tx_thresh.pthresh;
+				hthresh_tmp = tx_conf[qid].tx_thresh.hthresh;
+				wthresh_tmp = tx_conf[qid].tx_thresh.wthresh;
+				offloads_tmp = tx_conf[qid].offloads;
+				tx_rs_thresh_tmp = tx_conf[qid].tx_rs_thresh;
+			} else {
 				nb_tx_desc_tmp = tx_qinfo.nb_desc;
+				tx_free_thresh_tmp =
+						tx_qinfo.conf.tx_free_thresh;
+				pthresh_tmp = tx_qinfo.conf.tx_thresh.pthresh;
+				hthresh_tmp = tx_qinfo.conf.tx_thresh.hthresh;
+				wthresh_tmp = tx_qinfo.conf.tx_thresh.wthresh;
+				offloads_tmp = tx_qinfo.conf.offloads;
+				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
+			}
 
 			printf("    TX queue: %d\n", qid);
 			printf("      TX desc=%d - TX free threshold=%d\n",
-				nb_tx_desc_tmp, tx_conf[qid].tx_free_thresh);
+				nb_tx_desc_tmp, tx_free_thresh_tmp);
 			printf("      TX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				tx_conf[qid].tx_thresh.pthresh,
-				tx_conf[qid].tx_thresh.hthresh,
-				tx_conf[qid].tx_thresh.wthresh);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
 			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
-				tx_conf[qid].offloads, tx_conf->tx_rs_thresh);
+				offloads_tmp, tx_rs_thresh_tmp);
 		}
 	}
 }
-- 
2.9.5

Re: [dpdk-dev] [PATCH v3 1/6] app/testpmd: fix missing verification of port id

[Ferruh Yigit]

On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> To set Tx vlan offloads, it is required to stop port firstly. But before
> checking whether the port is stopped, the port id entered by the user
> is not checked for validity. When the port id is illegal, it would lead
> to a segmentation fault since it attempts to access a member of
> non-existent port.
> 
> This patch adds verification of port id in tx vlan offloads and remove
> duplicated check.
> 
> Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Currently, if nb_txd is not set, the txpkts is not allowed to be set
> because the nb_txd is used to avoid the numer of segments exceed the Tx
> ring size and the default value of nb_txd is 0. And there is a bug that
> nb_txd is the global configuration for Tx ring size and the ring size
> could be changed by some command per queue. So these valid check is
> unreliable and introduced unnecessary constraints.
> 
> This patch adds a valid check function to use the real Tx ring size to
> check the validity of txpkts.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 4e33208..882de2d 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>   	printf("Split packet: %s\n", split);
>   }
>   
> +static bool
> +nb_segs_is_invalid(unsigned int nb_segs)
> +{
> +	uint16_t port_id;
> +
> +	RTE_ETH_FOREACH_DEV(port_id) {
> +		struct rte_port *port = &ports[port_id];
> +		uint16_t ring_size;
> +		uint16_t queue_id;
> +
> +		/*
> +		 * When configure the txq by rte_eth_tx_queue_setup with
> +		 * nb_tx_desc being 0, it will use a default value provided by
> +		 * PMDs to setup this txq. If the default value is 0, it will
> +		 * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> +		 */
> +		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> +			if (port->nb_tx_desc[queue_id])
> +				ring_size = port->nb_tx_desc[queue_id];
> +			else if (port->dev_info.default_txportconf.ring_size)
> +				ring_size =
> +				    port->dev_info.default_txportconf.ring_size;
> +			else
> +				ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> +
> +			if (ring_size < nb_segs) {
> +				printf("nb segments per TX packets=%u >= TX "
> +				       "queue(%u) ring_size=%u - ignored\n",
> +					nb_segs, queue_id, ring_size);
> +				return true;
> +			}
> +		}

What do you think calling 'rte_eth_rx_queue_info_get()' & 
'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?

Re: [dpdk-dev] [PATCH v3 5/6] app/testpmd: fix valid desc id check

[Ferruh Yigit]

On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> The number of desc is a per queue configuration. But in the check function,
> nb_txd & nb_rxd are used to check whether the desc_id is valid. nb_txd &
> nb_rxd are the global configuration of number of desc. If the queue
> configuration is changed by cmdline liks: "port config xx txq xx ring_size
> xxx", the real value will be changed.
> 
> This patch use the real value to check whether the desc_id is valid. And if
> these are not configured by user. It will use the default value to check
> it, since the rte_eth_rx_queue_setup & rte_eth_tx_queue_setup will use a
> default value to confiure the queue if nb_rx_desc or nb_tx_desc is zero.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>   app/test-pmd/config.c | 53 +++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 43 insertions(+), 10 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 882de2d..b7851c7 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1891,22 +1891,55 @@ tx_queue_id_is_invalid(queueid_t txq_id)
>   }
>   
>   static int
> -rx_desc_id_is_invalid(uint16_t rxdesc_id)
> +rx_desc_id_is_invalid(portid_t port_id, queueid_t rxq_id, uint16_t rxdesc_id)
>   {
> -	if (rxdesc_id < nb_rxd)
> +	struct rte_port *port = &ports[port_id];
> +	uint16_t ring_size;
> +
> +	/*
> +	 * When configure the rxq by rte_eth_rx_queue_setup with nb_rx_desc
> +	 * being 0, it will use a default value provided by PMDs to setup this
> +	 * rxq. If the default value is 0, it will use the
> +	 * RTE_ETH_DEV_FALLBACK_RX_RINGSIZE to setup this rxq.
> +	 */
> +	if (port->nb_rx_desc[rxq_id])
> +		ring_size = port->nb_rx_desc[rxq_id];
> +	else if (port->dev_info.default_rxportconf.ring_size)
> +		ring_size = port->dev_info.default_rxportconf.ring_size;
> +	else
> +		ring_size =  RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
> +
> +	if (rxdesc_id < ring_size)
>   		return 0;
> -	printf("Invalid RX descriptor %d (must be < nb_rxd=%d)\n",
> -	       rxdesc_id, nb_rxd);
> +	printf("Invalid RX descriptor %d (must be < ring_size=%d)\n",
> +	       rxdesc_id, ring_size);
>   	return 1;

+1 to fix, but similar comment as previous patch, can 
'rte_eth_rx_queue_info_get()' be used to detect the 'ring_size'?

Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Wei Hu (Xavier)]

Hi, Ferruh Yigit

On 2020/9/22 22:51, Ferruh Yigit wrote:
> On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
>> From: Chengchang Tang <tangchengchang@huawei.com>
>>
>> Currently, if nb_txd is not set, the txpkts is not allowed to be set
>> because the nb_txd is used to avoid the numer of segments exceed the Tx
>> ring size and the default value of nb_txd is 0. And there is a bug that
>> nb_txd is the global configuration for Tx ring size and the ring size
>> could be changed by some command per queue. So these valid check is
>> unreliable and introduced unnecessary constraints.
>>
>> This patch adds a valid check function to use the real Tx ring size to
>> check the validity of txpkts.
>>
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>> ---
>>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>> index 4e33208..882de2d 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>>       printf("Split packet: %s\n", split);
>>   }
>>   +static bool
>> +nb_segs_is_invalid(unsigned int nb_segs)
>> +{
>> +    uint16_t port_id;
>> +
>> +    RTE_ETH_FOREACH_DEV(port_id) {
>> +        struct rte_port *port = &ports[port_id];
>> +        uint16_t ring_size;
>> +        uint16_t queue_id;
>> +
>> +        /*
>> +         * When configure the txq by rte_eth_tx_queue_setup with
>> +         * nb_tx_desc being 0, it will use a default value provided by
>> +         * PMDs to setup this txq. If the default value is 0, it will
>> +         * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>> +         */
>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>> +            if (port->nb_tx_desc[queue_id])
>> +                ring_size = port->nb_tx_desc[queue_id];
>> +            else if (port->dev_info.default_txportconf.ring_size)
>> +                ring_size =
>> + port->dev_info.default_txportconf.ring_size;
>> +            else
>> +                ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>> +
>> +            if (ring_size < nb_segs) {
>> +                printf("nb segments per TX packets=%u >= TX "
>> +                       "queue(%u) ring_size=%u - ignored\n",
>> +                    nb_segs, queue_id, ring_size);
>> +                return true;
>> +            }
>> +        }
>
> What do you think calling 'rte_eth_rx_queue_info_get()' & 
> 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?

Currently not all PMD driver implement the .rxq_info_get and

.txq_info_get hook function. If calling rte_eth_rx_queue_info_get

return -ENOSTUP, we still need to obtain the ring_size in this way.


Thanks, Xavier

Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Wei Hu (Xavier)]

Hi, Ferruh Yigit

On 2020/9/23 11:14, Wei Hu (Xavier) wrote:
> Hi, Ferruh Yigit
> 
> On 2020/9/22 22:51, Ferruh Yigit wrote:
>> On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>
>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set
>>> because the nb_txd is used to avoid the numer of segments exceed the Tx
>>> ring size and the default value of nb_txd is 0. And there is a bug that
>>> nb_txd is the global configuration for Tx ring size and the ring size
>>> could be changed by some command per queue. So these valid check is
>>> unreliable and introduced unnecessary constraints.
>>>
>>> This patch adds a valid check function to use the real Tx ring size to
>>> check the validity of txpkts.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> ---
>>>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 38 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>> index 4e33208..882de2d 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>>>       printf("Split packet: %s\n", split);
>>>   }
>>>   +static bool
>>> +nb_segs_is_invalid(unsigned int nb_segs)
>>> +{
>>> +    uint16_t port_id;
>>> +
>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>> +        struct rte_port *port = &ports[port_id];
>>> +        uint16_t ring_size;
>>> +        uint16_t queue_id;
>>> +
>>> +        /*
>>> +         * When configure the txq by rte_eth_tx_queue_setup with
>>> +         * nb_tx_desc being 0, it will use a default value provided by
>>> +         * PMDs to setup this txq. If the default value is 0, it will
>>> +         * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>> +         */
>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>> +            if (port->nb_tx_desc[queue_id])
>>> +                ring_size = port->nb_tx_desc[queue_id];
>>> +            else if (port->dev_info.default_txportconf.ring_size)
>>> +                ring_size =
>>> + port->dev_info.default_txportconf.ring_size;
>>> +            else
>>> +                ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>> +
>>> +            if (ring_size < nb_segs) {
>>> +                printf("nb segments per TX packets=%u >= TX "
>>> +                       "queue(%u) ring_size=%u - ignored\n",
>>> +                    nb_segs, queue_id, ring_size);
>>> +                return true;
>>> +            }
>>> +        }
>>
>> What do you think calling 'rte_eth_rx_queue_info_get()' & 
>> 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
> 
> Currently not all PMD driver implement the .rxq_info_get and
> 
> .txq_info_get hook function. If calling rte_eth_rx_queue_info_get
> 
> return -ENOSTUP, we still need to obtain the ring_size in this way.
> 
If calling rte_eth_rx_queue_info_get function get ring_size, because not 
all PMDS implement the relevant the related hook function, we need to
check the return value and if the return value is -ENOSTUP, we must 
obtain the ring_size in this way.
Do you prefer this method, right?

Hopes for your suggestion.
Thanks, Xavier
> 
> Thanks, Xavier

Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 9/23/2020 12:57 PM, Wei Hu (Xavier) wrote:
> Hi, Ferruh Yigit
> 
> On 2020/9/23 11:14, Wei Hu (Xavier) wrote:
>> Hi, Ferruh Yigit
>>
>> On 2020/9/22 22:51, Ferruh Yigit wrote:
>>> On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>
>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set
>>>> because the nb_txd is used to avoid the numer of segments exceed the Tx
>>>> ring size and the default value of nb_txd is 0. And there is a bug that
>>>> nb_txd is the global configuration for Tx ring size and the ring size
>>>> could be changed by some command per queue. So these valid check is
>>>> unreliable and introduced unnecessary constraints.
>>>>
>>>> This patch adds a valid check function to use the real Tx ring size to
>>>> check the validity of txpkts.
>>>>
>>>> Fixes: af75078fece3 ("first public release")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> ---
>>>>   app/test-pmd/config.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>>>   1 file changed, 38 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
>>>> index 4e33208..882de2d 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -2984,17 +2984,51 @@ show_tx_pkt_segments(void)
>>>>       printf("Split packet: %s\n", split);
>>>>   }
>>>>   +static bool
>>>> +nb_segs_is_invalid(unsigned int nb_segs)
>>>> +{
>>>> +    uint16_t port_id;
>>>> +
>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>>> +        struct rte_port *port = &ports[port_id];
>>>> +        uint16_t ring_size;
>>>> +        uint16_t queue_id;
>>>> +
>>>> +        /*
>>>> +         * When configure the txq by rte_eth_tx_queue_setup with
>>>> +         * nb_tx_desc being 0, it will use a default value provided by
>>>> +         * PMDs to setup this txq. If the default value is 0, it will
>>>> +         * use the RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>> +         */
>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>> +            if (port->nb_tx_desc[queue_id])
>>>> +                ring_size = port->nb_tx_desc[queue_id];
>>>> +            else if (port->dev_info.default_txportconf.ring_size)
>>>> +                ring_size =
>>>> + port->dev_info.default_txportconf.ring_size;
>>>> +            else
>>>> +                ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>> +
>>>> +            if (ring_size < nb_segs) {
>>>> +                printf("nb segments per TX packets=%u >= TX "
>>>> +                       "queue(%u) ring_size=%u - ignored\n",
>>>> +                    nb_segs, queue_id, ring_size);
>>>> +                return true;
>>>> +            }
>>>> +        }
>>>
>>> What do you think calling 'rte_eth_rx_queue_info_get()' & 
>>> 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
>>
>> Currently not all PMD driver implement the .rxq_info_get and
>>
>> .txq_info_get hook function. If calling rte_eth_rx_queue_info_get
>>
>> return -ENOSTUP, we still need to obtain the ring_size in this way.
>>
> If calling rte_eth_rx_queue_info_get function get ring_size, because not 
> all PMDS implement the relevant the related hook function, we need to
> check the return value and if the return value is -ENOSTUP, we must 
> obtain the ring_size in this way.
> Do you prefer this method, right?
 >

Do we really need this check?

What about verifying 'ring_size' if 'rte_eth_rx_queue_info_get()' 
returns a valid info, if not ignore the check?

> 
> Hopes for your suggestion.
> Thanks, Xavier
>>
>> Thanks, Xavier

Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Chengchang Tang]

On 2020/9/24 0:59, Ferruh Yigit wrote:
> On 9/23/2020 12:57 PM, Wei Hu (Xavier) wrote:
>> Hi, Ferruh Yigit
>>
>> On 2020/9/23 11:14, Wei Hu (Xavier) wrote:
>>> Hi, Ferruh Yigit
>>>
>>> On 2020/9/22 22:51, Ferruh Yigit wrote:
>>>> On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>>
>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set
>>>>> because the nb_txd is used to avoid the numer of segments exceed the Tx
>>>>> ring size and the default value of nb_txd is 0. And there is a bug that
>>>>> nb_txd is the global configuration for Tx ring size and the ring size
>>>>> could be changed by some command per queue. So these valid check is
>>>>> unreliable and introduced unnecessary constraints.
>>>>>
>>>>> This patch adds a valid check function to use the real Tx ring size to
>>>>> check the validity of txpkts.
>>>>>
>>>>> Fixes: af75078fece3 ("first public release")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>> ---

<...>

>>>> What do you think calling 'rte_eth_rx_queue_info_get()' & 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
>>>
>>> Currently not all PMD driver implement the .rxq_info_get and
>>>
>>> .txq_info_get hook function. If calling rte_eth_rx_queue_info_get
>>>
>>> return -ENOSTUP, we still need to obtain the ring_size in this way.
>>>
>> If calling rte_eth_rx_queue_info_get function get ring_size, because not all PMDS implement the relevant the related hook function, we need to
>> check the return value and if the return value is -ENOSTUP, we must obtain the ring_size in this way.
>> Do you prefer this method, right?
>>
> 
> Do we really need this check?

IMHO, these checks are needed. There are two patches use the same method to obtain the ring_size to implement the
verification in the patch set. One is used to verify the validity of the descriptor ID in the ring. Another is used
to avoid the number of segments configuration won't exceed the ring_size. For the first case, if no check is
performed, memory out of bound may occur. For the another one, if no check is performed, the sending may fail when
the number of segment exceed the ring size.
> 
> What about verifying 'ring_size' if 'rte_eth_rx_queue_info_get()' returns a valid info, if not ignore the check?
> 

if we ignore the check when the 'rte_eth_rx_queue_info_get()' returns -ENOTSUP, it may still cause an illegal memory
access or sending failure.
>>
<...>
> .
> 

Re: [dpdk-dev] [PATCH v3 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 9/24/2020 7:08 AM, Chengchang Tang wrote:
> 
> 
> On 2020/9/24 0:59, Ferruh Yigit wrote:
>> On 9/23/2020 12:57 PM, Wei Hu (Xavier) wrote:
>>> Hi, Ferruh Yigit
>>>
>>> On 2020/9/23 11:14, Wei Hu (Xavier) wrote:
>>>> Hi, Ferruh Yigit
>>>>
>>>> On 2020/9/22 22:51, Ferruh Yigit wrote:
>>>>> On 9/19/2020 11:47 AM, Wei Hu (Xavier) wrote:
>>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>
>>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set
>>>>>> because the nb_txd is used to avoid the numer of segments exceed the Tx
>>>>>> ring size and the default value of nb_txd is 0. And there is a bug that
>>>>>> nb_txd is the global configuration for Tx ring size and the ring size
>>>>>> could be changed by some command per queue. So these valid check is
>>>>>> unreliable and introduced unnecessary constraints.
>>>>>>
>>>>>> This patch adds a valid check function to use the real Tx ring size to
>>>>>> check the validity of txpkts.
>>>>>>
>>>>>> Fixes: af75078fece3 ("first public release")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>> ---
> 
> <...>
> 
>>>>> What do you think calling 'rte_eth_rx_queue_info_get()' & 'rte_eth_tx_queue_info_get()' to get the 'nb_desc'?
>>>>
>>>> Currently not all PMD driver implement the .rxq_info_get and
>>>>
>>>> .txq_info_get hook function. If calling rte_eth_rx_queue_info_get
>>>>
>>>> return -ENOSTUP, we still need to obtain the ring_size in this way.
>>>>
>>> If calling rte_eth_rx_queue_info_get function get ring_size, because not all PMDS implement the relevant the related hook function, we need to
>>> check the return value and if the return value is -ENOSTUP, we must obtain the ring_size in this way.
>>> Do you prefer this method, right?
>>>
>>
>> Do we really need this check?
> 
> IMHO, these checks are needed. There are two patches use the same method to obtain the ring_size to implement the
> verification in the patch set. One is used to verify the validity of the descriptor ID in the ring. Another is used
> to avoid the number of segments configuration won't exceed the ring_size. For the first case, if no check is
> performed, memory out of bound may occur. For the another one, if no check is performed, the sending may fail when
> the number of segment exceed the ring size.
>>
>> What about verifying 'ring_size' if 'rte_eth_rx_queue_info_get()' returns a valid info, if not ignore the check?
>>
> 
> if we ignore the check when the 'rte_eth_rx_queue_info_get()' returns -ENOTSUP, it may still cause an illegal memory
> access or sending failure.
 >

Ok, thanks for clarification, agree to your suggestion. (to check 
'rte_eth_rx_queue_info_get()' return value and if it is '-ENOSTUP' 
calculate the 'ring_size' as above.)

[dpdk-dev] [PATCH v4 0/6] minor fixes for testpmd

[Wei Hu (Xavier)]

This series are minor fixes for testpmd application.

Chengchang Tang (5):
  app/testpmd: fix missing verification of port id
  app/testpmd: fix VLAN offload configuration when config fail
  app/testpmd: remove restriction on txpkts set
  app/testpmd: fix packet header in txonly mode
  app/testpmd: fix valid desc id check

Huisong Li (1):
  app/testpmd: fix displaying Rx Tx queues information

 app/test-pmd/cmdline.c |   9 ++
 app/test-pmd/config.c  | 226 ++++++++++++++++++++++++++++++++++++++-----------
 app/test-pmd/txonly.c  |  32 +++++++
 3 files changed, 218 insertions(+), 49 deletions(-)

-- 
2.9.5

[dpdk-dev] [PATCH v4 1/6] app/testpmd: fix missing verification of port id

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

To set Tx vlan offloads, it is required to stop port firstly. But before
checking whether the port is stopped, the port id entered by the user
is not checked for validity. When the port id is illegal, it would lead
to a segmentation fault since it attempts to access a member of
non-existent port.

This patch adds verification of port id in tx vlan offloads and remove
duplicated check.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/cmdline.c | 9 +++++++++
 app/test-pmd/config.c  | 6 ------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 5f93409..43b7c2f 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -4291,6 +4291,9 @@ cmd_tx_vlan_set_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4345,6 +4348,9 @@ cmd_tx_vlan_set_qinq_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_set_qinq_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
@@ -4458,6 +4464,9 @@ cmd_tx_vlan_reset_parsed(void *parsed_result,
 {
 	struct cmd_tx_vlan_reset_result *res = parsed_result;
 
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
 	if (!port_is_stopped(res->port_id)) {
 		printf("Please stop port %d first\n", res->port_id);
 		return;
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 2d9a456..2ee40df 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3547,8 +3547,6 @@ tx_vlan_set(portid_t port_id, uint16_t vlan_id)
 	struct rte_eth_dev_info dev_info;
 	int ret;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
 	if (vlan_id_is_invalid(vlan_id))
 		return;
 
@@ -3579,8 +3577,6 @@ tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer)
 	struct rte_eth_dev_info dev_info;
 	int ret;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
 	if (vlan_id_is_invalid(vlan_id))
 		return;
 	if (vlan_id_is_invalid(vlan_id_outer))
@@ -3606,8 +3602,6 @@ tx_qinq_set(portid_t port_id, uint16_t vlan_id, uint16_t vlan_id_outer)
 void
 tx_vlan_reset(portid_t port_id)
 {
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
 	ports[port_id].dev_conf.txmode.offloads &=
 				~(DEV_TX_OFFLOAD_VLAN_INSERT |
 				  DEV_TX_OFFLOAD_QINQ_INSERT);
-- 
2.9.5

[dpdk-dev] [PATCH v4 2/6] app/testpmd: fix VLAN offload configuration when config fail

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

When failing to configure VLAN offloads after the port was started, there
is no need to update the port configuration. Currently, when user
configure an unsupported VLAN offloads and fails, and then restart the
port, it will fails since the configuration has been refreshed.

This patch makes the function return directly insead of refreshing the
configuration when execution fails.

Fixes: 384161e00627 ("app/testpmd: adjust on the fly VLAN configuration")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/config.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 2ee40df..6496d2f 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3392,9 +3392,11 @@ vlan_extend_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_extend_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3419,9 +3421,11 @@ rx_vlan_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_strip_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3460,9 +3464,11 @@ rx_vlan_filter_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("rx_vlan_filter_set(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
@@ -3487,9 +3493,11 @@ rx_vlan_qinq_strip_set(portid_t port_id, int on)
 	}
 
 	diag = rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
-	if (diag < 0)
+	if (diag < 0) {
 		printf("%s(port_pi=%d, on=%d) failed "
 	       "diag=%d\n", __func__, port_id, on, diag);
+		return;
+	}
 	ports[port_id].dev_conf.rxmode.offloads = port_rx_offloads;
 }
 
-- 
2.9.5

[dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

Currently, if nb_txd is not set, the txpkts is not allowed to be set
because the nb_txd is used to avoid the numer of segments exceed the Tx
ring size and the default value of nb_txd is 0. And there is a bug that
nb_txd is the global configuration for Tx ring size and the ring size
could be changed by some command per queue. So these valid check is
unreliable and introduced unnecessary constraints.

This patch adds a valid check function to use the real Tx ring size to
check the validity of txpkts.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
v3 -> v4:
	 add check 'rte_eth_rx_queue_info_get()' return value and
	 if it is '-ENOSTUP' calculate the 'ring_size'.
v3:      initial version.
---
 app/test-pmd/config.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 6496d2f..8ebb927 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)
 }
 
 static int
+get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t *ring_size)
+{
+	struct rte_port *port = &ports[port_id];
+	struct rte_eth_txq_info tx_qinfo;
+	int ret;
+
+	ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
+	if (ret == 0) {
+		*ring_size = tx_qinfo.nb_desc;
+		return ret;
+	}
+
+	if (ret != -ENOTSUP)
+		return ret;
+	/*
+	 * If the rte_eth_tx_queue_info_get is not support for this PMD,
+	 * ring_size stored in testpmd will be used for validity verification.
+	 * When configure the txq by rte_eth_tx_queue_setup with nb_tx_desc
+	 * being 0, it will use a default value provided by PMDs to setup this
+	 * txq. If the default value is 0, it will use the
+	 * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
+	 */
+	if (port->nb_tx_desc[txq_id])
+		*ring_size = port->nb_tx_desc[txq_id];
+	else if (port->dev_info.default_txportconf.ring_size)
+		*ring_size = port->dev_info.default_txportconf.ring_size;
+	else
+		*ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+	return 0;
+}
+
+static int
 rx_desc_id_is_invalid(uint16_t rxdesc_id)
 {
 	if (rxdesc_id < nb_rxd)
@@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
 	printf("Split packet: %s\n", split);
 }
 
+static bool
+nb_segs_is_invalid(unsigned int nb_segs)
+{
+	uint16_t ring_size;
+	uint16_t queue_id;
+	uint16_t port_id;
+	int ret;
+
+	RTE_ETH_FOREACH_DEV(port_id) {
+		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
+			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
+
+			if (ret)
+				return true;
+
+			if (ring_size < nb_segs) {
+				printf("nb segments per TX packets=%u >= "
+				       "TX queue(%u) ring_size=%u - ignored\n",
+				       nb_segs, queue_id, ring_size);
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
 void
 set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
 {
 	uint16_t tx_pkt_len;
 	unsigned i;
 
-	if (nb_segs >= (unsigned) nb_txd) {
-		printf("nb segments per TX packets=%u >= nb_txd=%u - ignored\n",
-		       nb_segs, (unsigned int) nb_txd);
+	if (nb_segs_is_invalid(nb_segs))
 		return;
-	}
 
 	/*
 	 * Check that each segment length is greater or equal than
-- 
2.9.5

[dpdk-dev] [PATCH v4 4/6] app/testpmd: fix packet header in txonly mode

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

In txonly forward mode, the packet header is fixed by the initial
setting, including the packet length and checksum. So when the packets
varies, this may cause a packet header error. Currently, there are two
methods in txonly mode to randomly change the packets.
1. Set txsplit random and txpkts (x[,y]*), the number of segments
   each packets will be a random value between 1 and total number of
   segments determined by txpkts settings.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --disable-device-start
     b) port config 0 tx_offload multi_segs on
     c) set fwd txonly
     d) set txsplit rand
     e) set txpkts 2048,2048,2048,2048
     f) start
The nb_segs of the packets sent by testpmd will be 1~4. The real packet
length will be 2048, 4096, 6144 and 8192. But in fact the packet length
in ip header and udp header will be fixed by 8178 and 8158.

2. Set txonly-multi-flow. the ip address will be varied to generate
   multiple flow.
   The step as follows:
     a) ./testpmd -w xxx -l xx -n 4 -- -i --txonly-multi-flow
     b) set fwd txonly
     c) start
The ip address of each pkts will change randomly, but since the header
is fixed, the checksum may be a error value.

Therefore, this patch adds a function to update the packet length and
check sum in the pkts header when the txsplit mode is set to rand or
multi-flow is set.

Fixes: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple flows")
Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
 app/test-pmd/txonly.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 45def72..d55ee7c 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -156,6 +156,34 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
 	ip_hdr->hdr_checksum = (uint16_t) ip_cksum;
 }
 
+static inline void
+update_pkt_header(struct rte_mbuf *pkt, uint32_t total_pkt_len)
+{
+	struct rte_ipv4_hdr *ip_hdr;
+	struct rte_udp_hdr *udp_hdr;
+	uint16_t pkt_data_len;
+	uint16_t pkt_len;
+
+	pkt_data_len = (uint16_t) (total_pkt_len - (
+					sizeof(struct rte_ether_hdr) +
+					sizeof(struct rte_ipv4_hdr) +
+					sizeof(struct rte_udp_hdr)));
+	/* updata udp pkt length */
+	udp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_udp_hdr *,
+				sizeof(struct rte_ether_hdr) +
+				sizeof(struct rte_ipv4_hdr));
+	pkt_len = (uint16_t) (pkt_data_len + sizeof(struct rte_udp_hdr));
+	udp_hdr->dgram_len = RTE_CPU_TO_BE_16(pkt_len);
+
+	/* updata ip pkt length and csum */
+	ip_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_ipv4_hdr *,
+				sizeof(struct rte_ether_hdr));
+	ip_hdr->hdr_checksum = 0;
+	pkt_len = (uint16_t) (pkt_len + sizeof(struct rte_ipv4_hdr));
+	ip_hdr->total_length = RTE_CPU_TO_BE_16(pkt_len);
+	ip_hdr->hdr_checksum = rte_ipv4_cksum(ip_hdr);
+}
+
 static inline bool
 pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 		struct rte_ether_hdr *eth_hdr, const uint16_t vlan_tci,
@@ -223,6 +251,10 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp,
 	copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt,
 			sizeof(struct rte_ether_hdr) +
 			sizeof(struct rte_ipv4_hdr));
+
+	if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND) || txonly_multi_flow)
+		update_pkt_header(pkt, pkt_len);
+
 	if (unlikely(timestamp_enable)) {
 		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
 		struct {
-- 
2.9.5

[dpdk-dev] [PATCH v4 5/6] app/testpmd: fix valid desc id check

[Wei Hu (Xavier)]

From: Chengchang Tang <tangchengchang@huawei.com>

The number of desc is a per queue configuration. But in the check function,
nb_txd & nb_rxd are used to check whether the desc_id is valid. nb_txd &
nb_rxd are the global configuration of number of desc. If the queue
configuration is changed by cmdline liks: "port config xx txq xx ring_size
xxx", the real value will be changed.

This patch use the real value to check whether the desc_id is valid. And if
these are not configured by user. It will use the default value to check
it, since the rte_eth_rx_queue_setup & rte_eth_tx_queue_setup will use a
default value to confiure the queue if nb_rx_desc or nb_tx_desc is zero.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
v3 -> v4:
	add check 'rte_eth_rx_queue_info_get()' return value and
         if it is '-ENOSTUP' calculate the 'ring_size'.
v3: initial version.
---
 app/test-pmd/config.c | 76 +++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 58 insertions(+), 18 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 8ebb927..791f8d8 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)
 }
 
 static int
+get_rx_ring_size(portid_t port_id, queueid_t rxq_id, uint16_t *ring_size)
+{
+	struct rte_port *port = &ports[port_id];
+	struct rte_eth_rxq_info rx_qinfo;
+	int ret;
+
+	ret = rte_eth_rx_queue_info_get(port_id, rxq_id, &rx_qinfo);
+	if (ret == 0) {
+		*ring_size = rx_qinfo.nb_desc;
+		return ret;
+	}
+
+	if (ret != -ENOTSUP)
+		return ret;
+	/*
+	 * If the rte_eth_rx_queue_info_get is not support for this PMD,
+	 * ring_size stored in testpmd will be used for validity verification.
+	 * When configure the rxq by rte_eth_rx_queue_setup with nb_rx_desc
+	 * being 0, it will use a default value provided by PMDs to setup this
+	 * rxq. If the default value is 0, it will use the
+	 * RTE_ETH_DEV_FALLBACK_RX_RINGSIZE to setup this rxq.
+	 */
+	if (port->nb_rx_desc[rxq_id])
+		*ring_size = port->nb_rx_desc[rxq_id];
+	else if (port->dev_info.default_rxportconf.ring_size)
+		*ring_size = port->dev_info.default_rxportconf.ring_size;
+	else
+		*ring_size = RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+	return 0;
+}
+
+static int
 get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t *ring_size)
 {
 	struct rte_port *port = &ports[port_id];
@@ -1925,22 +1957,38 @@ get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t *ring_size)
 }
 
 static int
-rx_desc_id_is_invalid(uint16_t rxdesc_id)
+rx_desc_id_is_invalid(portid_t port_id, queueid_t rxq_id, uint16_t rxdesc_id)
 {
-	if (rxdesc_id < nb_rxd)
+	uint16_t ring_size;
+	int ret;
+
+	ret = get_rx_ring_size(port_id, rxq_id, &ring_size);
+	if (ret)
+		return 1;
+
+	if (rxdesc_id < ring_size)
 		return 0;
-	printf("Invalid RX descriptor %d (must be < nb_rxd=%d)\n",
-	       rxdesc_id, nb_rxd);
+
+	printf("Invalid RX descriptor %u (must be < ring_size=%u)\n",
+	       rxdesc_id, ring_size);
 	return 1;
 }
 
 static int
-tx_desc_id_is_invalid(uint16_t txdesc_id)
+tx_desc_id_is_invalid(portid_t port_id, queueid_t txq_id, uint16_t txdesc_id)
 {
-	if (txdesc_id < nb_txd)
+	uint16_t ring_size;
+	int ret;
+
+	ret = get_tx_ring_size(port_id, txq_id, &ring_size);
+	if (ret)
+		return 1;
+
+	if (txdesc_id < ring_size)
 		return 0;
-	printf("Invalid TX descriptor %d (must be < nb_txd=%d)\n",
-	       txdesc_id, nb_txd);
+
+	printf("Invalid TX descriptor %u (must be < ring_size=%u)\n",
+	       txdesc_id, ring_size);
 	return 1;
 }
 
@@ -2061,11 +2109,7 @@ rx_ring_desc_display(portid_t port_id, queueid_t rxq_id, uint16_t rxd_id)
 {
 	const struct rte_memzone *rx_mz;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
-	if (rx_queue_id_is_invalid(rxq_id))
-		return;
-	if (rx_desc_id_is_invalid(rxd_id))
+	if (rx_desc_id_is_invalid(port_id, rxq_id, rxd_id))
 		return;
 	rx_mz = ring_dma_zone_lookup("rx_ring", port_id, rxq_id);
 	if (rx_mz == NULL)
@@ -2078,11 +2122,7 @@ tx_ring_desc_display(portid_t port_id, queueid_t txq_id, uint16_t txd_id)
 {
 	const struct rte_memzone *tx_mz;
 
-	if (port_id_is_invalid(port_id, ENABLED_WARN))
-		return;
-	if (tx_queue_id_is_invalid(txq_id))
-		return;
-	if (tx_desc_id_is_invalid(txd_id))
+	if (tx_desc_id_is_invalid(port_id, txq_id, txd_id))
 		return;
 	tx_mz = ring_dma_zone_lookup("tx_ring", port_id, txq_id);
 	if (tx_mz == NULL)
-- 
2.9.5

[dpdk-dev] [PATCH v4 6/6] app/testpmd: fix displaying Rx Tx queues information

[Wei Hu (Xavier)]

From: Huisong Li <lihuisong@huawei.com>

Currently, the information of Rx/Tx queues from PMD driver is not displayed
exactly in the rxtx_config_display function. Because "ports[pid].rx_conf"
and "ports[pid].tx_conf" maintained in testpmd application may be not the
value actually used by PMD driver. For instance, user does not set a field,
but PMD driver has to use the default value.

This patch fixes rxtx_config_display so that the information of Rx/Tx
queues can be really displayed for the PMD driver that implement
.rxq_info_get and .txq_info_get ops callback function.

Fixes: 75c530c1bd5351 ("app/testpmd: fix port configuration print")
Fixes: d44f8a485f5d1f ("app/testpmd: enable per queue configure")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/config.c | 64 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 791f8d8..6a0058c 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -2163,10 +2163,17 @@ rxtx_config_display(void)
 		struct rte_eth_txconf *tx_conf = &ports[pid].tx_conf[0];
 		uint16_t *nb_rx_desc = &ports[pid].nb_rx_desc[0];
 		uint16_t *nb_tx_desc = &ports[pid].nb_tx_desc[0];
-		uint16_t nb_rx_desc_tmp;
-		uint16_t nb_tx_desc_tmp;
 		struct rte_eth_rxq_info rx_qinfo;
 		struct rte_eth_txq_info tx_qinfo;
+		uint16_t rx_free_thresh_tmp;
+		uint16_t tx_free_thresh_tmp;
+		uint16_t tx_rs_thresh_tmp;
+		uint16_t nb_rx_desc_tmp;
+		uint16_t nb_tx_desc_tmp;
+		uint64_t offloads_tmp;
+		uint8_t pthresh_tmp;
+		uint8_t hthresh_tmp;
+		uint8_t wthresh_tmp;
 		int32_t rc;
 
 		/* per port config */
@@ -2180,41 +2187,64 @@ rxtx_config_display(void)
 		/* per rx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_rx_queue_info_get(pid, qid, &rx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_rx_desc_tmp = nb_rx_desc[qid];
-			else
+				rx_free_thresh_tmp =
+					rx_conf[qid].rx_free_thresh;
+				pthresh_tmp = rx_conf[qid].rx_thresh.pthresh;
+				hthresh_tmp = rx_conf[qid].rx_thresh.hthresh;
+				wthresh_tmp = rx_conf[qid].rx_thresh.wthresh;
+				offloads_tmp = rx_conf[qid].offloads;
+			} else {
 				nb_rx_desc_tmp = rx_qinfo.nb_desc;
+				rx_free_thresh_tmp =
+						rx_qinfo.conf.rx_free_thresh;
+				pthresh_tmp = rx_qinfo.conf.rx_thresh.pthresh;
+				hthresh_tmp = rx_qinfo.conf.rx_thresh.hthresh;
+				wthresh_tmp = rx_qinfo.conf.rx_thresh.wthresh;
+				offloads_tmp = rx_qinfo.conf.offloads;
+			}
 
 			printf("    RX queue: %d\n", qid);
 			printf("      RX desc=%d - RX free threshold=%d\n",
-				nb_rx_desc_tmp, rx_conf[qid].rx_free_thresh);
+				nb_rx_desc_tmp, rx_free_thresh_tmp);
 			printf("      RX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				rx_conf[qid].rx_thresh.pthresh,
-				rx_conf[qid].rx_thresh.hthresh,
-				rx_conf[qid].rx_thresh.wthresh);
-			printf("      RX Offloads=0x%"PRIx64"\n",
-				rx_conf[qid].offloads);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
+			printf("      RX Offloads=0x%"PRIx64"\n", offloads_tmp);
 		}
 
 		/* per tx queue config only for first queue to be less verbose */
 		for (qid = 0; qid < 1; qid++) {
 			rc = rte_eth_tx_queue_info_get(pid, qid, &tx_qinfo);
-			if (rc)
+			if (rc) {
 				nb_tx_desc_tmp = nb_tx_desc[qid];
-			else
+				tx_free_thresh_tmp =
+					tx_conf[qid].tx_free_thresh;
+				pthresh_tmp = tx_conf[qid].tx_thresh.pthresh;
+				hthresh_tmp = tx_conf[qid].tx_thresh.hthresh;
+				wthresh_tmp = tx_conf[qid].tx_thresh.wthresh;
+				offloads_tmp = tx_conf[qid].offloads;
+				tx_rs_thresh_tmp = tx_conf[qid].tx_rs_thresh;
+			} else {
 				nb_tx_desc_tmp = tx_qinfo.nb_desc;
+				tx_free_thresh_tmp =
+						tx_qinfo.conf.tx_free_thresh;
+				pthresh_tmp = tx_qinfo.conf.tx_thresh.pthresh;
+				hthresh_tmp = tx_qinfo.conf.tx_thresh.hthresh;
+				wthresh_tmp = tx_qinfo.conf.tx_thresh.wthresh;
+				offloads_tmp = tx_qinfo.conf.offloads;
+				tx_rs_thresh_tmp = tx_qinfo.conf.tx_rs_thresh;
+			}
 
 			printf("    TX queue: %d\n", qid);
 			printf("      TX desc=%d - TX free threshold=%d\n",
-				nb_tx_desc_tmp, tx_conf[qid].tx_free_thresh);
+				nb_tx_desc_tmp, tx_free_thresh_tmp);
 			printf("      TX threshold registers: pthresh=%d hthresh=%d "
 				" wthresh=%d\n",
-				tx_conf[qid].tx_thresh.pthresh,
-				tx_conf[qid].tx_thresh.hthresh,
-				tx_conf[qid].tx_thresh.wthresh);
+				pthresh_tmp, hthresh_tmp, wthresh_tmp);
 			printf("      TX offloads=0x%"PRIx64" - TX RS bit threshold=%d\n",
-				tx_conf[qid].offloads, tx_conf->tx_rs_thresh);
+				offloads_tmp, tx_rs_thresh_tmp);
 		}
 	}
 }
-- 
2.9.5

Re: [dpdk-dev] [PATCH v4 4/6] app/testpmd: fix packet header in txonly mode

[Ferruh Yigit]

On 9/25/2020 1:47 PM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> In txonly forward mode, the packet header is fixed by the initial
> setting, including the packet length and checksum. So when the packets
> varies, this may cause a packet header error. Currently, there are two
> methods in txonly mode to randomly change the packets.
> 1. Set txsplit random and txpkts (x[,y]*), the number of segments
>     each packets will be a random value between 1 and total number of
>     segments determined by txpkts settings.
>     The step as follows:
>       a) ./testpmd -w xxx -l xx -n 4 -- -i --disable-device-start
>       b) port config 0 tx_offload multi_segs on

Hi Xavier,

I confirm previously mentioned wrong sized packet is fixed with about setting, 
thanks for this.

>       c) set fwd txonly
>       d) set txsplit rand
>       e) set txpkts 2048,2048,2048,2048
>       f) start
> The nb_segs of the packets sent by testpmd will be 1~4. The real packet
> length will be 2048, 4096, 6144 and 8192. But in fact the packet length
> in ip header and udp header will be fixed by 8178 and 8158.
> 
> 2. Set txonly-multi-flow. the ip address will be varied to generate
>     multiple flow.
>     The step as follows:
>       a) ./testpmd -w xxx -l xx -n 4 -- -i --txonly-multi-flow
>       b) set fwd txonly
>       c) start
> The ip address of each pkts will change randomly, but since the header
> is fixed, the checksum may be a error value.
> 
> Therefore, this patch adds a function to update the packet length and
> check sum in the pkts header when the txsplit mode is set to rand or
> multi-flow is set.
> 
> Fixes: 82010ef55e7c ("app/testpmd: make txonly mode generate multiple flows")
> Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>

<...>

Re: [dpdk-dev] [PATCH v4 0/6] minor fixes for testpmd

[Ferruh Yigit]

On 9/25/2020 1:47 PM, Wei Hu (Xavier) wrote:
> This series are minor fixes for testpmd application.
> 
> Chengchang Tang (5):
>    app/testpmd: fix missing verification of port id
>    app/testpmd: fix VLAN offload configuration when config fail
>    app/testpmd: remove restriction on txpkts set
>    app/testpmd: fix packet header in txonly mode
>    app/testpmd: fix valid desc id check
> 
> Huisong Li (1):
>    app/testpmd: fix displaying Rx Tx queues information
> 

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Slava Ovsiienko]

Hi,  Wei

It was found this patch rejects the --txpkts command line settings.
set_tx_pkt_segments() is called before device started and
we have failure chain:

set_tx_pkt_segments()
  nb_segs_is_invalid()
    get_tx_ring_size ()
     rte_eth_tx_queue_info_get()

It causes --txpkts testpmd command line option is ignored.

With best regards, Slava

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
> Sent: Friday, September 25, 2020 15:47
> To: dev@dpdk.org
> Cc: xavier.huwei@huawei.com
> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts
> set
> 
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> Currently, if nb_txd is not set, the txpkts is not allowed to be set because the
> nb_txd is used to avoid the numer of segments exceed the Tx ring size and the
> default value of nb_txd is 0. And there is a bug that nb_txd is the global
> configuration for Tx ring size and the ring size could be changed by some
> command per queue. So these valid check is unreliable and introduced
> unnecessary constraints.
> 
> This patch adds a valid check function to use the real Tx ring size to check the
> validity of txpkts.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
> v3 -> v4:
> 	 add check 'rte_eth_rx_queue_info_get()' return value and
> 	 if it is '-ENOSTUP' calculate the 'ring_size'.
> v3:      initial version.
> ---
>  app/test-pmd/config.c | 64
> +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 6496d2f..8ebb927 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
> 
>  static int
> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
> +*ring_size) {
> +	struct rte_port *port = &ports[port_id];
> +	struct rte_eth_txq_info tx_qinfo;
> +	int ret;
> +
> +	ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
> +	if (ret == 0) {
> +		*ring_size = tx_qinfo.nb_desc;
> +		return ret;
> +	}
> +
> +	if (ret != -ENOTSUP)
> +		return ret;
> +	/*
> +	 * If the rte_eth_tx_queue_info_get is not support for this PMD,
> +	 * ring_size stored in testpmd will be used for validity verification.
> +	 * When configure the txq by rte_eth_tx_queue_setup with
> nb_tx_desc
> +	 * being 0, it will use a default value provided by PMDs to setup this
> +	 * txq. If the default value is 0, it will use the
> +	 * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> +	 */
> +	if (port->nb_tx_desc[txq_id])
> +		*ring_size = port->nb_tx_desc[txq_id];
> +	else if (port->dev_info.default_txportconf.ring_size)
> +		*ring_size = port->dev_info.default_txportconf.ring_size;
> +	else
> +		*ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> +	return 0;
> +}
> +
> +static int
>  rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>  	if (rxdesc_id < nb_rxd)
> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>  	printf("Split packet: %s\n", split);
>  }
> 
> +static bool
> +nb_segs_is_invalid(unsigned int nb_segs) {
> +	uint16_t ring_size;
> +	uint16_t queue_id;
> +	uint16_t port_id;
> +	int ret;
> +
> +	RTE_ETH_FOREACH_DEV(port_id) {
> +		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> +			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
> +
> +			if (ret)
> +				return true;
> +
> +			if (ring_size < nb_segs) {
> +				printf("nb segments per TX packets=%u >= "
> +				       "TX queue(%u) ring_size=%u - ignored\n",
> +				       nb_segs, queue_id, ring_size);
> +				return true;
> +			}
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  void
>  set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
>  	uint16_t tx_pkt_len;
>  	unsigned i;
> 
> -	if (nb_segs >= (unsigned) nb_txd) {
> -		printf("nb segments per TX packets=%u >= nb_txd=%u -
> ignored\n",
> -		       nb_segs, (unsigned int) nb_txd);
> +	if (nb_segs_is_invalid(nb_segs))
>  		return;
> -	}
> 
>  	/*
>  	 * Check that each segment length is greater or equal than
> --
> 2.9.5

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Thomas Monjalon]

Is it OK to keep this regression?

Ferruh, what do you suggest?


23/11/2020 12:50, Slava Ovsiienko:
> Hi,  Wei
> 
> It was found this patch rejects the --txpkts command line settings.
> set_tx_pkt_segments() is called before device started and
> we have failure chain:
> 
> set_tx_pkt_segments()
>   nb_segs_is_invalid()
>     get_tx_ring_size ()
>      rte_eth_tx_queue_info_get()
> 
> It causes --txpkts testpmd command line option is ignored.
> 
> With best regards, Slava
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
> > Sent: Friday, September 25, 2020 15:47
> > To: dev@dpdk.org
> > Cc: xavier.huwei@huawei.com
> > Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts
> > set
> > 
> > From: Chengchang Tang <tangchengchang@huawei.com>
> > 
> > Currently, if nb_txd is not set, the txpkts is not allowed to be set because the
> > nb_txd is used to avoid the numer of segments exceed the Tx ring size and the
> > default value of nb_txd is 0. And there is a bug that nb_txd is the global
> > configuration for Tx ring size and the ring size could be changed by some
> > command per queue. So these valid check is unreliable and introduced
> > unnecessary constraints.
> > 
> > This patch adds a valid check function to use the real Tx ring size to check the
> > validity of txpkts.
> > 
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> > ---
> > v3 -> v4:
> > 	 add check 'rte_eth_rx_queue_info_get()' return value and
> > 	 if it is '-ENOSTUP' calculate the 'ring_size'.
> > v3:      initial version.
> > ---
> >  app/test-pmd/config.c | 64
> > +++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 60 insertions(+), 4 deletions(-)
> > 
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 6496d2f..8ebb927 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
> > 
> >  static int
> > +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
> > +*ring_size) {
> > +	struct rte_port *port = &ports[port_id];
> > +	struct rte_eth_txq_info tx_qinfo;
> > +	int ret;
> > +
> > +	ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
> > +	if (ret == 0) {
> > +		*ring_size = tx_qinfo.nb_desc;
> > +		return ret;
> > +	}
> > +
> > +	if (ret != -ENOTSUP)
> > +		return ret;
> > +	/*
> > +	 * If the rte_eth_tx_queue_info_get is not support for this PMD,
> > +	 * ring_size stored in testpmd will be used for validity verification.
> > +	 * When configure the txq by rte_eth_tx_queue_setup with
> > nb_tx_desc
> > +	 * being 0, it will use a default value provided by PMDs to setup this
> > +	 * txq. If the default value is 0, it will use the
> > +	 * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> > +	 */
> > +	if (port->nb_tx_desc[txq_id])
> > +		*ring_size = port->nb_tx_desc[txq_id];
> > +	else if (port->dev_info.default_txportconf.ring_size)
> > +		*ring_size = port->dev_info.default_txportconf.ring_size;
> > +	else
> > +		*ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> > +	return 0;
> > +}
> > +
> > +static int
> >  rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
> >  	if (rxdesc_id < nb_rxd)
> > @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
> >  	printf("Split packet: %s\n", split);
> >  }
> > 
> > +static bool
> > +nb_segs_is_invalid(unsigned int nb_segs) {
> > +	uint16_t ring_size;
> > +	uint16_t queue_id;
> > +	uint16_t port_id;
> > +	int ret;
> > +
> > +	RTE_ETH_FOREACH_DEV(port_id) {
> > +		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> > +			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
> > +
> > +			if (ret)
> > +				return true;
> > +
> > +			if (ring_size < nb_segs) {
> > +				printf("nb segments per TX packets=%u >= "
> > +				       "TX queue(%u) ring_size=%u - ignored\n",
> > +				       nb_segs, queue_id, ring_size);
> > +				return true;
> > +			}
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  void
> >  set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
> >  	uint16_t tx_pkt_len;
> >  	unsigned i;
> > 
> > -	if (nb_segs >= (unsigned) nb_txd) {
> > -		printf("nb segments per TX packets=%u >= nb_txd=%u -
> > ignored\n",
> > -		       nb_segs, (unsigned int) nb_txd);
> > +	if (nb_segs_is_invalid(nb_segs))
> >  		return;
> > -	}
> > 
> >  	/*
> >  	 * Check that each segment length is greater or equal than
> > --
> > 2.9.5
> 
> 

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
> Is it OK to keep this regression?
> 
> Ferruh, what do you suggest?
> 

I confirm the '--txpkts' parameter is broken now, I suggest submitting a defect 
for it and continue with the regression.

We have alternative for the parameter, "set txpkts <len0[,len1]*>" command.
The parameter was only working when hardcoded '--txd=N' parameter is provided, 
the command is more dynamic and works however queue size is configured.

We can fix the '--txpkts' in next release.

> 
> 23/11/2020 12:50, Slava Ovsiienko:
>> Hi,  Wei
>>
>> It was found this patch rejects the --txpkts command line settings.
>> set_tx_pkt_segments() is called before device started and
>> we have failure chain:
>>
>> set_tx_pkt_segments()
>>    nb_segs_is_invalid()
>>      get_tx_ring_size ()
>>       rte_eth_tx_queue_info_get()
>>
>> It causes --txpkts testpmd command line option is ignored.
>>
>> With best regards, Slava
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
>>> Sent: Friday, September 25, 2020 15:47
>>> To: dev@dpdk.org
>>> Cc: xavier.huwei@huawei.com
>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts
>>> set
>>>
>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>
>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set because the
>>> nb_txd is used to avoid the numer of segments exceed the Tx ring size and the
>>> default value of nb_txd is 0. And there is a bug that nb_txd is the global
>>> configuration for Tx ring size and the ring size could be changed by some
>>> command per queue. So these valid check is unreliable and introduced
>>> unnecessary constraints.
>>>
>>> This patch adds a valid check function to use the real Tx ring size to check the
>>> validity of txpkts.
>>>
>>> Fixes: af75078fece3 ("first public release")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>> ---
>>> v3 -> v4:
>>> 	 add check 'rte_eth_rx_queue_info_get()' return value and
>>> 	 if it is '-ENOSTUP' calculate the 'ring_size'.
>>> v3:      initial version.
>>> ---
>>>   app/test-pmd/config.c | 64
>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 60 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>> 6496d2f..8ebb927 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
>>>
>>>   static int
>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
>>> +*ring_size) {
>>> +	struct rte_port *port = &ports[port_id];
>>> +	struct rte_eth_txq_info tx_qinfo;
>>> +	int ret;
>>> +
>>> +	ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
>>> +	if (ret == 0) {
>>> +		*ring_size = tx_qinfo.nb_desc;
>>> +		return ret;
>>> +	}
>>> +
>>> +	if (ret != -ENOTSUP)
>>> +		return ret;
>>> +	/*
>>> +	 * If the rte_eth_tx_queue_info_get is not support for this PMD,
>>> +	 * ring_size stored in testpmd will be used for validity verification.
>>> +	 * When configure the txq by rte_eth_tx_queue_setup with
>>> nb_tx_desc
>>> +	 * being 0, it will use a default value provided by PMDs to setup this
>>> +	 * txq. If the default value is 0, it will use the
>>> +	 * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>> +	 */
>>> +	if (port->nb_tx_desc[txq_id])
>>> +		*ring_size = port->nb_tx_desc[txq_id];
>>> +	else if (port->dev_info.default_txportconf.ring_size)
>>> +		*ring_size = port->dev_info.default_txportconf.ring_size;
>>> +	else
>>> +		*ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>>   rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>>>   	if (rxdesc_id < nb_rxd)
>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>>>   	printf("Split packet: %s\n", split);
>>>   }
>>>
>>> +static bool
>>> +nb_segs_is_invalid(unsigned int nb_segs) {
>>> +	uint16_t ring_size;
>>> +	uint16_t queue_id;
>>> +	uint16_t port_id;
>>> +	int ret;
>>> +
>>> +	RTE_ETH_FOREACH_DEV(port_id) {
>>> +		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>> +			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
>>> +
>>> +			if (ret)
>>> +				return true;
>>> +
>>> +			if (ring_size < nb_segs) {
>>> +				printf("nb segments per TX packets=%u >= "
>>> +				       "TX queue(%u) ring_size=%u - ignored\n",
>>> +				       nb_segs, queue_id, ring_size);
>>> +				return true;
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>>   void
>>>   set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
>>>   	uint16_t tx_pkt_len;
>>>   	unsigned i;
>>>
>>> -	if (nb_segs >= (unsigned) nb_txd) {
>>> -		printf("nb segments per TX packets=%u >= nb_txd=%u -
>>> ignored\n",
>>> -		       nb_segs, (unsigned int) nb_txd);
>>> +	if (nb_segs_is_invalid(nb_segs))
>>>   		return;
>>> -	}
>>>
>>>   	/*
>>>   	 * Check that each segment length is greater or equal than
>>> --
>>> 2.9.5
>>
>>
> 
> 
> 
> 
> 

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Kevin Traynor]

On 24/11/2020 12:23, Ferruh Yigit wrote:
> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
>> Is it OK to keep this regression?
>>
>> Ferruh, what do you suggest?
>>
> 
> I confirm the '--txpkts' parameter is broken now, I suggest submitting a defect 
> for it and continue with the regression.
> 
> We have alternative for the parameter, "set txpkts <len0[,len1]*>" command.
> The parameter was only working when hardcoded '--txd=N' parameter is provided, 
> the command is more dynamic and works however queue size is configured.
> 
> We can fix the '--txpkts' in next release.
> 

For LTS, I will revert from 18.11 branch as I don't want to add a
regression on last 18.11 LTS release.

+cc Luca for 19.11

>>
>> 23/11/2020 12:50, Slava Ovsiienko:
>>> Hi,  Wei
>>>
>>> It was found this patch rejects the --txpkts command line settings.
>>> set_tx_pkt_segments() is called before device started and
>>> we have failure chain:
>>>
>>> set_tx_pkt_segments()
>>>    nb_segs_is_invalid()
>>>      get_tx_ring_size ()
>>>       rte_eth_tx_queue_info_get()
>>>
>>> It causes --txpkts testpmd command line option is ignored.
>>>
>>> With best regards, Slava
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
>>>> Sent: Friday, September 25, 2020 15:47
>>>> To: dev@dpdk.org
>>>> Cc: xavier.huwei@huawei.com
>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts
>>>> set
>>>>
>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>
>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set because the
>>>> nb_txd is used to avoid the numer of segments exceed the Tx ring size and the
>>>> default value of nb_txd is 0. And there is a bug that nb_txd is the global
>>>> configuration for Tx ring size and the ring size could be changed by some
>>>> command per queue. So these valid check is unreliable and introduced
>>>> unnecessary constraints.
>>>>
>>>> This patch adds a valid check function to use the real Tx ring size to check the
>>>> validity of txpkts.
>>>>
>>>> Fixes: af75078fece3 ("first public release")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> ---
>>>> v3 -> v4:
>>>> 	 add check 'rte_eth_rx_queue_info_get()' return value and
>>>> 	 if it is '-ENOSTUP' calculate the 'ring_size'.
>>>> v3:      initial version.
>>>> ---
>>>>   app/test-pmd/config.c | 64
>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>   1 file changed, 60 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>> 6496d2f..8ebb927 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
>>>>
>>>>   static int
>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
>>>> +*ring_size) {
>>>> +	struct rte_port *port = &ports[port_id];
>>>> +	struct rte_eth_txq_info tx_qinfo;
>>>> +	int ret;
>>>> +
>>>> +	ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
>>>> +	if (ret == 0) {
>>>> +		*ring_size = tx_qinfo.nb_desc;
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	if (ret != -ENOTSUP)
>>>> +		return ret;
>>>> +	/*
>>>> +	 * If the rte_eth_tx_queue_info_get is not support for this PMD,
>>>> +	 * ring_size stored in testpmd will be used for validity verification.
>>>> +	 * When configure the txq by rte_eth_tx_queue_setup with
>>>> nb_tx_desc
>>>> +	 * being 0, it will use a default value provided by PMDs to setup this
>>>> +	 * txq. If the default value is 0, it will use the
>>>> +	 * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>> +	 */
>>>> +	if (port->nb_tx_desc[txq_id])
>>>> +		*ring_size = port->nb_tx_desc[txq_id];
>>>> +	else if (port->dev_info.default_txportconf.ring_size)
>>>> +		*ring_size = port->dev_info.default_txportconf.ring_size;
>>>> +	else
>>>> +		*ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int
>>>>   rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>>>>   	if (rxdesc_id < nb_rxd)
>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>>>>   	printf("Split packet: %s\n", split);
>>>>   }
>>>>
>>>> +static bool
>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
>>>> +	uint16_t ring_size;
>>>> +	uint16_t queue_id;
>>>> +	uint16_t port_id;
>>>> +	int ret;
>>>> +
>>>> +	RTE_ETH_FOREACH_DEV(port_id) {
>>>> +		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>> +			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
>>>> +
>>>> +			if (ret)
>>>> +				return true;
>>>> +
>>>> +			if (ring_size < nb_segs) {
>>>> +				printf("nb segments per TX packets=%u >= "
>>>> +				       "TX queue(%u) ring_size=%u - ignored\n",
>>>> +				       nb_segs, queue_id, ring_size);
>>>> +				return true;
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>> +
>>>>   void
>>>>   set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
>>>>   	uint16_t tx_pkt_len;
>>>>   	unsigned i;
>>>>
>>>> -	if (nb_segs >= (unsigned) nb_txd) {
>>>> -		printf("nb segments per TX packets=%u >= nb_txd=%u -
>>>> ignored\n",
>>>> -		       nb_segs, (unsigned int) nb_txd);
>>>> +	if (nb_segs_is_invalid(nb_segs))
>>>>   		return;
>>>> -	}
>>>>
>>>>   	/*
>>>>   	 * Check that each segment length is greater or equal than
>>>> --
>>>> 2.9.5
>>>
>>>
>>
>>
>>
>>
>>
> 

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
>> Is it OK to keep this regression?
>>
>> Ferruh, what do you suggest?
>>
> 
> I confirm the '--txpkts' parameter is broken now, I suggest submitting a defect 
> for it and continue with the regression.
> 

Hi Slava,

Can you please submit the Bugzilla defect?

Thanks,
ferruh


> We have alternative for the parameter, "set txpkts <len0[,len1]*>" command.
> The parameter was only working when hardcoded '--txd=N' parameter is provided, 
> the command is more dynamic and works however queue size is configured.
> 
> We can fix the '--txpkts' in next release.
> 
>>
>> 23/11/2020 12:50, Slava Ovsiienko:
>>> Hi,  Wei
>>>
>>> It was found this patch rejects the --txpkts command line settings.
>>> set_tx_pkt_segments() is called before device started and
>>> we have failure chain:
>>>
>>> set_tx_pkt_segments()
>>>    nb_segs_is_invalid()
>>>      get_tx_ring_size ()
>>>       rte_eth_tx_queue_info_get()
>>>
>>> It causes --txpkts testpmd command line option is ignored.
>>>
>>> With best regards, Slava
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
>>>> Sent: Friday, September 25, 2020 15:47
>>>> To: dev@dpdk.org
>>>> Cc: xavier.huwei@huawei.com
>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts
>>>> set
>>>>
>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>
>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be set because 
>>>> the
>>>> nb_txd is used to avoid the numer of segments exceed the Tx ring size and the
>>>> default value of nb_txd is 0. And there is a bug that nb_txd is the global
>>>> configuration for Tx ring size and the ring size could be changed by some
>>>> command per queue. So these valid check is unreliable and introduced
>>>> unnecessary constraints.
>>>>
>>>> This patch adds a valid check function to use the real Tx ring size to check 
>>>> the
>>>> validity of txpkts.
>>>>
>>>> Fixes: af75078fece3 ("first public release")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>> ---
>>>> v3 -> v4:
>>>>      add check 'rte_eth_rx_queue_info_get()' return value and
>>>>      if it is '-ENOSTUP' calculate the 'ring_size'.
>>>> v3:      initial version.
>>>> ---
>>>>   app/test-pmd/config.c | 64
>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>   1 file changed, 60 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>> 6496d2f..8ebb927 100644
>>>> --- a/app/test-pmd/config.c
>>>> +++ b/app/test-pmd/config.c
>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
>>>>
>>>>   static int
>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
>>>> +*ring_size) {
>>>> +    struct rte_port *port = &ports[port_id];
>>>> +    struct rte_eth_txq_info tx_qinfo;
>>>> +    int ret;
>>>> +
>>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
>>>> +    if (ret == 0) {
>>>> +        *ring_size = tx_qinfo.nb_desc;
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    if (ret != -ENOTSUP)
>>>> +        return ret;
>>>> +    /*
>>>> +     * If the rte_eth_tx_queue_info_get is not support for this PMD,
>>>> +     * ring_size stored in testpmd will be used for validity verification.
>>>> +     * When configure the txq by rte_eth_tx_queue_setup with
>>>> nb_tx_desc
>>>> +     * being 0, it will use a default value provided by PMDs to setup this
>>>> +     * txq. If the default value is 0, it will use the
>>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>> +     */
>>>> +    if (port->nb_tx_desc[txq_id])
>>>> +        *ring_size = port->nb_tx_desc[txq_id];
>>>> +    else if (port->dev_info.default_txportconf.ring_size)
>>>> +        *ring_size = port->dev_info.default_txportconf.ring_size;
>>>> +    else
>>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int
>>>>   rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>>>>       if (rxdesc_id < nb_rxd)
>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>>>>       printf("Split packet: %s\n", split);
>>>>   }
>>>>
>>>> +static bool
>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
>>>> +    uint16_t ring_size;
>>>> +    uint16_t queue_id;
>>>> +    uint16_t port_id;
>>>> +    int ret;
>>>> +
>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>> +            ret = get_tx_ring_size(port_id, queue_id, &ring_size);
>>>> +
>>>> +            if (ret)
>>>> +                return true;
>>>> +
>>>> +            if (ring_size < nb_segs) {
>>>> +                printf("nb segments per TX packets=%u >= "
>>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
>>>> +                       nb_segs, queue_id, ring_size);
>>>> +                return true;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return false;
>>>> +}
>>>> +
>>>>   void
>>>>   set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
>>>>       uint16_t tx_pkt_len;
>>>>       unsigned i;
>>>>
>>>> -    if (nb_segs >= (unsigned) nb_txd) {
>>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
>>>> ignored\n",
>>>> -               nb_segs, (unsigned int) nb_txd);
>>>> +    if (nb_segs_is_invalid(nb_segs))
>>>>           return;
>>>> -    }
>>>>
>>>>       /*
>>>>        * Check that each segment length is greater or equal than
>>>> -- 
>>>> 2.9.5
>>>
>>>
>>
>>
>>
>>
>>
> 

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Slava Ovsiienko]

The bug: https://bugs.dpdk.org/show_bug.cgi?id=584

Can we pass the nb_segs = 1 always? 
One descriptor is minimal basic capability to send, it should be always supported.

With best regards, Slava

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, November 25, 2020 16:07
> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
> <huwei013@chinasoftinc.com>
> Cc: dev@dpdk.org; xavier.huwei@huawei.com; Slava Ovsiienko
> <viacheslavo@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on
> txpkts set
> 
> On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
> > On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
> >> Is it OK to keep this regression?
> >>
> >> Ferruh, what do you suggest?
> >>
> >
> > I confirm the '--txpkts' parameter is broken now, I suggest submitting
> > a defect for it and continue with the regression.
> >
> 
> Hi Slava,
> 
> Can you please submit the Bugzilla defect?
> 
> Thanks,
> ferruh
> 
> 
> > We have alternative for the parameter, "set txpkts <len0[,len1]*>" command.
> > The parameter was only working when hardcoded '--txd=N' parameter is
> > provided, the command is more dynamic and works however queue size is
> configured.
> >
> > We can fix the '--txpkts' in next release.
> >
> >>
> >> 23/11/2020 12:50, Slava Ovsiienko:
> >>> Hi,  Wei
> >>>
> >>> It was found this patch rejects the --txpkts command line settings.
> >>> set_tx_pkt_segments() is called before device started and we have
> >>> failure chain:
> >>>
> >>> set_tx_pkt_segments()
> >>>    nb_segs_is_invalid()
> >>>      get_tx_ring_size ()
> >>>       rte_eth_tx_queue_info_get()
> >>>
> >>> It causes --txpkts testpmd command line option is ignored.
> >>>
> >>> With best regards, Slava
> >>>
> >>>> -----Original Message-----
> >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
> >>>> Sent: Friday, September 25, 2020 15:47
> >>>> To: dev@dpdk.org
> >>>> Cc: xavier.huwei@huawei.com
> >>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction
> >>>> on txpkts set
> >>>>
> >>>> From: Chengchang Tang <tangchengchang@huawei.com>
> >>>>
> >>>> Currently, if nb_txd is not set, the txpkts is not allowed to be
> >>>> set because the nb_txd is used to avoid the numer of segments
> >>>> exceed the Tx ring size and the default value of nb_txd is 0. And
> >>>> there is a bug that nb_txd is the global configuration for Tx ring
> >>>> size and the ring size could be changed by some command per queue.
> >>>> So these valid check is unreliable and introduced unnecessary
> >>>> constraints.
> >>>>
> >>>> This patch adds a valid check function to use the real Tx ring size
> >>>> to check the validity of txpkts.
> >>>>
> >>>> Fixes: af75078fece3 ("first public release")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>> ---
> >>>> v3 -> v4:
> >>>>      add check 'rte_eth_rx_queue_info_get()' return value and
> >>>>      if it is '-ENOSTUP' calculate the 'ring_size'.
> >>>> v3:      initial version.
> >>>> ---
> >>>>   app/test-pmd/config.c | 64
> >>>> +++++++++++++++++++++++++++++++++++++++++++++++----
> >>>>   1 file changed, 60 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>> 6496d2f..8ebb927 100644
> >>>> --- a/app/test-pmd/config.c
> >>>> +++ b/app/test-pmd/config.c
> >>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
> >>>>
> >>>>   static int
> >>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
> >>>> +*ring_size) {
> >>>> +    struct rte_port *port = &ports[port_id];
> >>>> +    struct rte_eth_txq_info tx_qinfo;
> >>>> +    int ret;
> >>>> +
> >>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
> >>>> +    if (ret == 0) {
> >>>> +        *ring_size = tx_qinfo.nb_desc;
> >>>> +        return ret;
> >>>> +    }
> >>>> +
> >>>> +    if (ret != -ENOTSUP)
> >>>> +        return ret;
> >>>> +    /*
> >>>> +     * If the rte_eth_tx_queue_info_get is not support for this
> >>>> +PMD,
> >>>> +     * ring_size stored in testpmd will be used for validity verification.
> >>>> +     * When configure the txq by rte_eth_tx_queue_setup with
> >>>> nb_tx_desc
> >>>> +     * being 0, it will use a default value provided by PMDs to
> >>>> +setup this
> >>>> +     * txq. If the default value is 0, it will use the
> >>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> >>>> +     */
> >>>> +    if (port->nb_tx_desc[txq_id])
> >>>> +        *ring_size = port->nb_tx_desc[txq_id];
> >>>> +    else if (port->dev_info.default_txportconf.ring_size)
> >>>> +        *ring_size = port->dev_info.default_txportconf.ring_size;
> >>>> +    else
> >>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> >>>> +    return 0;
> >>>> +}
> >>>> +
> >>>> +static int
> >>>>   rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
> >>>>       if (rxdesc_id < nb_rxd)
> >>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
> >>>>       printf("Split packet: %s\n", split);
> >>>>   }
> >>>>
> >>>> +static bool
> >>>> +nb_segs_is_invalid(unsigned int nb_segs) {
> >>>> +    uint16_t ring_size;
> >>>> +    uint16_t queue_id;
> >>>> +    uint16_t port_id;
> >>>> +    int ret;
> >>>> +
> >>>> +    RTE_ETH_FOREACH_DEV(port_id) {
> >>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> >>>> +            ret = get_tx_ring_size(port_id, queue_id, &ring_size);
> >>>> +
> >>>> +            if (ret)
> >>>> +                return true;
> >>>> +
> >>>> +            if (ring_size < nb_segs) {
> >>>> +                printf("nb segments per TX packets=%u >= "
> >>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
> >>>> +                       nb_segs, queue_id, ring_size);
> >>>> +                return true;
> >>>> +            }
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    return false;
> >>>> +}
> >>>> +
> >>>>   void
> >>>>   set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
> >>>>       uint16_t tx_pkt_len;
> >>>>       unsigned i;
> >>>>
> >>>> -    if (nb_segs >= (unsigned) nb_txd) {
> >>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
> >>>> ignored\n",
> >>>> -               nb_segs, (unsigned int) nb_txd);
> >>>> +    if (nb_segs_is_invalid(nb_segs))
> >>>>           return;
> >>>> -    }
> >>>>
> >>>>       /*
> >>>>        * Check that each segment length is greater or equal than
> >>>> --
> >>>> 2.9.5
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 11/26/2020 7:24 AM, Slava Ovsiienko wrote:
> The bug: https://bugs.dpdk.org/show_bug.cgi?id=584
> 
> Can we pass the nb_segs = 1 always?
> One descriptor is minimal basic capability to send, it should be always supported.
> 

Hi Slava,

I didn't get your comment, can you please elaborate?


> With best regards, Slava
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, November 25, 2020 16:07
>> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
>> <huwei013@chinasoftinc.com>
>> Cc: dev@dpdk.org; xavier.huwei@huawei.com; Slava Ovsiienko
>> <viacheslavo@nvidia.com>
>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on
>> txpkts set
>>
>> On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
>>> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
>>>> Is it OK to keep this regression?
>>>>
>>>> Ferruh, what do you suggest?
>>>>
>>>
>>> I confirm the '--txpkts' parameter is broken now, I suggest submitting
>>> a defect for it and continue with the regression.
>>>
>>
>> Hi Slava,
>>
>> Can you please submit the Bugzilla defect?
>>
>> Thanks,
>> ferruh
>>
>>
>>> We have alternative for the parameter, "set txpkts <len0[,len1]*>" command.
>>> The parameter was only working when hardcoded '--txd=N' parameter is
>>> provided, the command is more dynamic and works however queue size is
>> configured.
>>>
>>> We can fix the '--txpkts' in next release.
>>>
>>>>
>>>> 23/11/2020 12:50, Slava Ovsiienko:
>>>>> Hi,  Wei
>>>>>
>>>>> It was found this patch rejects the --txpkts command line settings.
>>>>> set_tx_pkt_segments() is called before device started and we have
>>>>> failure chain:
>>>>>
>>>>> set_tx_pkt_segments()
>>>>>     nb_segs_is_invalid()
>>>>>       get_tx_ring_size ()
>>>>>        rte_eth_tx_queue_info_get()
>>>>>
>>>>> It causes --txpkts testpmd command line option is ignored.
>>>>>
>>>>> With best regards, Slava
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
>>>>>> Sent: Friday, September 25, 2020 15:47
>>>>>> To: dev@dpdk.org
>>>>>> Cc: xavier.huwei@huawei.com
>>>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction
>>>>>> on txpkts set
>>>>>>
>>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>
>>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be
>>>>>> set because the nb_txd is used to avoid the numer of segments
>>>>>> exceed the Tx ring size and the default value of nb_txd is 0. And
>>>>>> there is a bug that nb_txd is the global configuration for Tx ring
>>>>>> size and the ring size could be changed by some command per queue.
>>>>>> So these valid check is unreliable and introduced unnecessary
>>>>>> constraints.
>>>>>>
>>>>>> This patch adds a valid check function to use the real Tx ring size
>>>>>> to check the validity of txpkts.
>>>>>>
>>>>>> Fixes: af75078fece3 ("first public release")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>> ---
>>>>>> v3 -> v4:
>>>>>>       add check 'rte_eth_rx_queue_info_get()' return value and
>>>>>>       if it is '-ENOSTUP' calculate the 'ring_size'.
>>>>>> v3:      initial version.
>>>>>> ---
>>>>>>    app/test-pmd/config.c | 64
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>    1 file changed, 60 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>>> 6496d2f..8ebb927 100644
>>>>>> --- a/app/test-pmd/config.c
>>>>>> +++ b/app/test-pmd/config.c
>>>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)  }
>>>>>>
>>>>>>    static int
>>>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
>>>>>> +*ring_size) {
>>>>>> +    struct rte_port *port = &ports[port_id];
>>>>>> +    struct rte_eth_txq_info tx_qinfo;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
>>>>>> +    if (ret == 0) {
>>>>>> +        *ring_size = tx_qinfo.nb_desc;
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (ret != -ENOTSUP)
>>>>>> +        return ret;
>>>>>> +    /*
>>>>>> +     * If the rte_eth_tx_queue_info_get is not support for this
>>>>>> +PMD,
>>>>>> +     * ring_size stored in testpmd will be used for validity verification.
>>>>>> +     * When configure the txq by rte_eth_tx_queue_setup with
>>>>>> nb_tx_desc
>>>>>> +     * being 0, it will use a default value provided by PMDs to
>>>>>> +setup this
>>>>>> +     * txq. If the default value is 0, it will use the
>>>>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>>>> +     */
>>>>>> +    if (port->nb_tx_desc[txq_id])
>>>>>> +        *ring_size = port->nb_tx_desc[txq_id];
>>>>>> +    else if (port->dev_info.default_txportconf.ring_size)
>>>>>> +        *ring_size = port->dev_info.default_txportconf.ring_size;
>>>>>> +    else
>>>>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int
>>>>>>    rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>>>>>>        if (rxdesc_id < nb_rxd)
>>>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>>>>>>        printf("Split packet: %s\n", split);
>>>>>>    }
>>>>>>
>>>>>> +static bool
>>>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
>>>>>> +    uint16_t ring_size;
>>>>>> +    uint16_t queue_id;
>>>>>> +    uint16_t port_id;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>>>> +            ret = get_tx_ring_size(port_id, queue_id, &ring_size);
>>>>>> +
>>>>>> +            if (ret)
>>>>>> +                return true;
>>>>>> +
>>>>>> +            if (ring_size < nb_segs) {
>>>>>> +                printf("nb segments per TX packets=%u >= "
>>>>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
>>>>>> +                       nb_segs, queue_id, ring_size);
>>>>>> +                return true;
>>>>>> +            }
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    return false;
>>>>>> +}
>>>>>> +
>>>>>>    void
>>>>>>    set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)  {
>>>>>>        uint16_t tx_pkt_len;
>>>>>>        unsigned i;
>>>>>>
>>>>>> -    if (nb_segs >= (unsigned) nb_txd) {
>>>>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
>>>>>> ignored\n",
>>>>>> -               nb_segs, (unsigned int) nb_txd);
>>>>>> +    if (nb_segs_is_invalid(nb_segs))
>>>>>>            return;
>>>>>> -    }
>>>>>>
>>>>>>        /*
>>>>>>         * Check that each segment length is greater or equal than
>>>>>> --
>>>>>> 2.9.5
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
> 

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Slava Ovsiienko]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, November 26, 2020 14:38
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
> <huwei013@chinasoftinc.com>
> Cc: dev@dpdk.org; xavier.huwei@huawei.com
> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on
> txpkts set
> 
> On 11/26/2020 7:24 AM, Slava Ovsiienko wrote:
> > The bug:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
> >
> .dpdk.org%2Fshow_bug.cgi%3Fid%3D584&amp;data=04%7C01%7Cviacheslavo
> %40n
> >
> vidia.com%7Ce52ba5bbab184ac8592808d8920842c5%7C43083d15727340c1b7
> db39e
> >
> fd9ccc17a%7C0%7C0%7C637419911462011700%7CUnknown%7CTWFpbGZsb3
> d8eyJWIjo
> >
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000
> &amp
> >
> ;sdata=QBB67WqEjUHgwqHNjqx2VLdaTRMzMeodh%2B%2FVFsHByQg%3D&am
> p;reserved
> > =0
> >
> > Can we pass the nb_segs = 1 always?
> > One descriptor is minimal basic capability to send, it should be always
> supported.
> >
> 
> Hi Slava,
> 
> I didn't get your comment, can you please elaborate?
> 
The --txpkts is rejected on testpmd startup due to port is not configured yet
and we can't find out how many descriptors are actually configured
in the Tx queues.

Configuring Tx queues with zero descriptors seems to be meaningless,
it would disable a basic capability to send the packets. And we could assume
the single segment packet sending is always supported.

If --txpkts sets only the size for the single segment we can assume that the
packets with only one segment is going to be sent, and we could ignore
the Tx queue descriptor number check for the case.

With best regards, Slava


> 
> > With best regards, Slava
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Wednesday, November 25, 2020 16:07
> >> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Wei Hu
> >> (Xavier) <huwei013@chinasoftinc.com>
> >> Cc: dev@dpdk.org; xavier.huwei@huawei.com; Slava Ovsiienko
> >> <viacheslavo@nvidia.com>
> >> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
> >> restriction on txpkts set
> >>
> >> On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
> >>> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
> >>>> Is it OK to keep this regression?
> >>>>
> >>>> Ferruh, what do you suggest?
> >>>>
> >>>
> >>> I confirm the '--txpkts' parameter is broken now, I suggest
> >>> submitting a defect for it and continue with the regression.
> >>>
> >>
> >> Hi Slava,
> >>
> >> Can you please submit the Bugzilla defect?
> >>
> >> Thanks,
> >> ferruh
> >>
> >>
> >>> We have alternative for the parameter, "set txpkts <len0[,len1]*>"
> command.
> >>> The parameter was only working when hardcoded '--txd=N' parameter is
> >>> provided, the command is more dynamic and works however queue size
> >>> is
> >> configured.
> >>>
> >>> We can fix the '--txpkts' in next release.
> >>>
> >>>>
> >>>> 23/11/2020 12:50, Slava Ovsiienko:
> >>>>> Hi,  Wei
> >>>>>
> >>>>> It was found this patch rejects the --txpkts command line settings.
> >>>>> set_tx_pkt_segments() is called before device started and we have
> >>>>> failure chain:
> >>>>>
> >>>>> set_tx_pkt_segments()
> >>>>>     nb_segs_is_invalid()
> >>>>>       get_tx_ring_size ()
> >>>>>        rte_eth_tx_queue_info_get()
> >>>>>
> >>>>> It causes --txpkts testpmd command line option is ignored.
> >>>>>
> >>>>> With best regards, Slava
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
> >>>>>> Sent: Friday, September 25, 2020 15:47
> >>>>>> To: dev@dpdk.org
> >>>>>> Cc: xavier.huwei@huawei.com
> >>>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
> >>>>>> restriction on txpkts set
> >>>>>>
> >>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
> >>>>>>
> >>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be
> >>>>>> set because the nb_txd is used to avoid the numer of segments
> >>>>>> exceed the Tx ring size and the default value of nb_txd is 0. And
> >>>>>> there is a bug that nb_txd is the global configuration for Tx
> >>>>>> ring size and the ring size could be changed by some command per
> queue.
> >>>>>> So these valid check is unreliable and introduced unnecessary
> >>>>>> constraints.
> >>>>>>
> >>>>>> This patch adds a valid check function to use the real Tx ring
> >>>>>> size to check the validity of txpkts.
> >>>>>>
> >>>>>> Fixes: af75078fece3 ("first public release")
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>>>> ---
> >>>>>> v3 -> v4:
> >>>>>>       add check 'rte_eth_rx_queue_info_get()' return value and
> >>>>>>       if it is '-ENOSTUP' calculate the 'ring_size'.
> >>>>>> v3:      initial version.
> >>>>>> ---
> >>>>>>    app/test-pmd/config.c | 64
> >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
> >>>>>>    1 file changed, 60 insertions(+), 4 deletions(-)
> >>>>>>
> >>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>>>> 6496d2f..8ebb927 100644
> >>>>>> --- a/app/test-pmd/config.c
> >>>>>> +++ b/app/test-pmd/config.c
> >>>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)
> >>>>>> }
> >>>>>>
> >>>>>>    static int
> >>>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
> >>>>>> +*ring_size) {
> >>>>>> +    struct rte_port *port = &ports[port_id];
> >>>>>> +    struct rte_eth_txq_info tx_qinfo;
> >>>>>> +    int ret;
> >>>>>> +
> >>>>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
> >>>>>> +    if (ret == 0) {
> >>>>>> +        *ring_size = tx_qinfo.nb_desc;
> >>>>>> +        return ret;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    if (ret != -ENOTSUP)
> >>>>>> +        return ret;
> >>>>>> +    /*
> >>>>>> +     * If the rte_eth_tx_queue_info_get is not support for this
> >>>>>> +PMD,
> >>>>>> +     * ring_size stored in testpmd will be used for validity verification.
> >>>>>> +     * When configure the txq by rte_eth_tx_queue_setup with
> >>>>>> nb_tx_desc
> >>>>>> +     * being 0, it will use a default value provided by PMDs to
> >>>>>> +setup this
> >>>>>> +     * txq. If the default value is 0, it will use the
> >>>>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> >>>>>> +     */
> >>>>>> +    if (port->nb_tx_desc[txq_id])
> >>>>>> +        *ring_size = port->nb_tx_desc[txq_id];
> >>>>>> +    else if (port->dev_info.default_txportconf.ring_size)
> >>>>>> +        *ring_size =
> >>>>>> +port->dev_info.default_txportconf.ring_size;
> >>>>>> +    else
> >>>>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> >>>>>> +    return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int
> >>>>>>    rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
> >>>>>>        if (rxdesc_id < nb_rxd)
> >>>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
> >>>>>>        printf("Split packet: %s\n", split);
> >>>>>>    }
> >>>>>>
> >>>>>> +static bool
> >>>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
> >>>>>> +    uint16_t ring_size;
> >>>>>> +    uint16_t queue_id;
> >>>>>> +    uint16_t port_id;
> >>>>>> +    int ret;
> >>>>>> +
> >>>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
> >>>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> >>>>>> +            ret = get_tx_ring_size(port_id, queue_id,
> >>>>>> +&ring_size);
> >>>>>> +
> >>>>>> +            if (ret)
> >>>>>> +                return true;
> >>>>>> +
> >>>>>> +            if (ring_size < nb_segs) {
> >>>>>> +                printf("nb segments per TX packets=%u >= "
> >>>>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
> >>>>>> +                       nb_segs, queue_id, ring_size);
> >>>>>> +                return true;
> >>>>>> +            }
> >>>>>> +        }
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    return false;
> >>>>>> +}
> >>>>>> +
> >>>>>>    void
> >>>>>>    set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
> >>>>>> {
> >>>>>>        uint16_t tx_pkt_len;
> >>>>>>        unsigned i;
> >>>>>>
> >>>>>> -    if (nb_segs >= (unsigned) nb_txd) {
> >>>>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
> >>>>>> ignored\n",
> >>>>>> -               nb_segs, (unsigned int) nb_txd);
> >>>>>> +    if (nb_segs_is_invalid(nb_segs))
> >>>>>>            return;
> >>>>>> -    }
> >>>>>>
> >>>>>>        /*
> >>>>>>         * Check that each segment length is greater or equal than
> >>>>>> --
> >>>>>> 2.9.5
> >>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 11/27/2020 1:05 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, November 26, 2020 14:38
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
>> <huwei013@chinasoftinc.com>
>> Cc: dev@dpdk.org; xavier.huwei@huawei.com
>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on
>> txpkts set
>>
>> On 11/26/2020 7:24 AM, Slava Ovsiienko wrote:
>>> The bug:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs
>>>
>> .dpdk.org%2Fshow_bug.cgi%3Fid%3D584&amp;data=04%7C01%7Cviacheslavo
>> %40n
>>>
>> vidia.com%7Ce52ba5bbab184ac8592808d8920842c5%7C43083d15727340c1b7
>> db39e
>>>
>> fd9ccc17a%7C0%7C0%7C637419911462011700%7CUnknown%7CTWFpbGZsb3
>> d8eyJWIjo
>>>
>> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000
>> &amp
>>>
>> ;sdata=QBB67WqEjUHgwqHNjqx2VLdaTRMzMeodh%2B%2FVFsHByQg%3D&am
>> p;reserved
>>> =0
>>>
>>> Can we pass the nb_segs = 1 always?
>>> One descriptor is minimal basic capability to send, it should be always
>> supported.
>>>
>>
>> Hi Slava,
>>
>> I didn't get your comment, can you please elaborate?
>>
> The --txpkts is rejected on testpmd startup due to port is not configured yet
> and we can't find out how many descriptors are actually configured
> in the Tx queues.
> 
> Configuring Tx queues with zero descriptors seems to be meaningless,
> it would disable a basic capability to send the packets. And we could assume
> the single segment packet sending is always supported.
> 
> If --txpkts sets only the size for the single segment we can assume that the
> packets with only one segment is going to be sent, and we could ignore
> the Tx queue descriptor number check for the case.
> 

Overall I was OK to remove the check completely, even multi segment used it is 
very unlikely that number of segments will be more than descriptor size.

But at least OK to ignore the check for single segment, also we can force 
'--txd' parameter provided to enable '--txpkts', like done before.


> With best regards, Slava
> 
> 
>>
>>> With best regards, Slava
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Wednesday, November 25, 2020 16:07
>>>> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Wei Hu
>>>> (Xavier) <huwei013@chinasoftinc.com>
>>>> Cc: dev@dpdk.org; xavier.huwei@huawei.com; Slava Ovsiienko
>>>> <viacheslavo@nvidia.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
>>>> restriction on txpkts set
>>>>
>>>> On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
>>>>> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
>>>>>> Is it OK to keep this regression?
>>>>>>
>>>>>> Ferruh, what do you suggest?
>>>>>>
>>>>>
>>>>> I confirm the '--txpkts' parameter is broken now, I suggest
>>>>> submitting a defect for it and continue with the regression.
>>>>>
>>>>
>>>> Hi Slava,
>>>>
>>>> Can you please submit the Bugzilla defect?
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>>>
>>>>> We have alternative for the parameter, "set txpkts <len0[,len1]*>"
>> command.
>>>>> The parameter was only working when hardcoded '--txd=N' parameter is
>>>>> provided, the command is more dynamic and works however queue size
>>>>> is
>>>> configured.
>>>>>
>>>>> We can fix the '--txpkts' in next release.
>>>>>
>>>>>>
>>>>>> 23/11/2020 12:50, Slava Ovsiienko:
>>>>>>> Hi,  Wei
>>>>>>>
>>>>>>> It was found this patch rejects the --txpkts command line settings.
>>>>>>> set_tx_pkt_segments() is called before device started and we have
>>>>>>> failure chain:
>>>>>>>
>>>>>>> set_tx_pkt_segments()
>>>>>>>      nb_segs_is_invalid()
>>>>>>>        get_tx_ring_size ()
>>>>>>>         rte_eth_tx_queue_info_get()
>>>>>>>
>>>>>>> It causes --txpkts testpmd command line option is ignored.
>>>>>>>
>>>>>>> With best regards, Slava
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
>>>>>>>> Sent: Friday, September 25, 2020 15:47
>>>>>>>> To: dev@dpdk.org
>>>>>>>> Cc: xavier.huwei@huawei.com
>>>>>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
>>>>>>>> restriction on txpkts set
>>>>>>>>
>>>>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>>>
>>>>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be
>>>>>>>> set because the nb_txd is used to avoid the numer of segments
>>>>>>>> exceed the Tx ring size and the default value of nb_txd is 0. And
>>>>>>>> there is a bug that nb_txd is the global configuration for Tx
>>>>>>>> ring size and the ring size could be changed by some command per
>> queue.
>>>>>>>> So these valid check is unreliable and introduced unnecessary
>>>>>>>> constraints.
>>>>>>>>
>>>>>>>> This patch adds a valid check function to use the real Tx ring
>>>>>>>> size to check the validity of txpkts.
>>>>>>>>
>>>>>>>> Fixes: af75078fece3 ("first public release")
>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>
>>>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>>>> ---
>>>>>>>> v3 -> v4:
>>>>>>>>        add check 'rte_eth_rx_queue_info_get()' return value and
>>>>>>>>        if it is '-ENOSTUP' calculate the 'ring_size'.
>>>>>>>> v3:      initial version.
>>>>>>>> ---
>>>>>>>>     app/test-pmd/config.c | 64
>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>>>     1 file changed, 60 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>>>>> 6496d2f..8ebb927 100644
>>>>>>>> --- a/app/test-pmd/config.c
>>>>>>>> +++ b/app/test-pmd/config.c
>>>>>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t txq_id)
>>>>>>>> }
>>>>>>>>
>>>>>>>>     static int
>>>>>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
>>>>>>>> +*ring_size) {
>>>>>>>> +    struct rte_port *port = &ports[port_id];
>>>>>>>> +    struct rte_eth_txq_info tx_qinfo;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
>>>>>>>> +    if (ret == 0) {
>>>>>>>> +        *ring_size = tx_qinfo.nb_desc;
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (ret != -ENOTSUP)
>>>>>>>> +        return ret;
>>>>>>>> +    /*
>>>>>>>> +     * If the rte_eth_tx_queue_info_get is not support for this
>>>>>>>> +PMD,
>>>>>>>> +     * ring_size stored in testpmd will be used for validity verification.
>>>>>>>> +     * When configure the txq by rte_eth_tx_queue_setup with
>>>>>>>> nb_tx_desc
>>>>>>>> +     * being 0, it will use a default value provided by PMDs to
>>>>>>>> +setup this
>>>>>>>> +     * txq. If the default value is 0, it will use the
>>>>>>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>>>>>> +     */
>>>>>>>> +    if (port->nb_tx_desc[txq_id])
>>>>>>>> +        *ring_size = port->nb_tx_desc[txq_id];
>>>>>>>> +    else if (port->dev_info.default_txportconf.ring_size)
>>>>>>>> +        *ring_size =
>>>>>>>> +port->dev_info.default_txportconf.ring_size;
>>>>>>>> +    else
>>>>>>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int
>>>>>>>>     rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>>>>>>>>         if (rxdesc_id < nb_rxd)
>>>>>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>>>>>>>>         printf("Split packet: %s\n", split);
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +static bool
>>>>>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
>>>>>>>> +    uint16_t ring_size;
>>>>>>>> +    uint16_t queue_id;
>>>>>>>> +    uint16_t port_id;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>>>>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>>>>>> +            ret = get_tx_ring_size(port_id, queue_id,
>>>>>>>> +&ring_size);
>>>>>>>> +
>>>>>>>> +            if (ret)
>>>>>>>> +                return true;
>>>>>>>> +
>>>>>>>> +            if (ring_size < nb_segs) {
>>>>>>>> +                printf("nb segments per TX packets=%u >= "
>>>>>>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
>>>>>>>> +                       nb_segs, queue_id, ring_size);
>>>>>>>> +                return true;
>>>>>>>> +            }
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return false;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>     void
>>>>>>>>     set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
>>>>>>>> {
>>>>>>>>         uint16_t tx_pkt_len;
>>>>>>>>         unsigned i;
>>>>>>>>
>>>>>>>> -    if (nb_segs >= (unsigned) nb_txd) {
>>>>>>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
>>>>>>>> ignored\n",
>>>>>>>> -               nb_segs, (unsigned int) nb_txd);
>>>>>>>> +    if (nb_segs_is_invalid(nb_segs))
>>>>>>>>             return;
>>>>>>>> -    }
>>>>>>>>
>>>>>>>>         /*
>>>>>>>>          * Check that each segment length is greater or equal than
>>>>>>>> --
>>>>>>>> 2.9.5
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
> 

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Slava Ovsiienko]

Hi, Ferruh

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, December 2, 2020 14:07
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
> <huwei013@chinasoftinc.com>
> Cc: dev@dpdk.org; xavier.huwei@huawei.com
> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on
> txpkts set
> 
> On 11/27/2020 1:05 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Thursday, November 26, 2020 14:38
> >> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
> >> Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
> >> <huwei013@chinasoftinc.com>
> >> Cc: dev@dpdk.org; xavier.huwei@huawei.com
> >> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
> >> restriction on txpkts set
> >>
> >> On 11/26/2020 7:24 AM, Slava Ovsiienko wrote:
> >>> The bug:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbu
> >>> gs
> >>>
> >>
> .dpdk.org%2Fshow_bug.cgi%3Fid%3D584&amp;data=04%7C01%7Cviacheslavo
> >> %40n
> >>>
> >>
> vidia.com%7Ce52ba5bbab184ac8592808d8920842c5%7C43083d15727340c1b7
> >> db39e
> >>>
> >>
> fd9ccc17a%7C0%7C0%7C637419911462011700%7CUnknown%7CTWFpbGZsb3
> >> d8eyJWIjo
> >>>
> >>
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000
> >> &amp
> >>>
> >>
> ;sdata=QBB67WqEjUHgwqHNjqx2VLdaTRMzMeodh%2B%2FVFsHByQg%3D&am
> >> p;reserved
> >>> =0
> >>>
> >>> Can we pass the nb_segs = 1 always?
> >>> One descriptor is minimal basic capability to send, it should be
> >>> always
> >> supported.
> >>>
> >>
> >> Hi Slava,
> >>
> >> I didn't get your comment, can you please elaborate?
> >>
> > The --txpkts is rejected on testpmd startup due to port is not
> > configured yet and we can't find out how many descriptors are actually
> > configured in the Tx queues.
> >
> > Configuring Tx queues with zero descriptors seems to be meaningless,
> > it would disable a basic capability to send the packets. And we could
> > assume the single segment packet sending is always supported.
> >
> > If --txpkts sets only the size for the single segment we can assume
> > that the packets with only one segment is going to be sent, and we
> > could ignore the Tx queue descriptor number check for the case.
> >
> 
> Overall I was OK to remove the check completely, even multi segment used it
> is very unlikely that number of segments will be more than descriptor size.
> 
> But at least OK to ignore the check for single segment, also we can force '--txd'
> parameter provided to enable '--txpkts', like done before.

OK, I'll provide the patch taking both approaches on testpmd startup:
- if --txd is specified the check will be done against it,  failed check for non-configured port will be ignored
- if there is the only one segment specified in txpkts,  failed check for non-configured port will be ignored

With best regards, Slava

> >
> >
> >>
> >>> With best regards, Slava
> >>>
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> Sent: Wednesday, November 25, 2020 16:07
> >>>> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Wei Hu
> >>>> (Xavier) <huwei013@chinasoftinc.com>
> >>>> Cc: dev@dpdk.org; xavier.huwei@huawei.com; Slava Ovsiienko
> >>>> <viacheslavo@nvidia.com>
> >>>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
> >>>> restriction on txpkts set
> >>>>
> >>>> On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
> >>>>> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
> >>>>>> Is it OK to keep this regression?
> >>>>>>
> >>>>>> Ferruh, what do you suggest?
> >>>>>>
> >>>>>
> >>>>> I confirm the '--txpkts' parameter is broken now, I suggest
> >>>>> submitting a defect for it and continue with the regression.
> >>>>>
> >>>>
> >>>> Hi Slava,
> >>>>
> >>>> Can you please submit the Bugzilla defect?
> >>>>
> >>>> Thanks,
> >>>> ferruh
> >>>>
> >>>>
> >>>>> We have alternative for the parameter, "set txpkts <len0[,len1]*>"
> >> command.
> >>>>> The parameter was only working when hardcoded '--txd=N' parameter is
> >>>>> provided, the command is more dynamic and works however queue size
> >>>>> is
> >>>> configured.
> >>>>>
> >>>>> We can fix the '--txpkts' in next release.
> >>>>>
> >>>>>>
> >>>>>> 23/11/2020 12:50, Slava Ovsiienko:
> >>>>>>> Hi,  Wei
> >>>>>>>
> >>>>>>> It was found this patch rejects the --txpkts command line settings.
> >>>>>>> set_tx_pkt_segments() is called before device started and we have
> >>>>>>> failure chain:
> >>>>>>>
> >>>>>>> set_tx_pkt_segments()
> >>>>>>>      nb_segs_is_invalid()
> >>>>>>>        get_tx_ring_size ()
> >>>>>>>         rte_eth_tx_queue_info_get()
> >>>>>>>
> >>>>>>> It causes --txpkts testpmd command line option is ignored.
> >>>>>>>
> >>>>>>> With best regards, Slava
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
> >>>>>>>> Sent: Friday, September 25, 2020 15:47
> >>>>>>>> To: dev@dpdk.org
> >>>>>>>> Cc: xavier.huwei@huawei.com
> >>>>>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
> >>>>>>>> restriction on txpkts set
> >>>>>>>>
> >>>>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
> >>>>>>>>
> >>>>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be
> >>>>>>>> set because the nb_txd is used to avoid the numer of segments
> >>>>>>>> exceed the Tx ring size and the default value of nb_txd is 0. And
> >>>>>>>> there is a bug that nb_txd is the global configuration for Tx
> >>>>>>>> ring size and the ring size could be changed by some command per
> >> queue.
> >>>>>>>> So these valid check is unreliable and introduced unnecessary
> >>>>>>>> constraints.
> >>>>>>>>
> >>>>>>>> This patch adds a valid check function to use the real Tx ring
> >>>>>>>> size to check the validity of txpkts.
> >>>>>>>>
> >>>>>>>> Fixes: af75078fece3 ("first public release")
> >>>>>>>> Cc: stable@dpdk.org
> >>>>>>>>
> >>>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> >>>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> >>>>>>>> ---
> >>>>>>>> v3 -> v4:
> >>>>>>>>        add check 'rte_eth_rx_queue_info_get()' return value and
> >>>>>>>>        if it is '-ENOSTUP' calculate the 'ring_size'.
> >>>>>>>> v3:      initial version.
> >>>>>>>> ---
> >>>>>>>>     app/test-pmd/config.c | 64
> >>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
> >>>>>>>>     1 file changed, 60 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> >>>>>>>> 6496d2f..8ebb927 100644
> >>>>>>>> --- a/app/test-pmd/config.c
> >>>>>>>> +++ b/app/test-pmd/config.c
> >>>>>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t
> txq_id)
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>     static int
> >>>>>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
> >>>>>>>> +*ring_size) {
> >>>>>>>> +    struct rte_port *port = &ports[port_id];
> >>>>>>>> +    struct rte_eth_txq_info tx_qinfo;
> >>>>>>>> +    int ret;
> >>>>>>>> +
> >>>>>>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
> >>>>>>>> +    if (ret == 0) {
> >>>>>>>> +        *ring_size = tx_qinfo.nb_desc;
> >>>>>>>> +        return ret;
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    if (ret != -ENOTSUP)
> >>>>>>>> +        return ret;
> >>>>>>>> +    /*
> >>>>>>>> +     * If the rte_eth_tx_queue_info_get is not support for this
> >>>>>>>> +PMD,
> >>>>>>>> +     * ring_size stored in testpmd will be used for validity
> verification.
> >>>>>>>> +     * When configure the txq by rte_eth_tx_queue_setup with
> >>>>>>>> nb_tx_desc
> >>>>>>>> +     * being 0, it will use a default value provided by PMDs to
> >>>>>>>> +setup this
> >>>>>>>> +     * txq. If the default value is 0, it will use the
> >>>>>>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
> >>>>>>>> +     */
> >>>>>>>> +    if (port->nb_tx_desc[txq_id])
> >>>>>>>> +        *ring_size = port->nb_tx_desc[txq_id];
> >>>>>>>> +    else if (port->dev_info.default_txportconf.ring_size)
> >>>>>>>> +        *ring_size =
> >>>>>>>> +port->dev_info.default_txportconf.ring_size;
> >>>>>>>> +    else
> >>>>>>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
> >>>>>>>> +    return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int
> >>>>>>>>     rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
> >>>>>>>>         if (rxdesc_id < nb_rxd)
> >>>>>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
> >>>>>>>>         printf("Split packet: %s\n", split);
> >>>>>>>>     }
> >>>>>>>>
> >>>>>>>> +static bool
> >>>>>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
> >>>>>>>> +    uint16_t ring_size;
> >>>>>>>> +    uint16_t queue_id;
> >>>>>>>> +    uint16_t port_id;
> >>>>>>>> +    int ret;
> >>>>>>>> +
> >>>>>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
> >>>>>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> >>>>>>>> +            ret = get_tx_ring_size(port_id, queue_id,
> >>>>>>>> +&ring_size);
> >>>>>>>> +
> >>>>>>>> +            if (ret)
> >>>>>>>> +                return true;
> >>>>>>>> +
> >>>>>>>> +            if (ring_size < nb_segs) {
> >>>>>>>> +                printf("nb segments per TX packets=%u >= "
> >>>>>>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
> >>>>>>>> +                       nb_segs, queue_id, ring_size);
> >>>>>>>> +                return true;
> >>>>>>>> +            }
> >>>>>>>> +        }
> >>>>>>>> +    }
> >>>>>>>> +
> >>>>>>>> +    return false;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>>     void
> >>>>>>>>     set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
> >>>>>>>> {
> >>>>>>>>         uint16_t tx_pkt_len;
> >>>>>>>>         unsigned i;
> >>>>>>>>
> >>>>>>>> -    if (nb_segs >= (unsigned) nb_txd) {
> >>>>>>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
> >>>>>>>> ignored\n",
> >>>>>>>> -               nb_segs, (unsigned int) nb_txd);
> >>>>>>>> +    if (nb_segs_is_invalid(nb_segs))
> >>>>>>>>             return;
> >>>>>>>> -    }
> >>>>>>>>
> >>>>>>>>         /*
> >>>>>>>>          * Check that each segment length is greater or equal than
> >>>>>>>> --
> >>>>>>>> 2.9.5
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>
> >

Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on txpkts set

[Ferruh Yigit]

On 12/3/2020 9:45 AM, Slava Ovsiienko wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, December 2, 2020 14:07
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
>> <huwei013@chinasoftinc.com>
>> Cc: dev@dpdk.org; xavier.huwei@huawei.com
>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove restriction on
>> txpkts set
>>
>> On 11/27/2020 1:05 PM, Slava Ovsiienko wrote:
>>>> -----Original Message-----
>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> Sent: Thursday, November 26, 2020 14:38
>>>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; NBU-Contact-Thomas
>>>> Monjalon <thomas@monjalon.net>; Wei Hu (Xavier)
>>>> <huwei013@chinasoftinc.com>
>>>> Cc: dev@dpdk.org; xavier.huwei@huawei.com
>>>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
>>>> restriction on txpkts set
>>>>
>>>> On 11/26/2020 7:24 AM, Slava Ovsiienko wrote:
>>>>> The bug:
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbu
>>>>> gs
>>>>>
>>>>
>> .dpdk.org%2Fshow_bug.cgi%3Fid%3D584&amp;data=04%7C01%7Cviacheslavo
>>>> %40n
>>>>>
>>>>
>> vidia.com%7Ce52ba5bbab184ac8592808d8920842c5%7C43083d15727340c1b7
>>>> db39e
>>>>>
>>>>
>> fd9ccc17a%7C0%7C0%7C637419911462011700%7CUnknown%7CTWFpbGZsb3
>>>> d8eyJWIjo
>>>>>
>>>>
>> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000
>>>> &amp
>>>>>
>>>>
>> ;sdata=QBB67WqEjUHgwqHNjqx2VLdaTRMzMeodh%2B%2FVFsHByQg%3D&am
>>>> p;reserved
>>>>> =0
>>>>>
>>>>> Can we pass the nb_segs = 1 always?
>>>>> One descriptor is minimal basic capability to send, it should be
>>>>> always
>>>> supported.
>>>>>
>>>>
>>>> Hi Slava,
>>>>
>>>> I didn't get your comment, can you please elaborate?
>>>>
>>> The --txpkts is rejected on testpmd startup due to port is not
>>> configured yet and we can't find out how many descriptors are actually
>>> configured in the Tx queues.
>>>
>>> Configuring Tx queues with zero descriptors seems to be meaningless,
>>> it would disable a basic capability to send the packets. And we could
>>> assume the single segment packet sending is always supported.
>>>
>>> If --txpkts sets only the size for the single segment we can assume
>>> that the packets with only one segment is going to be sent, and we
>>> could ignore the Tx queue descriptor number check for the case.
>>>
>>
>> Overall I was OK to remove the check completely, even multi segment used it
>> is very unlikely that number of segments will be more than descriptor size.
>>
>> But at least OK to ignore the check for single segment, also we can force '--txd'
>> parameter provided to enable '--txpkts', like done before.
> 
> OK, I'll provide the patch taking both approaches on testpmd startup:
> - if --txd is specified the check will be done against it,  failed check for non-configured port will be ignored
> - if there is the only one segment specified in txpkts,  failed check for non-configured port will be ignored
> 

Sounds good, thank you.

> With best regards, Slava
> 
>>>
>>>
>>>>
>>>>> With best regards, Slava
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>> Sent: Wednesday, November 25, 2020 16:07
>>>>>> To: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Wei Hu
>>>>>> (Xavier) <huwei013@chinasoftinc.com>
>>>>>> Cc: dev@dpdk.org; xavier.huwei@huawei.com; Slava Ovsiienko
>>>>>> <viacheslavo@nvidia.com>
>>>>>> Subject: Re: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
>>>>>> restriction on txpkts set
>>>>>>
>>>>>> On 11/24/2020 12:23 PM, Ferruh Yigit wrote:
>>>>>>> On 11/24/2020 10:27 AM, Thomas Monjalon wrote:
>>>>>>>> Is it OK to keep this regression?
>>>>>>>>
>>>>>>>> Ferruh, what do you suggest?
>>>>>>>>
>>>>>>>
>>>>>>> I confirm the '--txpkts' parameter is broken now, I suggest
>>>>>>> submitting a defect for it and continue with the regression.
>>>>>>>
>>>>>>
>>>>>> Hi Slava,
>>>>>>
>>>>>> Can you please submit the Bugzilla defect?
>>>>>>
>>>>>> Thanks,
>>>>>> ferruh
>>>>>>
>>>>>>
>>>>>>> We have alternative for the parameter, "set txpkts <len0[,len1]*>"
>>>> command.
>>>>>>> The parameter was only working when hardcoded '--txd=N' parameter is
>>>>>>> provided, the command is more dynamic and works however queue size
>>>>>>> is
>>>>>> configured.
>>>>>>>
>>>>>>> We can fix the '--txpkts' in next release.
>>>>>>>
>>>>>>>>
>>>>>>>> 23/11/2020 12:50, Slava Ovsiienko:
>>>>>>>>> Hi,  Wei
>>>>>>>>>
>>>>>>>>> It was found this patch rejects the --txpkts command line settings.
>>>>>>>>> set_tx_pkt_segments() is called before device started and we have
>>>>>>>>> failure chain:
>>>>>>>>>
>>>>>>>>> set_tx_pkt_segments()
>>>>>>>>>       nb_segs_is_invalid()
>>>>>>>>>         get_tx_ring_size ()
>>>>>>>>>          rte_eth_tx_queue_info_get()
>>>>>>>>>
>>>>>>>>> It causes --txpkts testpmd command line option is ignored.
>>>>>>>>>
>>>>>>>>> With best regards, Slava
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Hu (Xavier)
>>>>>>>>>> Sent: Friday, September 25, 2020 15:47
>>>>>>>>>> To: dev@dpdk.org
>>>>>>>>>> Cc: xavier.huwei@huawei.com
>>>>>>>>>> Subject: [dpdk-dev] [PATCH v4 3/6] app/testpmd: remove
>>>>>>>>>> restriction on txpkts set
>>>>>>>>>>
>>>>>>>>>> From: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>>>>>
>>>>>>>>>> Currently, if nb_txd is not set, the txpkts is not allowed to be
>>>>>>>>>> set because the nb_txd is used to avoid the numer of segments
>>>>>>>>>> exceed the Tx ring size and the default value of nb_txd is 0. And
>>>>>>>>>> there is a bug that nb_txd is the global configuration for Tx
>>>>>>>>>> ring size and the ring size could be changed by some command per
>>>> queue.
>>>>>>>>>> So these valid check is unreliable and introduced unnecessary
>>>>>>>>>> constraints.
>>>>>>>>>>
>>>>>>>>>> This patch adds a valid check function to use the real Tx ring
>>>>>>>>>> size to check the validity of txpkts.
>>>>>>>>>>
>>>>>>>>>> Fixes: af75078fece3 ("first public release")
>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
>>>>>>>>>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
>>>>>>>>>> ---
>>>>>>>>>> v3 -> v4:
>>>>>>>>>>         add check 'rte_eth_rx_queue_info_get()' return value and
>>>>>>>>>>         if it is '-ENOSTUP' calculate the 'ring_size'.
>>>>>>>>>> v3:      initial version.
>>>>>>>>>> ---
>>>>>>>>>>      app/test-pmd/config.c | 64
>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>>>>>      1 file changed, 60 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>>>>>>>>> 6496d2f..8ebb927 100644
>>>>>>>>>> --- a/app/test-pmd/config.c
>>>>>>>>>> +++ b/app/test-pmd/config.c
>>>>>>>>>> @@ -1893,6 +1893,38 @@ tx_queue_id_is_invalid(queueid_t
>> txq_id)
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>      static int
>>>>>>>>>> +get_tx_ring_size(portid_t port_id, queueid_t txq_id, uint16_t
>>>>>>>>>> +*ring_size) {
>>>>>>>>>> +    struct rte_port *port = &ports[port_id];
>>>>>>>>>> +    struct rte_eth_txq_info tx_qinfo;
>>>>>>>>>> +    int ret;
>>>>>>>>>> +
>>>>>>>>>> +    ret = rte_eth_tx_queue_info_get(port_id, txq_id, &tx_qinfo);
>>>>>>>>>> +    if (ret == 0) {
>>>>>>>>>> +        *ring_size = tx_qinfo.nb_desc;
>>>>>>>>>> +        return ret;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    if (ret != -ENOTSUP)
>>>>>>>>>> +        return ret;
>>>>>>>>>> +    /*
>>>>>>>>>> +     * If the rte_eth_tx_queue_info_get is not support for this
>>>>>>>>>> +PMD,
>>>>>>>>>> +     * ring_size stored in testpmd will be used for validity
>> verification.
>>>>>>>>>> +     * When configure the txq by rte_eth_tx_queue_setup with
>>>>>>>>>> nb_tx_desc
>>>>>>>>>> +     * being 0, it will use a default value provided by PMDs to
>>>>>>>>>> +setup this
>>>>>>>>>> +     * txq. If the default value is 0, it will use the
>>>>>>>>>> +     * RTE_ETH_DEV_FALLBACK_TX_RINGSIZE to setup this txq.
>>>>>>>>>> +     */
>>>>>>>>>> +    if (port->nb_tx_desc[txq_id])
>>>>>>>>>> +        *ring_size = port->nb_tx_desc[txq_id];
>>>>>>>>>> +    else if (port->dev_info.default_txportconf.ring_size)
>>>>>>>>>> +        *ring_size =
>>>>>>>>>> +port->dev_info.default_txportconf.ring_size;
>>>>>>>>>> +    else
>>>>>>>>>> +        *ring_size = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int
>>>>>>>>>>      rx_desc_id_is_invalid(uint16_t rxdesc_id)  {
>>>>>>>>>>          if (rxdesc_id < nb_rxd)
>>>>>>>>>> @@ -2986,17 +3018,41 @@ show_tx_pkt_segments(void)
>>>>>>>>>>          printf("Split packet: %s\n", split);
>>>>>>>>>>      }
>>>>>>>>>>
>>>>>>>>>> +static bool
>>>>>>>>>> +nb_segs_is_invalid(unsigned int nb_segs) {
>>>>>>>>>> +    uint16_t ring_size;
>>>>>>>>>> +    uint16_t queue_id;
>>>>>>>>>> +    uint16_t port_id;
>>>>>>>>>> +    int ret;
>>>>>>>>>> +
>>>>>>>>>> +    RTE_ETH_FOREACH_DEV(port_id) {
>>>>>>>>>> +        for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>>>>>>>>> +            ret = get_tx_ring_size(port_id, queue_id,
>>>>>>>>>> +&ring_size);
>>>>>>>>>> +
>>>>>>>>>> +            if (ret)
>>>>>>>>>> +                return true;
>>>>>>>>>> +
>>>>>>>>>> +            if (ring_size < nb_segs) {
>>>>>>>>>> +                printf("nb segments per TX packets=%u >= "
>>>>>>>>>> +                       "TX queue(%u) ring_size=%u - ignored\n",
>>>>>>>>>> +                       nb_segs, queue_id, ring_size);
>>>>>>>>>> +                return true;
>>>>>>>>>> +            }
>>>>>>>>>> +        }
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>> +    return false;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>      void
>>>>>>>>>>      set_tx_pkt_segments(unsigned *seg_lengths, unsigned nb_segs)
>>>>>>>>>> {
>>>>>>>>>>          uint16_t tx_pkt_len;
>>>>>>>>>>          unsigned i;
>>>>>>>>>>
>>>>>>>>>> -    if (nb_segs >= (unsigned) nb_txd) {
>>>>>>>>>> -        printf("nb segments per TX packets=%u >= nb_txd=%u -
>>>>>>>>>> ignored\n",
>>>>>>>>>> -               nb_segs, (unsigned int) nb_txd);
>>>>>>>>>> +    if (nb_segs_is_invalid(nb_segs))
>>>>>>>>>>              return;
>>>>>>>>>> -    }
>>>>>>>>>>
>>>>>>>>>>          /*
>>>>>>>>>>           * Check that each segment length is greater or equal than
>>>>>>>>>> --
>>>>>>>>>> 2.9.5
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
> 

[dpdk-dev] [PATCH] app/testpmd: fix segment number check

[Viacheslav Ovsiienko]

The --txpkts command line parameter was silently ignored due to
application was unable to check the Tx queue ring sizes for non
configured ports [1].

The "set txpkts <len0[,len1]*>" was also rejected if there
was some stopped or /unconfigured port.

This provides the following:

  - number of segment check is performed against
    configured Tx queues only

  - the capability to send single packet is supposed to
    be very basic and always supported, the setting segment
    number to 1 is always allowed, no check performed

  - at the moment of Tx queue setup the descriptor number is
    checked against configured segment number

Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set")
Cc: stable@dpdk.org
Bugzilla ID: 584

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
---
 app/test-pmd/cmdline.c |  5 +++++
 app/test-pmd/config.c  | 21 ++++++++++++++++-----
 app/test-pmd/testpmd.c |  7 ++++++-
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 0d2d6aa..86388a2 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue {
 		if (!numa_support || socket_id == NUMA_NO_CONFIG)
 			socket_id = port->socket_id;
 
+		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
+			printf("Failed to setup TX queue: "
+			       "not enough descriptors\n");
+			return;
+		}
 		ret = rte_eth_tx_queue_setup(res->portid,
 					     res->qid,
 					     port->nb_tx_desc[res->qid],
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index b51de59..a6fccfa 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3911,12 +3911,18 @@ struct igb_ring_desc_16_bytes {
 		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
 			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
 
-			if (ret)
+			/* Do the check only for the active/configured ports. */
+			if (ret == -EINVAL)
+				continue;
+			if (ret) {
+				printf("failed to get ring size for TX "
+				       "queue(%u) Port(%u) - txpkts ignored\n",
+				       port_id, queue_id);
 				return true;
-
+			}
 			if (ring_size < nb_segs) {
-				printf("nb segments per TX packets=%u >= "
-				       "TX queue(%u) ring_size=%u - ignored\n",
+				printf("nb segments per TX packets=%u >= TX "
+				       "queue(%u) ring_size=%u - txpkts ignored\n",
 				       nb_segs, queue_id, ring_size);
 				return true;
 			}
@@ -3932,7 +3938,12 @@ struct igb_ring_desc_16_bytes {
 	uint16_t tx_pkt_len;
 	unsigned int i;
 
-	if (nb_segs_is_invalid(nb_segs))
+	/*
+	 * For single sengment settings failed check is ignored.
+	 * It is a very basic capability to send the single segment
+	 * packets, suppose it is always supported.
+	 */
+	if (nb_segs > 1 && nb_segs_is_invalid(nb_segs))
 		return;
 
 	/*
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 33fc0fd..9ea0145 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2575,6 +2575,11 @@ struct extmem_param {
 			port->need_reconfig_queues = 0;
 			/* setup tx queues */
 			for (qi = 0; qi < nb_txq; qi++) {
+				if (port->nb_tx_desc[qi] < tx_pkt_nb_segs) {
+					printf("Failed to setup TX queue: "
+					       "not enough descriptors\n");
+					goto fail;
+				}
 				if ((numa_support) &&
 					(txring_numa[pi] != NUMA_NO_CONFIG))
 					diag = rte_eth_tx_queue_setup(pi, qi,
@@ -2589,7 +2594,7 @@ struct extmem_param {
 
 				if (diag == 0)
 					continue;
-
+fail:
 				/* Fail to setup tx queue, return */
 				if (rte_atomic16_cmpset(&(port->port_status),
 							RTE_PORT_HANDLING,
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check

[Andrew Boyer]

> On Dec 11, 2020, at 10:07 AM, Viacheslav Ovsiienko <viacheslavo@nvidia.com> wrote:
> 
> The --txpkts command line parameter was silently ignored due to
> application was unable to check the Tx queue ring sizes for non
> configured ports [1].

... ignored because the application...

> The "set txpkts <len0[,len1]*>" was also rejected if there
> was some stopped or /unconfigured port.

... was a stopped or unconfigured ...

> 
> This provides the following:
> 
>  - number of segment check is performed against
>    configured Tx queues only
> 
>  - the capability to send single packet is supposed to
>    be very basic and always supported, the setting segment
>    number to 1 is always allowed, no check performed
> 
>  - at the moment of Tx queue setup the descriptor number is
>    checked against configured segment number
> 
> Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set")
> Cc: stable@dpdk.org
> Bugzilla ID: 584
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
> app/test-pmd/cmdline.c |  5 +++++
> app/test-pmd/config.c  | 21 ++++++++++++++++-----
> app/test-pmd/testpmd.c |  7 ++++++-
> 3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0d2d6aa..86388a2 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue {
> 		if (!numa_support || socket_id == NUMA_NO_CONFIG)
> 			socket_id = port->socket_id;
> 
> +		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
> +			printf("Failed to setup TX queue: "

setup -> set up
I find it helpful when the numbers are logged in the error message.  Like “nb_desc 8 < nb_segs 16”.

> +			       "not enough descriptors\n");
> +			return;
> +		}

Why is there a relationship between the number of descriptors and the number of segments? For our device, there isn’t. We can send 16 Tx segments per descriptor and (I suppose) you could try to create an 8 descriptor ring.

Maybe this is to protect a simpler device that consumes one descriptor per segment? If so, the check would ideally be conditioned on a related device capability flag. I’m not sure if there is such a flag today.

> 		ret = rte_eth_tx_queue_setup(res->portid,
> 					     res->qid,
> 					     port->nb_tx_desc[res->qid],
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index b51de59..a6fccfa 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3911,12 +3911,18 @@ struct igb_ring_desc_16_bytes {
> 		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> 			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
> 
> -			if (ret)
> +			/* Do the check only for the active/configured ports. */
> +			if (ret == -EINVAL)
> +				continue;
> +			if (ret) {
> +				printf("failed to get ring size for TX "
> +				       "queue(%u) Port(%u) - txpkts ignored\n",
> +				       port_id, queue_id);
> 				return true;
> -
> +			}
> 			if (ring_size < nb_segs) {
> -				printf("nb segments per TX packets=%u >= "
> -				       "TX queue(%u) ring_size=%u - ignored\n",
> +				printf("nb segments per TX packets=%u >= TX "
> +				       "queue(%u) ring_size=%u - txpkts ignored\n",
> 				       nb_segs, queue_id, ring_size);
> 				return true;
> 			}
> @@ -3932,7 +3938,12 @@ struct igb_ring_desc_16_bytes {
> 	uint16_t tx_pkt_len;
> 	unsigned int i;
> 
> -	if (nb_segs_is_invalid(nb_segs))
> +	/*
> +	 * For single sengment settings failed check is ignored.
> +	 * It is a very basic capability to send the single segment
> +	 * packets, suppose it is always supported.

sengment -> segment
... to send single segment...
suppose -> assume

> +	 */
> +	if (nb_segs > 1 && nb_segs_is_invalid(nb_segs))
> 		return;
> 
> 	/*
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 33fc0fd..9ea0145 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2575,6 +2575,11 @@ struct extmem_param {
> 			port->need_reconfig_queues = 0;
> 			/* setup tx queues */
> 			for (qi = 0; qi < nb_txq; qi++) {
> +				if (port->nb_tx_desc[qi] < tx_pkt_nb_segs) {
> +					printf("Failed to setup TX queue: "
> +					       "not enough descriptors\n");

Same comments as above

> +					goto fail;
> +				}
> 				if ((numa_support) &&
> 					(txring_numa[pi] != NUMA_NO_CONFIG))
> 					diag = rte_eth_tx_queue_setup(pi, qi,
> @@ -2589,7 +2594,7 @@ struct extmem_param {
> 
> 				if (diag == 0)
> 					continue;
> -
> +fail:
> 				/* Fail to setup tx queue, return */
> 				if (rte_atomic16_cmpset(&(port->port_status),
> 							RTE_PORT_HANDLING,
> -- 
> 1.8.3.1
> 

Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check

[Slava Ovsiienko]

Hi, Andrew

Thank you for the review, please, see below.

> -----Original Message-----
> From: Andrew Boyer <aboyer@pensando.io>
> Sent: Friday, December 11, 2020 18:00
> To: Slava Ovsiienko <viacheslavo@nvidia.com>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> ferruh.yigit@intel.com; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check
> 
> 
> 
> > On Dec 11, 2020, at 10:07 AM, Viacheslav Ovsiienko
> <viacheslavo@nvidia.com> wrote:
> >
> > The --txpkts command line parameter was silently ignored due to
> > application was unable to check the Tx queue ring sizes for non
> > configured ports [1].
> 
> ... ignored because the application...
OK, will fix.

> 
> > The "set txpkts <len0[,len1]*>" was also rejected if there was some
> > stopped or /unconfigured port.
> 
> ... was a stopped or unconfigured ...
OK, will fix.

> 
> >
> > This provides the following:
> >
> >  - number of segment check is performed against
> >    configured Tx queues only
> >
> >  - the capability to send single packet is supposed to
> >    be very basic and always supported, the setting segment
> >    number to 1 is always allowed, no check performed
> >
> >  - at the moment of Tx queue setup the descriptor number is
> >    checked against configured segment number
> >
> > Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments
> > set")
> > Cc: stable@dpdk.org
> > Bugzilla ID: 584
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> > ---
> > app/test-pmd/cmdline.c |  5 +++++
> > app/test-pmd/config.c  | 21 ++++++++++++++++-----
> > app/test-pmd/testpmd.c |  7 ++++++-
> > 3 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> > 0d2d6aa..86388a2 100644
> > --- a/app/test-pmd/cmdline.c
> > +++ b/app/test-pmd/cmdline.c
> > @@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue {
> > 		if (!numa_support || socket_id == NUMA_NO_CONFIG)
> > 			socket_id = port->socket_id;
> >
> > +		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
> > +			printf("Failed to setup TX queue: "
> 
> setup -> set up
Disagree, it is quite common in testpmd code to use "setup" wording,
 I just copy-pasted the message from the neighbor lines.

> I find it helpful when the numbers are logged in the error message.  Like
> “nb_desc 8 < nb_segs 16”.
> 
> > +			       "not enough descriptors\n");
> > +			return;
> > +		}
> 
Do you think it is worth to be informative so much? OK, will add.

> Why is there a relationship between the number of descriptors and the
> number of segments? For our device, there isn’t. We can send 16 Tx segments
> per descriptor and (I suppose) you could try to create an 8 descriptor ring.
> 
> Maybe this is to protect a simpler device that consumes one descriptor per
> segment? If so, the check would ideally be conditioned on a related device
> capability flag. I’m not sure if there is such a flag today.
There is no correlation between  n_desc and n_seg for Tx in mlx5 PMD either.
And there is no information provided how many descriptors should be
provided for the multi-segment packets.

If we have a look at original commit being fixed
("app/testpmd: remove restriction on Tx segments set") we'll see:

-       if (nb_segs >= (unsigned) nb_txd) {
-               printf("nb segments per TX packets=%u >= nb_txd=%u - ignored\n",
-                      nb_segs, (unsigned int) nb_txd);

So, the check was added in replacement for other, more strict, check.
Now we are just improving one a little bit.


> 
> > 		ret = rte_eth_tx_queue_setup(res->portid,
> > 					     res->qid,
> > 					     port->nb_tx_desc[res->qid],
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > b51de59..a6fccfa 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -3911,12 +3911,18 @@ struct igb_ring_desc_16_bytes {
> > 		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
> > 			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
> >
> > -			if (ret)
> > +			/* Do the check only for the active/configured ports.
> */
> > +			if (ret == -EINVAL)
> > +				continue;
> > +			if (ret) {
> > +				printf("failed to get ring size for TX "
> > +				       "queue(%u) Port(%u) - txpkts ignored\n",
> > +				       port_id, queue_id);
> > 				return true;
> > -
> > +			}
> > 			if (ring_size < nb_segs) {
> > -				printf("nb segments per TX packets=%u >= "
> > -				       "TX queue(%u) ring_size=%u - ignored\n",
> > +				printf("nb segments per TX packets=%u >= TX
> "
> > +				       "queue(%u) ring_size=%u - txpkts
> ignored\n",
> > 				       nb_segs, queue_id, ring_size);
> > 				return true;
> > 			}
> > @@ -3932,7 +3938,12 @@ struct igb_ring_desc_16_bytes {
> > 	uint16_t tx_pkt_len;
> > 	unsigned int i;
> >
> > -	if (nb_segs_is_invalid(nb_segs))
> > +	/*
> > +	 * For single sengment settings failed check is ignored.
> > +	 * It is a very basic capability to send the single segment
> > +	 * packets, suppose it is always supported.
> 
> sengment -> segment
> ... to send single segment...
> suppose -> assume
OK, np, will fix.

> 
> > +	 */
> > +	if (nb_segs > 1 && nb_segs_is_invalid(nb_segs))
> > 		return;
> >
> > 	/*
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 33fc0fd..9ea0145 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2575,6 +2575,11 @@ struct extmem_param {
> > 			port->need_reconfig_queues = 0;
> > 			/* setup tx queues */
> > 			for (qi = 0; qi < nb_txq; qi++) {
> > +				if (port->nb_tx_desc[qi] < tx_pkt_nb_segs) {
> > +					printf("Failed to setup TX queue: "
> > +					       "not enough descriptors\n");
> 
> Same comments as above
OK.

> 
> > +					goto fail;
> > +				}
> > 				if ((numa_support) &&
> > 					(txring_numa[pi] !=
> NUMA_NO_CONFIG))
> > 					diag = rte_eth_tx_queue_setup(pi, qi,
> @@ -2589,7 +2594,7 @@
> > struct extmem_param {
> >
> > 				if (diag == 0)
> > 					continue;
> > -
> > +fail:
> > 				/* Fail to setup tx queue, return */
> > 				if (rte_atomic16_cmpset(&(port-
> >port_status),
> >
> 	RTE_PORT_HANDLING,
> > --
> > 1.8.3.1
> >

Thanks a lot, I will wait for a while for more comments and provide v2.

With best regards, Slava

Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check

[Ferruh Yigit]

On 12/11/2020 4:14 PM, Slava Ovsiienko wrote:
> Hi, Andrew
> 
> Thank you for the review, please, see below.
> 
>> -----Original Message-----
>> From: Andrew Boyer <aboyer@pensando.io>
>> Sent: Friday, December 11, 2020 18:00
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>
>> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>> ferruh.yigit@intel.com; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check
>>
>>
>>
>>> On Dec 11, 2020, at 10:07 AM, Viacheslav Ovsiienko
>> <viacheslavo@nvidia.com> wrote:
>>>
>>> The --txpkts command line parameter was silently ignored due to
>>> application was unable to check the Tx queue ring sizes for non
>>> configured ports [1].
>>
>> ... ignored because the application...
> OK, will fix.
> 
>>
>>> The "set txpkts <len0[,len1]*>" was also rejected if there was some
>>> stopped or /unconfigured port.
>>
>> ... was a stopped or unconfigured ...
> OK, will fix.
> 
>>
>>>
>>> This provides the following:
>>>
>>>   - number of segment check is performed against
>>>     configured Tx queues only
>>>
>>>   - the capability to send single packet is supposed to
>>>     be very basic and always supported, the setting segment
>>>     number to 1 is always allowed, no check performed
>>>
>>>   - at the moment of Tx queue setup the descriptor number is
>>>     checked against configured segment number
>>>
>>> Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments
>>> set")
>>> Cc: stable@dpdk.org
>>> Bugzilla ID: 584
>>>
>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>> ---
>>> app/test-pmd/cmdline.c |  5 +++++
>>> app/test-pmd/config.c  | 21 ++++++++++++++++-----
>>> app/test-pmd/testpmd.c |  7 ++++++-
>>> 3 files changed, 27 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
>>> 0d2d6aa..86388a2 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue {
>>> 		if (!numa_support || socket_id == NUMA_NO_CONFIG)
>>> 			socket_id = port->socket_id;
>>>
>>> +		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
>>> +			printf("Failed to setup TX queue: "
>>
>> setup -> set up
> Disagree, it is quite common in testpmd code to use "setup" wording,
>   I just copy-pasted the message from the neighbor lines.
> 
>> I find it helpful when the numbers are logged in the error message.  Like
>> “nb_desc 8 < nb_segs 16”.
>>
>>> +			       "not enough descriptors\n");
>>> +			return;
>>> +		}
>>
> Do you think it is worth to be informative so much? OK, will add.
> 
>> Why is there a relationship between the number of descriptors and the
>> number of segments? For our device, there isn’t. We can send 16 Tx segments
>> per descriptor and (I suppose) you could try to create an 8 descriptor ring.
>>
>> Maybe this is to protect a simpler device that consumes one descriptor per
>> segment? If so, the check would ideally be conditioned on a related device
>> capability flag. I’m not sure if there is such a flag today.
> There is no correlation between  n_desc and n_seg for Tx in mlx5 PMD either.
> And there is no information provided how many descriptors should be
> provided for the multi-segment packets.
> 
> If we have a look at original commit being fixed
> ("app/testpmd: remove restriction on Tx segments set") we'll see:
> 
> -       if (nb_segs >= (unsigned) nb_txd) {
> -               printf("nb segments per TX packets=%u >= nb_txd=%u - ignored\n",
> -                      nb_segs, (unsigned int) nb_txd);
> 
> So, the check was added in replacement for other, more strict, check.
> Now we are just improving one a little bit.
> 

Many devices use a descriptor per segment, and if there is no enough free 
descriptor to fit all segments they won't able to send the packet, I guess this 
check is to cover them.

Out of curiosity, is your device has 16 buffer address fields in the descriptor, 
can they be utilized to send multiple independent packets in single descriptor?


> 
>>
>>> 		ret = rte_eth_tx_queue_setup(res->portid,
>>> 					     res->qid,
>>> 					     port->nb_tx_desc[res->qid],
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>> b51de59..a6fccfa 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -3911,12 +3911,18 @@ struct igb_ring_desc_16_bytes {
>>> 		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>>> 			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
>>>
>>> -			if (ret)
>>> +			/* Do the check only for the active/configured ports.
>> */
>>> +			if (ret == -EINVAL)
>>> +				continue;
>>> +			if (ret) {
>>> +				printf("failed to get ring size for TX "
>>> +				       "queue(%u) Port(%u) - txpkts ignored\n",
>>> +				       port_id, queue_id);
>>> 				return true;
>>> -
>>> +			}
>>> 			if (ring_size < nb_segs) {
>>> -				printf("nb segments per TX packets=%u >= "
>>> -				       "TX queue(%u) ring_size=%u - ignored\n",
>>> +				printf("nb segments per TX packets=%u >= TX
>> "
>>> +				       "queue(%u) ring_size=%u - txpkts
>> ignored\n",
>>> 				       nb_segs, queue_id, ring_size);
>>> 				return true;
>>> 			}
>>> @@ -3932,7 +3938,12 @@ struct igb_ring_desc_16_bytes {
>>> 	uint16_t tx_pkt_len;
>>> 	unsigned int i;
>>>
>>> -	if (nb_segs_is_invalid(nb_segs))
>>> +	/*
>>> +	 * For single sengment settings failed check is ignored.
>>> +	 * It is a very basic capability to send the single segment
>>> +	 * packets, suppose it is always supported.
>>
>> sengment -> segment
>> ... to send single segment...
>> suppose -> assume
> OK, np, will fix.
> 
>>
>>> +	 */
>>> +	if (nb_segs > 1 && nb_segs_is_invalid(nb_segs))
>>> 		return;
>>>
>>> 	/*
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 33fc0fd..9ea0145 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2575,6 +2575,11 @@ struct extmem_param {
>>> 			port->need_reconfig_queues = 0;
>>> 			/* setup tx queues */
>>> 			for (qi = 0; qi < nb_txq; qi++) {
>>> +				if (port->nb_tx_desc[qi] < tx_pkt_nb_segs) {
>>> +					printf("Failed to setup TX queue: "
>>> +					       "not enough descriptors\n");
>>
>> Same comments as above
> OK.
> 
>>
>>> +					goto fail;
>>> +				}
>>> 				if ((numa_support) &&
>>> 					(txring_numa[pi] !=
>> NUMA_NO_CONFIG))
>>> 					diag = rte_eth_tx_queue_setup(pi, qi,
>> @@ -2589,7 +2594,7 @@
>>> struct extmem_param {
>>>
>>> 				if (diag == 0)
>>> 					continue;
>>> -
>>> +fail:
>>> 				/* Fail to setup tx queue, return */
>>> 				if (rte_atomic16_cmpset(&(port-
>>> port_status),
>>>
>> 	RTE_PORT_HANDLING,
>>> --
>>> 1.8.3.1
>>>
> 
> Thanks a lot, I will wait for a while for more comments and provide v2.
> 
> With best regards, Slava
> 

Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check

[Slava Ovsiienko]

Hi, Ferruh

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, December 16, 2020 14:12
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Andrew Boyer
> <aboyer@pensando.io>
> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check
> 
> On 12/11/2020 4:14 PM, Slava Ovsiienko wrote:
> > Hi, Andrew
> >
> > Thank you for the review, please, see below.
> >
> >> -----Original Message-----
> >> From: Andrew Boyer <aboyer@pensando.io>
> >> Sent: Friday, December 11, 2020 18:00
> >> To: Slava Ovsiienko <viacheslavo@nvidia.com>
> >> Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>;
> >> ferruh.yigit@intel.com; stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check
> >>
> >>
> >>
> >>> On Dec 11, 2020, at 10:07 AM, Viacheslav Ovsiienko
> >> <viacheslavo@nvidia.com> wrote:
> >>>
> >>> The --txpkts command line parameter was silently ignored due to
> >>> application was unable to check the Tx queue ring sizes for non
> >>> configured ports [1].
> >>
> >> ... ignored because the application...
> > OK, will fix.
> >
> >>
> >>> The "set txpkts <len0[,len1]*>" was also rejected if there was some
> >>> stopped or /unconfigured port.
> >>
> >> ... was a stopped or unconfigured ...
> > OK, will fix.
> >
> >>
> >>>
> >>> This provides the following:
> >>>
> >>>   - number of segment check is performed against
> >>>     configured Tx queues only
> >>>
> >>>   - the capability to send single packet is supposed to
> >>>     be very basic and always supported, the setting segment
> >>>     number to 1 is always allowed, no check performed
> >>>
> >>>   - at the moment of Tx queue setup the descriptor number is
> >>>     checked against configured segment number
> >>>
> >>> Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments
> >>> set")
> >>> Cc: stable@dpdk.org
> >>> Bugzilla ID: 584
> >>>
> >>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> >>> ---
> >>> app/test-pmd/cmdline.c |  5 +++++
> >>> app/test-pmd/config.c  | 21 ++++++++++++++++-----
> >>> app/test-pmd/testpmd.c |  7 ++++++-
> >>> 3 files changed, 27 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> >>> 0d2d6aa..86388a2 100644
> >>> --- a/app/test-pmd/cmdline.c
> >>> +++ b/app/test-pmd/cmdline.c
> >>> @@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue {
> >>> 		if (!numa_support || socket_id == NUMA_NO_CONFIG)
> >>> 			socket_id = port->socket_id;
> >>>
> >>> +		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
> >>> +			printf("Failed to setup TX queue: "
> >>
> >> setup -> set up
> > Disagree, it is quite common in testpmd code to use "setup" wording,
> >   I just copy-pasted the message from the neighbor lines.
> >
> >> I find it helpful when the numbers are logged in the error message.
> >> Like “nb_desc 8 < nb_segs 16”.
> >>
> >>> +			       "not enough descriptors\n");
> >>> +			return;
> >>> +		}
> >>
> > Do you think it is worth to be informative so much? OK, will add.
> >
> >> Why is there a relationship between the number of descriptors and the
> >> number of segments? For our device, there isn’t. We can send 16 Tx
> >> segments per descriptor and (I suppose) you could try to create an 8
> descriptor ring.
> >>
> >> Maybe this is to protect a simpler device that consumes one
> >> descriptor per segment? If so, the check would ideally be conditioned
> >> on a related device capability flag. I’m not sure if there is such a flag today.
> > There is no correlation between  n_desc and n_seg for Tx in mlx5 PMD either.
> > And there is no information provided how many descriptors should be
> > provided for the multi-segment packets.
> >
> > If we have a look at original commit being fixed
> > ("app/testpmd: remove restriction on Tx segments set") we'll see:
> >
> > -       if (nb_segs >= (unsigned) nb_txd) {
> > -               printf("nb segments per TX packets=%u >= nb_txd=%u - ignored\n",
> > -                      nb_segs, (unsigned int) nb_txd);
> >
> > So, the check was added in replacement for other, more strict, check.
> > Now we are just improving one a little bit.
> >
> 
> Many devices use a descriptor per segment, and if there is no enough free
> descriptor to fit all segments they won't able to send the packet, I guess this
> check is to cover them.
> 
> Out of curiosity, is your device has 16 buffer address fields in the descriptor,
> can they be utilized to send multiple independent packets in single descriptor?
> 
Regarding mlx5 - there is no strong correspondence between WQE (HW desc) and
mbufs. The ConnectX-5+ supports various method of placing data to the descriptors -
by direct data inline or by pointers. In average, with engaged MPW (multipacket-write)
feature we can put up to 4 mbuf pointers into one WQE. WQEs can be combined to
handle 16-or-even-more-mbufs-chain packets. Hence, check for descriptors being discussed 
is still relevant for mlx5 disregarding it is just evaluative.

With best regards, Slava

[snip]

Re: [dpdk-dev] [PATCH] app/testpmd: fix segment number check

[Ferruh Yigit]

On 12/11/2020 3:07 PM, Viacheslav Ovsiienko wrote:
> The --txpkts command line parameter was silently ignored due to
> application was unable to check the Tx queue ring sizes for non
> configured ports [1].
> 
> The "set txpkts <len0[,len1]*>" was also rejected if there
> was some stopped or /unconfigured port.
> 

May not be for the commit log but to understand the problem here,
what are the steps to reproduce the problem in "set txpkts" command?

> This provides the following:
> 
>    - number of segment check is performed against
>      configured Tx queues only
> 
>    - the capability to send single packet is supposed to
>      be very basic and always supported, the setting segment
>      number to 1 is always allowed, no check performed
> 
>    - at the moment of Tx queue setup the descriptor number is
>      checked against configured segment number

Not sure about this one, more comments below.

> 
> Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set")
> Cc: stable@dpdk.org
> Bugzilla ID: 584
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> ---
>   app/test-pmd/cmdline.c |  5 +++++
>   app/test-pmd/config.c  | 21 ++++++++++++++++-----
>   app/test-pmd/testpmd.c |  7 ++++++-
>   3 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 0d2d6aa..86388a2 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2798,6 +2798,11 @@ struct cmd_setup_rxtx_queue {
>   		if (!numa_support || socket_id == NUMA_NO_CONFIG)
>   			socket_id = port->socket_id;
>   
> +		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
> +			printf("Failed to setup TX queue: "
> +			       "not enough descriptors\n");
> +			return;
> +		}

The "port->nb_tx_desc[res->qid]" can be '0', and that is the default value in 
the testpmd, in that case device suggested values are used. Above check fails 
when '--txd' arguments is not provided.
Same problem with the same check below in 'start_port()', I think both can be 
removed.

>   		ret = rte_eth_tx_queue_setup(res->portid,
>   					     res->qid,
>   					     port->nb_tx_desc[res->qid],
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index b51de59..a6fccfa 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -3911,12 +3911,18 @@ struct igb_ring_desc_16_bytes {
>   		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
>   			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
>   
> -			if (ret)
> +			/* Do the check only for the active/configured ports. */
> +			if (ret == -EINVAL)
> +				continue;
> +			if (ret) {
> +				printf("failed to get ring size for TX "
> +				       "queue(%u) Port(%u) - txpkts ignored\n",
> +				       port_id, queue_id);
>   				return true;

Is there a need to filter the '-EINVAL' errors only, the other error this 
function can get is '-EINVAL' & '-ENODEV' (which is 'port_id' is not valid), to 
simplify the logic, what do you think just ignore any kind of error and not fail 
in that case?

> -
> +			}
>   			if (ring_size < nb_segs) {
> -				printf("nb segments per TX packets=%u >= "
> -				       "TX queue(%u) ring_size=%u - ignored\n",
> +				printf("nb segments per TX packets=%u >= TX "
> +				       "queue(%u) ring_size=%u - txpkts ignored\n",
>   				       nb_segs, queue_id, ring_size);
>   				return true;
>   			}
> @@ -3932,7 +3938,12 @@ struct igb_ring_desc_16_bytes {
>   	uint16_t tx_pkt_len;
>   	unsigned int i;
>   
> -	if (nb_segs_is_invalid(nb_segs))
> +	/*
> +	 * For single sengment settings failed check is ignored.
> +	 * It is a very basic capability to send the single segment
> +	 * packets, suppose it is always supported.
> +	 */
> +	if (nb_segs > 1 && nb_segs_is_invalid(nb_segs))
>   		return;

if the user provided '--txd' in the command line, we can compare the segment 
size with that without going into the device configured values, as similar to 
what is done before:

At the very beginning of the 'get_tx_ring_size()':

if (nb_txd) {
   *ring_size = nb_txd;
   return 0;
}

So following combination of parameters will be supported

"--txpkts=X,Y --txd=Z"  (segment size checked against nb_txd)
"--txpkts=N "           (single segment, no check)
"--txpkts=X,Y"          (segment size not checked)

And setting same in the command line always should be supported with segment 
size checks against the
- nb_txd when '--txd=Z' provided
- dynamic device provided values when '--txd=Z' not provided

>   
>   	/*
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 33fc0fd..9ea0145 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2575,6 +2575,11 @@ struct extmem_param {
>   			port->need_reconfig_queues = 0;
>   			/* setup tx queues */
>   			for (qi = 0; qi < nb_txq; qi++) {
> +				if (port->nb_tx_desc[qi] < tx_pkt_nb_segs) {
> +					printf("Failed to setup TX queue: "
> +					       "not enough descriptors\n");
> +					goto fail;
> +				}

Please check above comment.

>   				if ((numa_support) &&
>   					(txring_numa[pi] != NUMA_NO_CONFIG))
>   					diag = rte_eth_tx_queue_setup(pi, qi,
> @@ -2589,7 +2594,7 @@ struct extmem_param {
>   
>   				if (diag == 0)
>   					continue;
> -
> +fail:
>   				/* Fail to setup tx queue, return */
>   				if (rte_atomic16_cmpset(&(port->port_status),
>   							RTE_PORT_HANDLING,
> 

[dpdk-dev] [PATCH v2] app/testpmd: fix segment number check

[Ferruh Yigit]

From: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

The --txpkts command line parameter was silently ignored due to
application was unable to check the Tx queue ring sizes for non
configured ports [1].

The "set txpkts <len0[,len1]*>" was also rejected if there
was some stopped or /unconfigured port.

This provides the following:

  - If fails to get ring size from the port, this can be because port is
    not initialized yet, ignore the check and just be sure segment size
    won't cause an out of bound access. The port descriptor check will
    be done during Tx setup.

  - The capability to send single packet is supposed to be very basic
    and always supported, the setting segment number to 1 is always
    allowed, no check performed

  - At the moment of Tx queue setup the descriptor number is checked
    against configured segment number

Bugzilla ID: 584
Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set")
Cc: stable@dpdk.org

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Andrew Boyer <aboyer@pensando.io>

v2:
* Become more flexible for the '--txpkts' command line, if not able to
  get the descriptor size from port, ignore the check.

  ('nb_txd' check was proposed before, this will require '--txd'
  parameter, but also enforces a specific order on the parameters,
  instead going with the option to flex the checks for parameter.)
---
 app/test-pmd/cmdline.c |  4 ++++
 app/test-pmd/config.c  | 32 ++++++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 12efbc0cab46..7feba8337781 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2910,6 +2910,10 @@ cmd_setup_rxtx_queue_parsed(
 		if (!numa_support || socket_id == NUMA_NO_CONFIG)
 			socket_id = port->socket_id;
 
+		if (port->nb_tx_desc[res->qid] < tx_pkt_nb_segs) {
+			printf("Failed to setup TX queue: not enough descriptors\n");
+			return;
+		}
 		ret = rte_eth_tx_queue_setup(res->portid,
 					     res->qid,
 					     port->nb_tx_desc[res->qid],
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index e189062efde8..a4445a73bfa5 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -3697,13 +3697,15 @@ nb_segs_is_invalid(unsigned int nb_segs)
 	RTE_ETH_FOREACH_DEV(port_id) {
 		for (queue_id = 0; queue_id < nb_txq; queue_id++) {
 			ret = get_tx_ring_size(port_id, queue_id, &ring_size);
-
-			if (ret)
-				return true;
-
+			if (ret) {
+				/* Port may not be initialized yet, can't say
+				 * the port is invalid in this stage.
+				 */
+				continue;
+			}
 			if (ring_size < nb_segs) {
-				printf("nb segments per TX packets=%u >= "
-				       "TX queue(%u) ring_size=%u - ignored\n",
+				printf("nb segments per TX packets=%u >= TX "
+				       "queue(%u) ring_size=%u - txpkts ignored\n",
 				       nb_segs, queue_id, ring_size);
 				return true;
 			}
@@ -3719,12 +3721,26 @@ set_tx_pkt_segments(unsigned int *seg_lengths, unsigned int nb_segs)
 	uint16_t tx_pkt_len;
 	unsigned int i;
 
-	if (nb_segs_is_invalid(nb_segs))
+	/*
+	 * For single segment settings failed check is ignored.
+	 * It is a very basic capability to send the single segment
+	 * packets, suppose it is always supported.
+	 */
+	if (nb_segs > 1 && nb_segs_is_invalid(nb_segs)) {
+		printf("Tx segment size(%u) is not supported - txpkts ignored\n",
+			nb_segs);
 		return;
+	}
+
+	if (nb_segs > RTE_MAX_SEGS_PER_PKT) {
+		printf("Tx segment size(%u) is bigger than max number of segment(%u)\n",
+			nb_segs, RTE_MAX_SEGS_PER_PKT);
+		return;
+	}
 
 	/*
 	 * Check that each segment length is greater or equal than
-	 * the mbuf data sise.
+	 * the mbuf data size.
 	 * Check also that the total packet length is greater or equal than the
 	 * size of an empty UDP/IP packet (sizeof(struct rte_ether_hdr) +
 	 * 20 + 8).
-- 
2.30.2

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix segment number check

[Li, Xiaoyun]

Hi

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Saturday, April 24, 2021 00:10
> To: Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Wei Hu (Xavier) <xavier.huwei@huawei.com>;
> Chengchang Tang <tangchengchang@huawei.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
> Andrew Boyer <aboyer@pensando.io>
> Subject: [PATCH v2] app/testpmd: fix segment number check
> 
> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> 
> The --txpkts command line parameter was silently ignored due to application
> was unable to check the Tx queue ring sizes for non configured ports [1].

Remove this [1] or mark the following items as [1] [2] [3].

> 
> The "set txpkts <len0[,len1]*>" was also rejected if there was some stopped or
> /unconfigured port.
> 
> This provides the following:
> 
>   - If fails to get ring size from the port, this can be because port is
>     not initialized yet, ignore the check and just be sure segment size
>     won't cause an out of bound access. The port descriptor check will
>     be done during Tx setup.
> 
>   - The capability to send single packet is supposed to be very basic
>     and always supported, the setting segment number to 1 is always
>     allowed, no check performed
> 
>   - At the moment of Tx queue setup the descriptor number is checked
>     against configured segment number
> 
> Bugzilla ID: 584
> Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Andrew Boyer <aboyer@pensando.io>
> 
> v2:
> * Become more flexible for the '--txpkts' command line, if not able to
>   get the descriptor size from port, ignore the check.
> 
>   ('nb_txd' check was proposed before, this will require '--txd'
>   parameter, but also enforces a specific order on the parameters,
>   instead going with the option to flex the checks for parameter.)
> ---
>  app/test-pmd/cmdline.c |  4 ++++
>  app/test-pmd/config.c  | 32 ++++++++++++++++++++++++--------
>  2 files changed, 28 insertions(+), 8 deletions(-)

Except the one comment above for commit log,
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>

Re: [dpdk-dev] [PATCH v2] app/testpmd: fix segment number check

[Ferruh Yigit]

On 4/26/2021 12:23 PM, Li, Xiaoyun wrote:
> Hi
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Saturday, April 24, 2021 00:10
>> To: Viacheslav Ovsiienko <viacheslavo@nvidia.com>; Li, Xiaoyun
>> <xiaoyun.li@intel.com>; Wei Hu (Xavier) <xavier.huwei@huawei.com>;
>> Chengchang Tang <tangchengchang@huawei.com>
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; dev@dpdk.org; stable@dpdk.org;
>> Andrew Boyer <aboyer@pensando.io>
>> Subject: [PATCH v2] app/testpmd: fix segment number check
>>
>> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>>
>> The --txpkts command line parameter was silently ignored due to application
>> was unable to check the Tx queue ring sizes for non configured ports [1].
> 
> Remove this [1] or mark the following items as [1] [2] [3].
> 
>>
>> The "set txpkts <len0[,len1]*>" was also rejected if there was some stopped or
>> /unconfigured port.
>>
>> This provides the following:
>>
>>   - If fails to get ring size from the port, this can be because port is
>>     not initialized yet, ignore the check and just be sure segment size
>>     won't cause an out of bound access. The port descriptor check will
>>     be done during Tx setup.
>>
>>   - The capability to send single packet is supposed to be very basic
>>     and always supported, the setting segment number to 1 is always
>>     allowed, no check performed
>>
>>   - At the moment of Tx queue setup the descriptor number is checked
>>     against configured segment number
>>
>> Bugzilla ID: 584
>> Fixes: 8dae835d88b7 ("app/testpmd: remove restriction on Tx segments set")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> Cc: Andrew Boyer <aboyer@pensando.io>
>>
>> v2:
>> * Become more flexible for the '--txpkts' command line, if not able to
>>   get the descriptor size from port, ignore the check.
>>
>>   ('nb_txd' check was proposed before, this will require '--txd'
>>   parameter, but also enforces a specific order on the parameters,
>>   instead going with the option to flex the checks for parameter.)
>> ---
>>  app/test-pmd/cmdline.c |  4 ++++
>>  app/test-pmd/config.c  | 32 ++++++++++++++++++++++++--------
>>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> Except the one comment above for commit log,
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> 

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


Above missing reference in the commit log removed while merging.