[v10,07/10] app/testpmd: replace POSIX specific code

Message ID 1620088449-14770-8-git-send-email-jizh@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series app/testpmd: enable testpmd on Windows |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Jie Zhou May 4, 2021, 12:34 a.m. UTC
  - Make printf format OS independent
 - Replace htons with RTE_BE16
 - Replace POSIX specific inet_aton with OS independent inet_pton
 - Replace sleep with rte_delay_us_sleep
 - Repalce random with rte_rand
 - #ifndef mman related code for now

Signed-off-by: Jie Zhou <jizh@microsoft.com>
Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
---
 app/test-pmd/cmdline.c     |  3 +--
 app/test-pmd/csumonly.c    |  2 +-
 app/test-pmd/icmpecho.c    |  4 ++--
 app/test-pmd/ieee1588fwd.c |  8 ++++----
 app/test-pmd/parameters.c  | 10 +++++++---
 app/test-pmd/testpmd.c     | 21 ++++++++++++++++++++-
 app/test-pmd/testpmd.h     |  2 +-
 7 files changed, 36 insertions(+), 14 deletions(-)
  

Comments

Tal Shnaiderman May 5, 2021, 8:34 a.m. UTC | #1
> Subject: [PATCH v10 07/10] app/testpmd: replace POSIX specific code
> 
> External email: Use caution opening links or attachments
> 
> 
>  - Make printf format OS independent
>  - Replace htons with RTE_BE16
>  - Replace POSIX specific inet_aton with OS independent inet_pton
>  - Replace sleep with rte_delay_us_sleep
>  - Repalce random with rte_rand
>  - #ifndef mman related code for now

Do you think we can reduce the number of ifndefs by using the functions rte_mem_map, rte_mem_lock?

> 
> Signed-off-by: Jie Zhou <jizh@microsoft.com>
> Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> ---
>  app/test-pmd/cmdline.c     |  3 +--
>  app/test-pmd/csumonly.c    |  2 +-
>  app/test-pmd/icmpecho.c    |  4 ++--
>  app/test-pmd/ieee1588fwd.c |  8 ++++----  app/test-pmd/parameters.c  |
> 10 +++++++---
>  app/test-pmd/testpmd.c     | 21 ++++++++++++++++++++-
>  app/test-pmd/testpmd.h     |  2 +-
>  7 files changed, 36 insertions(+), 14 deletions(-)
> 

<snip>

