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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/11/2022 8:36 AM, Andrew Rybchenko wrote:
> 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.
> 

Hi Andrew,

What you said is true for this patch, but the intentions of the patch is 
preparation for TCP support.
So eventually "tx-udp" argument will be used to configure TCP flows, 
which is confusing.

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

Not separate variables, separate testpmd parameter for TCP.

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

I think better to separate them. But I didn't get why it adds not tested 
functionality?

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