Re: [PATCH 1/2] app/testpmd: prepare to support TCP in Tx only mode

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/19/22 19:39, Ferruh Yigit wrote:
> On 10/17/2022 3:41 PM, Andrew Rybchenko wrote:
>> This is useful for the incoming support of TCP TSO in Tx only mode.
>>
>> Signed-off-by: Georgiy Levashov <georgiy.levashov@oktetlabs.ru>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>>   app/test-pmd/parameters.c |  6 +--
>>   app/test-pmd/testpmd.h    |  4 +-
>>   app/test-pmd/txonly.c     | 98 +++++++++++++++++++++++++--------------
>>   3 files changed, 67 insertions(+), 41 deletions(-)
>>
>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>> index ccb1173d7d..545ebee16b 100644
>> --- a/app/test-pmd/parameters.c
>> +++ b/app/test-pmd/parameters.c
>> @@ -835,7 +835,7 @@ launch_args_parse(int argc, char** argv)
>>                       rte_exit(EXIT_FAILURE,
>>                            "Invalid UDP port: %s\n",
>>                            optarg);
>> -                tx_udp_src_port = n;
>> +                tx_l4_src_port = n;
>>                   if (*end == ',') {
>>                       char *dst = end + 1;
>> @@ -845,9 +845,9 @@ launch_args_parse(int argc, char** argv)
>>                           rte_exit(EXIT_FAILURE,
>>                                "Invalid destination UDP port: %s\n",
>>                                dst);
>> -                    tx_udp_dst_port = n;
>> +                    tx_l4_dst_port = n;
>>                   } else {
>> -                    tx_udp_dst_port = n;
>> +                    tx_l4_dst_port = n;
>>                   }
>>               }
> 
> The argument to set this variable is "tx-udp", so it has explicit 'udp' 
> in the name, so I am not sure to reuse it as L4.
> 

The patch changes nothing externally. UDP is an layer 4
protocol and I see no problems in storage of the the
UDP port in the variable names tx_l4_dst_port.

It is just cosmetics patch with prepares code to
further changes.

> Also documentation [1] and help output [2] explicitly mentions from this 
> as UDP, please check `git grep "tx-udp"` output.
> 

UDP is a layer 4 protocol. The variable could be named
this way from the very beginning.
Do you think we need separate variables for TCP and UDP?
I think no.

> One option is to update the argument name and documentation, even this 
> can break some exitsting test scripts etc..

I think it is a bad option.

> OR
> Can add a new parameter as "tx-tcp", I think this is better because 
> right now next patch uses "txonly-tso-mss" to decide protocol is TCP, 
> instead of using implied parameter "tx-tcp" can clarify that TCP 
> protocol is requested. This also can help to decouple TCP and TSO support.
> What do you think?
> 

We can introduce tx-tcp parameter to decouple TCP and TSO
in a separate patch and it should save the port number to
tx_l4_dst_port variable.
I simply skipped it for now since additoin of not used and
not tested functionality is not good.
If you think that we really need tx-tcp, I'll add it in v2.
Please, confirm.