> @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
>                 }
>         }
> 
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  err:
>         if (rte_mp == NULL) {
>                 rte_exit(EXIT_FAILURE,
> @@ -1065,6 +1077,7 @@ mbuf_pool_create(uint16_t mbuf_seg_size,
> unsigned nb_mbuf,
>         } else if (verbose_level > 0) {
>                 rte_mempool_dump(stdout, rte_mp);
>         }
> +#endif

Why do we need the err flow ifndef'ed?  Isn't it needed in the MP_ALLOC_NATIVE case?

>         return rte_mp;
>  }
>
  
Jie Zhou May 5, 2021, 4:09 p.m. UTC | #2
On Wed, May 05, 2021 at 08:34:34AM +0000, Tal Shnaiderman wrote:
> > Subject: [PATCH v10 07/10] app/testpmd: replace POSIX specific code
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> >  - Make printf format OS independent
> >  - Replace htons with RTE_BE16
> >  - Replace POSIX specific inet_aton with OS independent inet_pton
> >  - Replace sleep with rte_delay_us_sleep
> >  - Repalce random with rte_rand
> >  - #ifndef mman related code for now
> 
> Do you think we can reduce the number of ifndefs by using the functions rte_mem_map, rte_mem_lock?

I used rte_mem_xxx APIs in V3. DmitryK pointed out that Apps except unit tests should not use these internal APIs, and also rte_mem_map does not support hugepage with different sized. Given that, decided to work on the mman related part in a future separate patch.

> 
> > 
> > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > ---
> >  app/test-pmd/cmdline.c     |  3 +--
> >  app/test-pmd/csumonly.c    |  2 +-
> >  app/test-pmd/icmpecho.c    |  4 ++--
> >  app/test-pmd/ieee1588fwd.c |  8 ++++----  app/test-pmd/parameters.c  |
> > 10 +++++++---
> >  app/test-pmd/testpmd.c     | 21 ++++++++++++++++++++-
> >  app/test-pmd/testpmd.h     |  2 +-
> >  7 files changed, 36 insertions(+), 14 deletions(-)
> > 
> 
> <snip>
> 
> > @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
> >                 }
> >         }
> > 
> > +#ifndef RTE_EXEC_ENV_WINDOWS
> >  err:
> >         if (rte_mp == NULL) {
> >                 rte_exit(EXIT_FAILURE,
> > @@ -1065,6 +1077,7 @@ mbuf_pool_create(uint16_t mbuf_seg_size,
> > unsigned nb_mbuf,
> >         } else if (verbose_level > 0) {
> >                 rte_mempool_dump(stdout, rte_mp);
> >         }
> > +#endif
> 
> Why do we need the err flow ifndef'ed?  Isn't it needed in the MP_ALLOC_NATIVE case?

err: is not reachable from the MP_ALLOC_NATIVE.

> >         return rte_mp;
> >  }
> >
  
Jie Zhou May 5, 2021, 4:41 p.m. UTC | #3
On Wed, May 05, 2021 at 09:09:20AM -0700, Jie Zhou wrote:
> On Wed, May 05, 2021 at 08:34:34AM +0000, Tal Shnaiderman wrote:
> > > Subject: [PATCH v10 07/10] app/testpmd: replace POSIX specific code
> > > 
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > >  - Make printf format OS independent
> > >  - Replace htons with RTE_BE16
> > >  - Replace POSIX specific inet_aton with OS independent inet_pton
> > >  - Replace sleep with rte_delay_us_sleep
> > >  - Repalce random with rte_rand
> > >  - #ifndef mman related code for now
> > 
> > Do you think we can reduce the number of ifndefs by using the functions rte_mem_map, rte_mem_lock?
> 
> I used rte_mem_xxx APIs in V3. DmitryK pointed out that Apps except unit tests should not use these internal APIs, and also rte_mem_map does not support hugepage with different sized. Given that, decided to work on the mman related part in a future separate patch.
> 
> > 
> > > 
> > > Signed-off-by: Jie Zhou <jizh@microsoft.com>
> > > Signed-off-by: Jie Zhou <jizh@linux.microsoft.com>
> > > ---
> > >  app/test-pmd/cmdline.c     |  3 +--
> > >  app/test-pmd/csumonly.c    |  2 +-
> > >  app/test-pmd/icmpecho.c    |  4 ++--
> > >  app/test-pmd/ieee1588fwd.c |  8 ++++----  app/test-pmd/parameters.c  |
> > > 10 +++++++---
> > >  app/test-pmd/testpmd.c     | 21 ++++++++++++++++++++-
> > >  app/test-pmd/testpmd.h     |  2 +-
> > >  7 files changed, 36 insertions(+), 14 deletions(-)
> > > 
> > 
> > <snip>
> > 
> > > @@ mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
> > >                 }
> > >         }
> > > 
> > > +#ifndef RTE_EXEC_ENV_WINDOWS
> > >  err:
> > >         if (rte_mp == NULL) {
> > >                 rte_exit(EXIT_FAILURE,
> > > @@ -1065,6 +1077,7 @@ mbuf_pool_create(uint16_t mbuf_seg_size,
> > > unsigned nb_mbuf,
> > >         } else if (verbose_level > 0) {
> > >                 rte_mempool_dump(stdout, rte_mp);
> > >         }
> > > +#endif
> > 
> > Why do we need the err flow ifndef'ed?  Isn't it needed in the MP_ALLOC_NATIVE case?
> 

You are right. Will fix in V11. To avoid the unused err: lable warning, just need to ifndef the err: lable instead of the whole err flow. My mistake.

> 
> > >         return rte_mp;
> > >  }
> > >
  

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index d804ee233..f0636ca9b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -8,7 +8,6 @@ 
 #include <stdio.h>
 #include <stdint.h>
 #include <string.h>
-#include <termios.h>
 #include <unistd.h>
 #include <inttypes.h>
 #include <sys/socket.h>
@@ -3601,7 +3600,7 @@  cmdline_parse_inst_t cmd_stop = {
 /* *** SET CORELIST and PORTLIST CONFIGURATION *** */
 
 unsigned int
-parse_item_list(char* str, const char* item_name, unsigned int max_items,
+parse_item_list(const char *str, const char *item_name, unsigned int max_items,
 		unsigned int *parsed_items, int check_unique_values)
 {
 	unsigned int nb_item;
diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 6b4df335f..089936587 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -696,7 +696,7 @@  pkt_copy_split(const struct rte_mbuf *pkt)
 	mp = current_fwd_lcore()->mbp;
 
 	if (tx_pkt_split == TX_PKT_SPLIT_RND)
-		nb_seg = random() % tx_pkt_nb_segs + 1;
+		nb_seg = rte_rand() % tx_pkt_nb_segs + 1;
 	else
 		nb_seg = tx_pkt_nb_segs;
 
diff --git a/app/test-pmd/icmpecho.c b/app/test-pmd/icmpecho.c
index af6f7e790..8948f28eb 100644
--- a/app/test-pmd/icmpecho.c
+++ b/app/test-pmd/icmpecho.c
@@ -474,8 +474,8 @@  reply_to_icmp_echo_rqsts(struct fwd_stream *fs)
 		}
 		icmp_h->icmp_type = RTE_IP_ICMP_ECHO_REPLY;
 		cksum = ~icmp_h->icmp_cksum & 0xffff;
-		cksum += ~htons(RTE_IP_ICMP_ECHO_REQUEST << 8) & 0xffff;
-		cksum += htons(RTE_IP_ICMP_ECHO_REPLY << 8);
+		cksum += ~RTE_BE16(RTE_IP_ICMP_ECHO_REQUEST << 8) & 0xffff;
+		cksum += RTE_BE16(RTE_IP_ICMP_ECHO_REPLY << 8);
 		cksum = (cksum & 0xffff) + (cksum >> 16);
 		cksum = (cksum & 0xffff) + (cksum >> 16);
 		icmp_h->icmp_cksum = ~cksum;
diff --git a/app/test-pmd/ieee1588fwd.c b/app/test-pmd/ieee1588fwd.c
index e3b98e3e0..034f238c3 100644
--- a/app/test-pmd/ieee1588fwd.c
+++ b/app/test-pmd/ieee1588fwd.c
@@ -60,8 +60,8 @@  port_ieee1588_rx_timestamp_check(portid_t pi, uint32_t index)
 		printf("Port %u RX timestamp registers not valid\n", pi);
 		return;
 	}
-	printf("Port %u RX timestamp value %lu s %lu ns\n",
-		pi, timestamp.tv_sec, timestamp.tv_nsec);
+	printf("Port %u RX timestamp value %ju s %lu ns\n",
+		pi, (uintmax_t)timestamp.tv_sec, timestamp.tv_nsec);
 }
 
 #define MAX_TX_TMST_WAIT_MICROSECS 1000 /**< 1 milli-second */
@@ -83,9 +83,9 @@  port_ieee1588_tx_timestamp_check(portid_t pi)
 		       pi, MAX_TX_TMST_WAIT_MICROSECS);
 		return;
 	}
