Re: [dpdk-dev] [PATCH v5 2/2] app/testpmd: fix txonly forwording

[Ivan Malov <Ivan.Malov@oktetlabs.ru>]

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;
> 

-- 
Ivan M