> 
> [1]
> doc/guides/testpmd_app_ug/run_app.rst
> 
> [2]
> printf("  --tx-udp=src[,dst]: UDP ports in Tx-only mode\n");
> 
>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
>> index acdb7e855d..30915bd84b 100644
>> --- a/app/test-pmd/testpmd.h
>> +++ b/app/test-pmd/testpmd.h
>> @@ -619,8 +619,8 @@ extern int8_t tx_pthresh;
>>   extern int8_t tx_hthresh;
>>   extern int8_t tx_wthresh;
>> -extern uint16_t tx_udp_src_port;
>> -extern uint16_t tx_udp_dst_port;
>> +extern uint16_t tx_l4_src_port;
>> +extern uint16_t tx_l4_dst_port;
>>   extern uint32_t tx_ip_src_addr;
>>   extern uint32_t tx_ip_dst_addr;
>> diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c
>> index 021624952d..44bda752bc 100644
>> --- a/app/test-pmd/txonly.c
>> +++ b/app/test-pmd/txonly.c
>> @@ -46,8 +46,8 @@ struct tx_timestamp {
>>   };
>>   /* use RFC863 Discard Protocol */
>> -uint16_t tx_udp_src_port = 9;
>> -uint16_t tx_udp_dst_port = 9;
>> +uint16_t tx_l4_src_port = 9;
>> +uint16_t tx_l4_dst_port = 9;
>>   /* use RFC5735 / RFC2544 reserved network test addresses */
>>   uint32_t tx_ip_src_addr = (198U << 24) | (18 << 16) | (0 << 8) | 1;
>> @@ -57,7 +57,10 @@ uint32_t tx_ip_dst_addr = (198U << 24) | (18 << 16) 
>> | (0 << 8) | 2;
>>   static struct rte_ipv4_hdr pkt_ip_hdr; /**< IP header of transmitted 
>> packets. */
>>   RTE_DEFINE_PER_LCORE(uint8_t, _ip_var); /**< IP address variation */
>> -static struct rte_udp_hdr pkt_udp_hdr; /**< UDP header of tx packets. */
>> +
>> +static union pkt_l4_hdr_t {
>> +    struct rte_udp_hdr udp;    /**< UDP header of tx packets. */
>> +} pkt_l4_hdr; /**< Layer 4 header of tx packets. */
>>   static uint64_t timestamp_mask; /**< Timestamp dynamic flag mask */
>>   static int32_t timestamp_off; /**< Timestamp dynamic field offset */
>> @@ -102,22 +105,30 @@ copy_buf_to_pkt(void* buf, unsigned len, struct 
>> rte_mbuf *pkt, unsigned offset)
>>   }
>>   static void
>> -setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
>> -             struct rte_udp_hdr *udp_hdr,
>> -             uint16_t pkt_data_len)
>> +setup_pkt_l4_ip_headers(uint8_t ip_proto, struct rte_ipv4_hdr *ip_hdr,
>> +            union pkt_l4_hdr_t *l4_hdr, uint16_t pkt_data_len)
>>   {
>>       uint16_t *ptr16;
>>       uint32_t ip_cksum;
>>       uint16_t pkt_len;
>> +    struct rte_udp_hdr *udp_hdr;
>> -    /*
>> -     * Initialize UDP header.
>> -     */
>> -    pkt_len = (uint16_t) (pkt_data_len + sizeof(struct rte_udp_hdr));
>> -    udp_hdr->src_port = rte_cpu_to_be_16(tx_udp_src_port);
>> -    udp_hdr->dst_port = rte_cpu_to_be_16(tx_udp_dst_port);
>> -    udp_hdr->dgram_len      = RTE_CPU_TO_BE_16(pkt_len);
>> -    udp_hdr->dgram_cksum    = 0; /* No UDP checksum. */
>> +    switch (ip_proto) {
>> +    case IPPROTO_UDP:
>> +        /*
>> +         * Initialize UDP header.
>> +         */
>> +        pkt_len = (uint16_t)(pkt_data_len + sizeof(struct rte_udp_hdr));
>> +        udp_hdr = &l4_hdr->udp;
>> +        udp_hdr->src_port = rte_cpu_to_be_16(tx_l4_src_port);
>> +        udp_hdr->dst_port = rte_cpu_to_be_16(tx_l4_dst_port);
>> +        udp_hdr->dgram_len      = RTE_CPU_TO_BE_16(pkt_len);
>> +        udp_hdr->dgram_cksum    = 0; /* No UDP checksum. */
>> +        break;
>> +    default:
>> +        pkt_len = pkt_data_len;
>> +        break;
>> +    }
>>       /*
>>        * Initialize IP header.
>> @@ -127,7 +138,7 @@ setup_pkt_udp_ip_headers(struct rte_ipv4_hdr *ip_hdr,
>>       ip_hdr->type_of_service   = 0;
>>       ip_hdr->fragment_offset = 0;
>>       ip_hdr->time_to_live   = IP_DEFTTL;
>> -    ip_hdr->next_proto_id = IPPROTO_UDP;
>> +    ip_hdr->next_proto_id = ip_proto;
>>       ip_hdr->packet_id = 0;
>>       ip_hdr->total_length   = RTE_CPU_TO_BE_16(pkt_len);
>>       ip_hdr->src_addr = rte_cpu_to_be_32(tx_ip_src_addr);
>> @@ -162,27 +173,32 @@ 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 ip_data_len;
>>       uint16_t pkt_len;
>> -    pkt_data_len = (uint16_t) (total_pkt_len - (
>> +    ip_data_len = (uint16_t) (total_pkt_len - (
>>                       sizeof(struct rte_ether_hdr) +
>> -                    sizeof(struct rte_ipv4_hdr) +
>> -                    sizeof(struct rte_udp_hdr)));
>> -    /* update UDP packet 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);
>> +                    sizeof(struct rte_ipv4_hdr)));
>>       /* update IP packet length and checksum */
>>       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));
>> +    pkt_len = (uint16_t) (ip_data_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);
>> +
>> +    switch (ip_hdr->next_proto_id) {
>> +    case IPPROTO_UDP:
>> +        /* update UDP packet length */
>> +        udp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_udp_hdr *,
>> +                    sizeof(struct rte_ether_hdr) +
>> +                    sizeof(struct rte_ipv4_hdr));
>> +        udp_hdr->dgram_len = RTE_CPU_TO_BE_16(ip_data_len);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>>   }
>>   static inline bool
>> @@ -193,8 +209,9 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct 
>> rte_mempool *mbp,
>>   {
>>       struct rte_mbuf *pkt_segs[RTE_MAX_SEGS_PER_PKT];
>>       struct rte_mbuf *pkt_seg;
>> -    uint32_t nb_segs, pkt_len;
>> +    uint32_t nb_segs, pkt_len, l4_hdr_size;
>>       uint8_t i;
>> +    struct rte_ipv4_hdr *ip_hdr;
>>       if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND))
>>           nb_segs = rte_rand() % tx_pkt_nb_segs + 1;
>> @@ -230,14 +247,14 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct 
>> rte_mempool *mbp,
>>       copy_buf_to_pkt(eth_hdr, sizeof(*eth_hdr), pkt, 0);
>>       copy_buf_to_pkt(&pkt_ip_hdr, sizeof(pkt_ip_hdr), pkt,
>>               sizeof(struct rte_ether_hdr));
>> +
>> +    ip_hdr = rte_pktmbuf_mtod_offset(pkt,
>> +            struct rte_ipv4_hdr *,
>> +            sizeof(struct rte_ether_hdr));
>>       if (txonly_multi_flow) {
>>           uint8_t  ip_var = RTE_PER_LCORE(_ip_var);
>> -        struct rte_ipv4_hdr *ip_hdr;
>>           uint32_t addr;
>> -        ip_hdr = rte_pktmbuf_mtod_offset(pkt,
>> -                struct rte_ipv4_hdr *,
>> -                sizeof(struct rte_ether_hdr));
>>           /*
>>            * Generate multiple flows by varying IP src addr. This
>>            * enables packets are well distributed by RSS in
>> @@ -249,9 +266,17 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct 
>> rte_mempool *mbp,
>>           ip_hdr->src_addr = rte_cpu_to_be_32(addr);
>>           RTE_PER_LCORE(_ip_var) = ip_var;
>>       }
>> -    copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt,
>> -            sizeof(struct rte_ether_hdr) +
>> -            sizeof(struct rte_ipv4_hdr));
>> +    switch (ip_hdr->next_proto_id) {
>> +    case IPPROTO_UDP:
>> +        copy_buf_to_pkt(&pkt_l4_hdr.udp, sizeof(pkt_l4_hdr.udp), pkt,
>> +                sizeof(struct rte_ether_hdr) +
>> +                sizeof(struct rte_ipv4_hdr));
>> +        l4_hdr_size = sizeof(pkt_l4_hdr.udp);
>> +        break;
>> +    default:
>> +        l4_hdr_size = 0;
>> +        break;
>> +    }
>>       if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND) || 
>> txonly_multi_flow)
>>           update_pkt_header(pkt, pkt_len);
>> @@ -308,7 +333,7 @@ pkt_burst_prepare(struct rte_mbuf *pkt, struct 
>> rte_mempool *mbp,
>>           copy_buf_to_pkt(&timestamp_mark, sizeof(timestamp_mark), pkt,
>>               sizeof(struct rte_ether_hdr) +
>>               sizeof(struct rte_ipv4_hdr) +
>> -            sizeof(pkt_udp_hdr));
>> +            l4_hdr_size);
>>       }
>>       /*
>>        * Complete first mbuf of packet and append it to the
>> @@ -449,7 +474,8 @@ tx_only_begin(portid_t pi)
>>           return -EINVAL;
>>       }
>> -    setup_pkt_udp_ip_headers(&pkt_ip_hdr, &pkt_udp_hdr, pkt_data_len);
>> +    setup_pkt_l4_ip_headers(IPPROTO_UDP, &pkt_ip_hdr, &pkt_l4_hdr,
>> +                pkt_data_len);
> 
> 'pkt_data_len' is calculated as following, it is correct for this patch, 
> but it will be wrong in next patch because UDP header size is used in 
> calculation.
> Need to fix this code, either in this patch and make it protocol 
> agnostic, or in next patch with protocol check.

Again, the goal of the patch is to do cosmetic changes to
prepare to add new functionality in follow up patches.
The patch does not add TCP support. So, I don't understand
how it can be improved here. So, I'll fix the problem in the
next patch when I have TCP support and corresponding branching.
Thanks a lot for the catch.

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