diff mbox series

[v5,2/2] app/testpmd: fix txonly forwording

Message ID 20210923014951.12696-2-alvinx.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v5,1/2] app/testpmd: update forward engine beginning |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Alvin Zhang Sept. 23, 2021, 1:49 a.m. UTC 
  When random number of Tx segments is enabled, because the actual
number of segments may be only one, the first segment of the Tx
packets must accommodate a complete being sending Eth/IP/UDP packet.

Besides, if multiple flow is enabled, the forwarding will update
the IP and UDP header, these headers shouldn't cross segments.
This also requires the first Tx segment can accommodate a complete
Eth/IP/UDP packet.

In addition, if time stamp is enabled, the forwarding needs more
Tx segment space for time stamp information.

This patch adds checks in beginning of forward engine to make sure
all above conditions are met.

Bugzilla ID: 797
Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
---

v5: fixes a compilation issue
---
 app/test-pmd/txonly.c | 67 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 12 deletions(-)

Comments

Ivan Malov Sept. 23, 2021, 4:25 a.m. UTC | #1 
Hi Alvin,

There's a typo in the commit summary: forwording -> forwarding.

On 23/09/2021 04:49, Alvin Zhang wrote:
> When random number of Tx segments is enabled, because the actual
> number of segments may be only one, the first segment of the Tx
> packets must accommodate a complete being sending Eth/IP/UDP packet.
> 
> Besides, if multiple flow is enabled, the forwarding will update
> the IP and UDP header, these headers shouldn't cross segments.
> This also requires the first Tx segment can accommodate a complete
> Eth/IP/UDP packet.
> 
> In addition, if time stamp is enabled, the forwarding needs more
> Tx segment space for time stamp information.
> 
> This patch adds checks in beginning of forward engine to make sure
> all above conditions are met.
> 
> Bugzilla ID: 797
> Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing packets")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> ---
> 
> v5: fixes a compilation issue
> ---
>   app/test-pmd/txonly.c | 67 ++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 55 insertions(+), 12 deletions(-)
> 
> diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
> index 386a4ff..e7b1b42 100644
> --- a/app/test-pmd/txonly.c
> +++ b/app/test-pmd/txonly.c
> @@ -40,6 +40,13 @@
>   
>   #include "testpmd.h"
>   
> +struct tx_timestamp {
> +	rte_be32_t signature;
> +	rte_be16_t pkt_idx;
> +	rte_be16_t queue_idx;
> +	rte_be64_t ts;
> +};
> +
>   /* use RFC863 Discard Protocol */
>   uint16_t tx_udp_src_port = 9;
>   uint16_t tx_udp_dst_port = 9;
> @@ -257,12 +264,7 @@
>   
>   	if (unlikely(timestamp_enable)) {
>   		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
> -		struct {
> -			rte_be32_t signature;
> -			rte_be16_t pkt_idx;
> -			rte_be16_t queue_idx;
> -			rte_be64_t ts;
> -		} timestamp_mark;
> +		struct tx_timestamp timestamp_mark;
>   
>   		if (unlikely(timestamp_init_req !=
>   				RTE_PER_LCORE(timestamp_idone))) {
> @@ -438,13 +440,23 @@
>   static int
>   tx_only_begin(portid_t pi)
>   {
> -	uint16_t pkt_data_len;
> +	uint16_t pkt_hdr_len, pkt_data_len;
>   	int dynf;
>   
> -	pkt_data_len = (uint16_t) (tx_pkt_length - (
> -					sizeof(struct rte_ether_hdr) +
> -					sizeof(struct rte_ipv4_hdr) +
> -					sizeof(struct rte_udp_hdr)));
> +	pkt_hdr_len = (uint16_t)(sizeof(struct rte_ether_hdr) +
> +				 sizeof(struct rte_ipv4_hdr) +
> +				 sizeof(struct rte_udp_hdr));
> +	pkt_data_len = tx_pkt_length - pkt_hdr_len;
> +
> +	if ((tx_pkt_split == TX_PKT_SPLIT_RND || txonly_multi_flow) &&
> +	    tx_pkt_seg_lengths[0] < pkt_hdr_len) {
> +		TESTPMD_LOG(ERR,
> +			    "Random segment number or multiple flow enabled,"
> +			    " but tx_pkt_seg_lengths[0] %u < %u (needed)\n",

This should probably be on a single line:

TESTPMD_LOG(ERR, "Random segment number or multiple flow enabled, but 
tx_pkt_seg_lengths[0] %u < %u (needed)\n",

because this way it's more search-friendly. Style checks should be OK 
with that.

> +			    tx_pkt_seg_lengths[0], pkt_hdr_len);
> +		return -EINVAL;
> +	}
> +
>   	setup_pkt_udp_ip_headers(&pkt_ip_hdr, &pkt_udp_hdr, pkt_data_len);
>   
>   	timestamp_enable = false;
> @@ -463,8 +475,39 @@
>   			   timestamp_mask &&
>   			   timestamp_off >= 0 &&
>   			   !rte_eth_read_clock(pi, &timestamp_initial[pi]);
> -	if (timestamp_enable)
> +
> +	if (timestamp_enable) {
> +		pkt_hdr_len += sizeof(struct tx_timestamp);
> +
> +		if (tx_pkt_split == TX_PKT_SPLIT_RND) {
> +			if (tx_pkt_seg_lengths[0] < pkt_hdr_len) {
> +				TESTPMD_LOG(ERR,
> +					    "Time stamp and random segment number enabled,"
> +					    " but tx_pkt_seg_lengths[0] %u < %u (needed)\n",

Likewise.

> +					    tx_pkt_seg_lengths[0], pkt_hdr_len);
> +				return -EINVAL;
> +			}
> +		} else {
> +			uint16_t total = 0;
> +			uint8_t i;
> +
> +			for (i = 0; i < tx_pkt_nb_segs; i++) {
> +				total += tx_pkt_seg_lengths[i];
> +				if (total >= pkt_hdr_len)
> +					break;
> +			}
> +
> +			if (total < pkt_hdr_len) {
> +				TESTPMD_LOG(ERR,
> +					    "No enough Tx segment space for time stamp info."
> +					    " total %u < %u (needed)\n",

Likewise.

> +					    total, pkt_hdr_len);
> +				return -EINVAL;
> +			}
> +		}
>   		timestamp_init_req++;
> +	}
> +
>   	/* Make sure all settings are visible on forwarding cores.*/
>   	rte_wmb();
>   	return 0;
>
Alvin Zhang Sept. 23, 2021, 5:11 a.m. UTC | #2 
> -----Original Message-----
> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
> Sent: Thursday, September 23, 2021 12:26 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v5 2/2] app/testpmd: fix txonly forwording
> 
> Hi Alvin,
> 
> There's a typo in the commit summary: forwording -> forwarding.

I'll update it in V6.

> 
> On 23/09/2021 04:49, Alvin Zhang wrote:
> > When random number of Tx segments is enabled, because the actual
> > number of segments may be only one, the first segment of the Tx
> > packets must accommodate a complete being sending Eth/IP/UDP packet.
> >
> > Besides, if multiple flow is enabled, the forwarding will update the
> > IP and UDP header, these headers shouldn't cross segments.
> > This also requires the first Tx segment can accommodate a complete
> > Eth/IP/UDP packet.
> >
> > In addition, if time stamp is enabled, the forwarding needs more Tx
> > segment space for time stamp information.
> >
> > This patch adds checks in beginning of forward engine to make sure all
> > above conditions are met.
> >
> > Bugzilla ID: 797
> > Fixes: 79bec05b32b7 ("app/testpmd: add ability to split outgoing
> > packets")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > ---
> >
> > v5: fixes a compilation issue
> > ---
> >   app/test-pmd/txonly.c | 67
> ++++++++++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 55 insertions(+), 12 deletions(-)
> >
> > diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c index
> > 386a4ff..e7b1b42 100644
> > --- a/app/test-pmd/txonly.c
> > +++ b/app/test-pmd/txonly.c
> > @@ -40,6 +40,13 @@
> >
> >   #include "testpmd.h"
> >
> > +struct tx_timestamp {
> > +	rte_be32_t signature;
> > +	rte_be16_t pkt_idx;
> > +	rte_be16_t queue_idx;
> > +	rte_be64_t ts;
> > +};
> > +
> >   /* use RFC863 Discard Protocol */
> >   uint16_t tx_udp_src_port = 9;
> >   uint16_t tx_udp_dst_port = 9;
> > @@ -257,12 +264,7 @@
> >
> >   	if (unlikely(timestamp_enable)) {
> >   		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
> > -		struct {
> > -			rte_be32_t signature;
> > -			rte_be16_t pkt_idx;
> > -			rte_be16_t queue_idx;
> > -			rte_be64_t ts;
> > -		} timestamp_mark;
> > +		struct tx_timestamp timestamp_mark;
> >
> >   		if (unlikely(timestamp_init_req !=
> >   				RTE_PER_LCORE(timestamp_idone))) { @@ -438,13
> +440,23 @@
> >   static int
> >   tx_only_begin(portid_t pi)
> >   {
> > -	uint16_t pkt_data_len;
> > +	uint16_t pkt_hdr_len, pkt_data_len;
> >   	int dynf;
> >
> > -	pkt_data_len = (uint16_t) (tx_pkt_length - (
> > -					sizeof(struct rte_ether_hdr) +
> > -					sizeof(struct rte_ipv4_hdr) +
> > -					sizeof(struct rte_udp_hdr)));
> > +	pkt_hdr_len = (uint16_t)(sizeof(struct rte_ether_hdr) +
> > +				 sizeof(struct rte_ipv4_hdr) +
> > +				 sizeof(struct rte_udp_hdr));
> > +	pkt_data_len = tx_pkt_length - pkt_hdr_len;
> > +
> > +	if ((tx_pkt_split == TX_PKT_SPLIT_RND || txonly_multi_flow) &&
> > +	    tx_pkt_seg_lengths[0] < pkt_hdr_len) {
> > +		TESTPMD_LOG(ERR,
> > +			    "Random segment number or multiple flow enabled,"
> > +			    " but tx_pkt_seg_lengths[0] %u < %u (needed)\n",
> 
> This should probably be on a single line:
> 
> TESTPMD_LOG(ERR, "Random segment number or multiple flow enabled, but
> tx_pkt_seg_lengths[0] %u < %u (needed)\n",
> 
> because this way it's more search-friendly. Style checks should be OK with that.

I'll update them in V6.

> 
> > +			    tx_pkt_seg_lengths[0], pkt_hdr_len);
> > +		return -EINVAL;
> > +	}
> > +
> >   	setup_pkt_udp_ip_headers(&pkt_ip_hdr, &pkt_udp_hdr,
> pkt_data_len);
> >
> >   	timestamp_enable = false;
> > @@ -463,8 +475,39 @@
> >   			   timestamp_mask &&
> >   			   timestamp_off >= 0 &&
> >   			   !rte_eth_read_clock(pi, &timestamp_initial[pi]);
> > -	if (timestamp_enable)
> > +
> > +	if (timestamp_enable) {
> > +		pkt_hdr_len += sizeof(struct tx_timestamp);
> > +
> > +		if (tx_pkt_split == TX_PKT_SPLIT_RND) {
> > +			if (tx_pkt_seg_lengths[0] < pkt_hdr_len) {
> > +				TESTPMD_LOG(ERR,
> > +					    "Time stamp and random segment number
> enabled,"
> > +					    " but tx_pkt_seg_lengths[0] %u < %u (needed)\n",
> 
> Likewise.
> 
> > +					    tx_pkt_seg_lengths[0], pkt_hdr_len);
> > +				return -EINVAL;
> > +			}
> > +		} else {
> > +			uint16_t total = 0;
> > +			uint8_t i;
> > +
> > +			for (i = 0; i < tx_pkt_nb_segs; i++) {
> > +				total += tx_pkt_seg_lengths[i];
> > +				if (total >= pkt_hdr_len)
> > +					break;
> > +			}
> > +
> > +			if (total < pkt_hdr_len) {
> > +				TESTPMD_LOG(ERR,
> > +					    "No enough Tx segment space for time stamp
> info."
> > +					    " total %u < %u (needed)\n",
> 
> Likewise.
> 
> > +					    total, pkt_hdr_len);
> > +				return -EINVAL;
> > +			}
> > +		}
> >   		timestamp_init_req++;
> > +	}
> > +
> >   	/* Make sure all settings are visible on forwarding cores.*/
> >   	rte_wmb();
> >   	return 0;
> >
> 
> --
> Ivan M
diff mbox series

Patch

diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
index 386a4ff..e7b1b42 100644
--- a/app/test-pmd/txonly.c
+++ b/app/test-pmd/txonly.c
@@ -40,6 +40,13 @@ 
 
 #include "testpmd.h"
 
+struct tx_timestamp {
+	rte_be32_t signature;
+	rte_be16_t pkt_idx;
+	rte_be16_t queue_idx;
+	rte_be64_t ts;
+};
+
 /* use RFC863 Discard Protocol */
 uint16_t tx_udp_src_port = 9;
 uint16_t tx_udp_dst_port = 9;
@@ -257,12 +264,7 @@ 
 
 	if (unlikely(timestamp_enable)) {
 		uint64_t skew = RTE_PER_LCORE(timestamp_qskew);
-		struct {
-			rte_be32_t signature;
-			rte_be16_t pkt_idx;
-			rte_be16_t queue_idx;
-			rte_be64_t ts;
-		} timestamp_mark;
+		struct tx_timestamp timestamp_mark;
 
 		if (unlikely(timestamp_init_req !=
 				RTE_PER_LCORE(timestamp_idone))) {
@@ -438,13 +440,23 @@ 
 static int
 tx_only_begin(portid_t pi)
 {
-	uint16_t pkt_data_len;
+	uint16_t pkt_hdr_len, pkt_data_len;
 	int dynf;
 
-	pkt_data_len = (uint16_t) (tx_pkt_length - (
-					sizeof(struct rte_ether_hdr) +
-					sizeof(struct rte_ipv4_hdr) +
-					sizeof(struct rte_udp_hdr)));
+	pkt_hdr_len = (uint16_t)(sizeof(struct rte_ether_hdr) +
+				 sizeof(struct rte_ipv4_hdr) +
+				 sizeof(struct rte_udp_hdr));
+	pkt_data_len = tx_pkt_length - pkt_hdr_len;
+
+	if ((tx_pkt_split == TX_PKT_SPLIT_RND || txonly_multi_flow) &&
+	    tx_pkt_seg_lengths[0] < pkt_hdr_len) {
+		TESTPMD_LOG(ERR,
+			    "Random segment number or multiple flow enabled,"
+			    " but tx_pkt_seg_lengths[0] %u < %u (needed)\n",
+			    tx_pkt_seg_lengths[0], pkt_hdr_len);
+		return -EINVAL;
+	}
+
 	setup_pkt_udp_ip_headers(&pkt_ip_hdr, &pkt_udp_hdr, pkt_data_len);
 
 	timestamp_enable = false;
@@ -463,8 +475,39 @@ 
 			   timestamp_mask &&
 			   timestamp_off >= 0 &&
 			   !rte_eth_read_clock(pi, &timestamp_initial[pi]);
-	if (timestamp_enable)
+
+	if (timestamp_enable) {
+		pkt_hdr_len += sizeof(struct tx_timestamp);
+
+		if (tx_pkt_split == TX_PKT_SPLIT_RND) {
+			if (tx_pkt_seg_lengths[0] < pkt_hdr_len) {
+				TESTPMD_LOG(ERR,
+					    "Time stamp and random segment number enabled,"
+					    " but tx_pkt_seg_lengths[0] %u < %u (needed)\n",
+					    tx_pkt_seg_lengths[0], pkt_hdr_len);
+				return -EINVAL;
+			}
+		} else {
+			uint16_t total = 0;
+			uint8_t i;
+
+			for (i = 0; i < tx_pkt_nb_segs; i++) {
+				total += tx_pkt_seg_lengths[i];
+				if (total >= pkt_hdr_len)
+					break;
+			}
+
+			if (total < pkt_hdr_len) {
+				TESTPMD_LOG(ERR,
+					    "No enough Tx segment space for time stamp info."
+					    " total %u < %u (needed)\n",
+					    total, pkt_hdr_len);
+				return -EINVAL;
+			}
+		}
 		timestamp_init_req++;
+	}
+
 	/* Make sure all settings are visible on forwarding cores.*/
 	rte_wmb();
 	return 0;