From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C8793A0542; Fri, 11 Nov 2022 09:36:35 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7CDE840697; Fri, 11 Nov 2022 09:36:35 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3E46E4014F for ; Fri, 11 Nov 2022 09:36:33 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 9AEAA7D; Fri, 11 Nov 2022 11:36:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 9AEAA7D DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1668155792; bh=DAS47SsG5UeUhbPiYozWfCgxh2kAxMsBQG+v22RSWRI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=W/gQP3Nm8W6j21hcZVn6Y19qVZDqzOT2cWB1AdZLt6G9GoFDM2MThAw9bdC/1krLa GVYlW3B+CHYizSHeo5OrQbNo4t2Z1+bfdVSOV822qI3v3GeSsEjOkzRZAW9oIGxs4R opzYQTp0syO4OXhqjQofLyAAx2B4c6it55lmUyaw= Message-ID: Date: Fri, 11 Nov 2022 11:36:32 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 Subject: Re: [PATCH 1/2] app/testpmd: prepare to support TCP in Tx only mode Content-Language: en-US To: Ferruh Yigit , Aman Singh , Yuying Zhang Cc: dev@dpdk.org, Georgiy Levashov , Ivan Ilchenko References: <20221017144133.1899052-1-andrew.rybchenko@oktetlabs.ru> <20221017144133.1899052-2-andrew.rybchenko@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >> Signed-off-by: Ivan Ilchenko >> Signed-off-by: Andrew Rybchenko >> --- >>   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(×tamp_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; > ` >