-	printf("Port %u TX timestamp value %lu s %lu ns validated after "
+	printf("Port %u TX timestamp value %ju s %lu ns validated after "
 	       "%u micro-second%s\n",
-	       pi, timestamp.tv_sec, timestamp.tv_nsec, wait_us,
+	       pi, (uintmax_t)timestamp.tv_sec, timestamp.tv_nsec, wait_us,
 	       (wait_us == 1) ? "" : "s");
 }
 
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index f3954c1c6..4c3cbbac3 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -185,8 +185,10 @@  usage(char* progname)
 	printf("  --hot-plug: enable hot plug for device.\n");
 	printf("  --vxlan-gpe-port=N: UPD port of tunnel VXLAN-GPE\n");
 	printf("  --geneve-parsed-port=N: UPD port to parse GENEVE tunnel protocol\n");
+#ifndef RTE_EXEC_ENV_WINDOWS
 	printf("  --mlockall: lock all memory\n");
 	printf("  --no-mlockall: do not lock all memory\n");
+#endif
 	printf("  --mp-alloc <native|anon|xmem|xmemhuge>: mempool allocation method.\n"
 	       "    native: use regular DPDK memory to create and populate mempool\n"
 	       "    anon: use regular DPDK memory to create and anonymous memory to populate mempool\n"
@@ -211,7 +213,7 @@  usage(char* progname)
 
 #ifdef RTE_LIB_CMDLINE
 static int
-init_peer_eth_addrs(char *config_filename)
+init_peer_eth_addrs(const char *config_filename)
 {
 	FILE *config_file;
 	portid_t i;
@@ -610,8 +612,10 @@  launch_args_parse(int argc, char** argv)
 		{ "hot-plug",			0, 0, 0 },
 		{ "vxlan-gpe-port",		1, 0, 0 },
 		{ "geneve-parsed-port",		1, 0, 0 },
+#ifndef RTE_EXEC_ENV_WINDOWS
 		{ "mlockall",			0, 0, 0 },
 		{ "no-mlockall",		0, 0, 0 },
+#endif
 		{ "mp-alloc",			1, 0, 0 },
 		{ "tx-ip",			1, 0, 0 },
 		{ "tx-udp",			1, 0, 0 },
@@ -723,13 +727,13 @@  launch_args_parse(int argc, char** argv)
 						 "Invalid tx-ip: %s", optarg);
 
 				*end++ = 0;
-				if (inet_aton(optarg, &in) == 0)
+				if (inet_pton(AF_INET, optarg, &in) == 0)
 					rte_exit(EXIT_FAILURE,
 						 "Invalid source IP address: %s\n",
 						 optarg);
 				tx_ip_src_addr = rte_be_to_cpu_32(in.s_addr);
 
-				if (inet_aton(end, &in) == 0)
+				if (inet_pton(AF_INET, end, &in) == 0)
 					rte_exit(EXIT_FAILURE,
 						 "Invalid destination IP address: %s\n",
 						 optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index d4be23f8f..d4a9f5b6b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -9,7 +9,9 @@ 
 #include <string.h>
 #include <time.h>
 #include <fcntl.h>
+#ifndef RTE_EXEC_ENV_WINDOWS
 #include <sys/mman.h>
+#endif
 #include <sys/types.h>
 #include <errno.h>
 #include <stdbool.h>
@@ -60,6 +62,9 @@ 
 #ifdef RTE_LIB_LATENCYSTATS
 #include <rte_latencystats.h>
 #endif
+#ifdef RTE_EXEC_ENV_WINDOWS
+#include <process.h>
+#endif
 
 #include "testpmd.h"
 
@@ -632,6 +637,7 @@  set_def_fwd_config(void)
 	set_default_fwd_ports_config();
 }
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 /* extremely pessimistic estimation of memory required to create a mempool */
 static int
 calc_mem_size(uint32_t nb_mbufs, uint32_t mbuf_sz, size_t pgsz, size_t *out)
@@ -902,6 +908,7 @@  dma_map_cb(struct rte_mempool *mp __rte_unused, void *opaque __rte_unused,
 		}
 	}
 }
