> -----Original Message-----
> From: Ivan Malov <ivan.ma...@oktetlabs.ru>
> Sent: Thursday, September 23, 2021 12:26 PM
> To: Zhang, AlvinX <alvinx.zh...@intel.com>; Li, Xiaoyun
> <xiaoyun...@intel.com>; Ananyev, Konstantin <konstantin.anan...@intel.com>
> Cc: dev@dpdk.org; sta...@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: sta...@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zh...@intel.com>
> > Acked-by: Xiaoyun Li <xiaoyun...@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, ×tamp_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
