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

["Zhang, AlvinX" <alvinx.zhang@intel.com>]

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