+#endif
 
 static unsigned int
 setup_extbuf(uint32_t nb_mbufs, uint16_t mbuf_sz, unsigned int socket_id,
@@ -972,9 +979,11 @@  mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 {
 	char pool_name[RTE_MEMPOOL_NAMESIZE];
 	struct rte_mempool *rte_mp = NULL;
+#ifndef RTE_EXEC_ENV_WINDOWS
 	uint32_t mb_size;
 
 	mb_size = sizeof(struct rte_mbuf) + mbuf_seg_size;
+#endif
 	mbuf_poolname_build(socket_id, pool_name, sizeof(pool_name), size_idx);
 
 	TESTPMD_LOG(INFO,
@@ -991,6 +1000,7 @@  mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 				mb_mempool_cache, 0, mbuf_seg_size, socket_id);
 			break;
 		}
+#ifndef RTE_EXEC_ENV_WINDOWS
 	case MP_ALLOC_ANON:
 		{
 			rte_mp = rte_mempool_create_empty(pool_name, nb_mbuf,
@@ -1031,6 +1041,7 @@  mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 					heap_socket);
 			break;
 		}
+#endif
 	case MP_ALLOC_XBUF:
 		{
 			struct rte_pktmbuf_extmem *ext_mem;
@@ -1057,6 +1068,7 @@  mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 		}
 	}
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 err:
 	if (rte_mp == NULL) {
 		rte_exit(EXIT_FAILURE,
@@ -1065,6 +1077,7 @@  mbuf_pool_create(uint16_t mbuf_seg_size, unsigned nb_mbuf,
 	} else if (verbose_level > 0) {
 		rte_mempool_dump(stdout, rte_mp);
 	}
+#endif
 	return rte_mp;
 }
 
@@ -3055,6 +3068,7 @@  pmd_test_exit(void)
 	if (test_done == 0)
 		stop_packet_forwarding();
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 	for (i = 0 ; i < RTE_DIM(mempools) ; i++) {
 		if (mempools[i]) {
 			if (mp_alloc_type == MP_ALLOC_ANON)
@@ -3062,6 +3076,7 @@  pmd_test_exit(void)
 						     NULL);
 		}
 	}
+#endif
 	if (ports != NULL) {
 		no_link_check = 1;
 		RTE_ETH_FOREACH_DEV(pt_id) {
@@ -3761,8 +3776,10 @@  signal_handler(int signum)
 		/* Set flag to indicate the force termination. */
 		f_quit = 1;
 		/* exit with the expected status */
+#ifndef RTE_EXEC_ENV_WINDOWS
 		signal(signum, SIG_DFL);
 		kill(getpid(), signum);
+#endif
 	}
 }
 
@@ -3837,10 +3854,12 @@  main(int argc, char** argv)
 	if (argc > 1)
 		launch_args_parse(argc, argv);
 
+#ifndef RTE_EXEC_ENV_WINDOWS
 	if (do_mlockall && mlockall(MCL_CURRENT | MCL_FUTURE)) {
 		TESTPMD_LOG(NOTICE, "mlockall() failed with error \"%s\"\n",
 			strerror(errno));
 	}
+#endif
 
 	if (tx_first && interactive)
 		rte_exit(EXIT_FAILURE, "--tx-first cannot be used on "
@@ -3961,7 +3980,7 @@  main(int argc, char** argv)
 				}
 				/* Sleep to avoid unnecessary checks */
 				prev_time = cur_time;
-				sleep(1);
+				rte_delay_us_sleep(US_PER_S);
 			}
 		}
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 1d104213b..a4115861b 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -763,7 +763,7 @@  inc_tx_burst_stats(struct fwd_stream *fs, uint16_t nb_tx)
 }
 
 /* Prototypes */
-unsigned int parse_item_list(char* str, const char* item_name,
+unsigned int parse_item_list(const char *str, const char *item_name,
 			unsigned int max_items,
 			unsigned int *parsed_items, int check_unique_values);
 void launch_args_parse(int argc, char** argv);