Re: [dpdk-dev] [PATCH v2] app/test-pmd: enable testpmd on windows

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

Hi Jie,

General comment: try to avoid #ifdef RTE_EXEC_ENV_WINDOWS except for
inherently non-portable Unix code. It clutters the source and complicates
testing: unless you build on Linux you won't know something broke in code
under #ifdef.
Take device hot-plug for example. Since you disable parameter parsing with
#ifdef, hot_plug variable is always 0. You don't need to disable code guarded
by this variable until it only uses DPDK API. You may need to add stubs to
EAL in a separate commit.

2021-03-19 09:51 (UTC-0700), Jie Zhou:
[...]
> diff --git a/app/meson.build b/app/meson.build
> index 87fc195db..00622933e 100644
> --- a/app/meson.build
> +++ b/app/meson.build
> @@ -1,10 +1,6 @@
>  # SPDX-License-Identifier: BSD-3-Clause
>  # Copyright(c) 2017-2019 Intel Corporation
>  
> -if is_windows
> -	subdir_done()
> -endif
> -
>  apps = [
>  	'pdump',
>  	'proc-info',
> @@ -21,6 +17,11 @@ apps = [
>  	'test-regex',
>  	'test-sad']
>  
> +if is_windows
> +	apps = [
> +		'test-pmd']
> +endif
> +

Can we disable individual apps instead of having two lists,
like Pallavi did in drivers/net directory?

[...]
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 14110eb2e..35a6dd0d3 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -8,12 +8,14 @@
>  #include <stdio.h>
>  #include <stdint.h>
>  #include <string.h>
> +
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  #include <termios.h>
> +#endif

This include is not needed at all: cmdline abstracts termios.


> +#ifdef RTE_EXEC_ENV_WINDOWS
> +#ifndef IPDEFTTL
> +#define IPDEFTTL 64
> +#endif
> +#endif
> +

[...]
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 576d5acab..af7570429 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -38,7 +38,6 @@
>  #include <rte_string_fns.h>
>  #include <rte_cycles.h>
>  #include <rte_flow.h>
> -#include <rte_errno.h>
>  #ifdef RTE_NET_IXGBE
>  #include <rte_pmd_ixgbe.h>
>  #endif
> @@ -170,6 +169,27 @@ print_ethaddr(const char *name, struct rte_ether_addr *eth_addr)
>  	printf("%s%s", name, buf);
>  }
>  
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +static int
> +clock_gettime_monotonic(struct timespec *tp)
> +{
> +	LARGE_INTEGER pf, pc;
> +	LONGLONG nsec;
> +
> +	if (QueryPerformanceFrequency(&pf) == 0)
> +		return -1;
> +
> +	if (QueryPerformanceCounter(&pc) == 0)
> +		return -1;
> +
> +	nsec = pc.QuadPart * NS_PER_SEC / pf.QuadPart;
> +	tp->tv_sec = nsec / NS_PER_SEC;
> +	tp->tv_nsec = nsec - tp->tv_sec * NS_PER_SEC;
> +
> +	return 0;
> +}
> +#endif
> +
>  void
>  nic_stats_display(portid_t port_id)
>  {
> @@ -182,6 +202,7 @@ nic_stats_display(portid_t port_id)
>  	uint64_t diff_pkts_rx, diff_pkts_tx, diff_bytes_rx, diff_bytes_tx,
>  								diff_ns;
>  	uint64_t mpps_rx, mpps_tx, mbps_rx, mbps_tx;
> +	int ret;
>  	struct rte_eth_stats stats;
>  
>  	static const char *nic_stats_border = "########################";
> @@ -202,7 +223,13 @@ nic_stats_display(portid_t port_id)
>  	       "%-"PRIu64"\n", stats.opackets, stats.oerrors, stats.obytes);
>  
>  	diff_ns = 0;
> -	if (clock_gettime(CLOCK_TYPE_ID, &cur_time) == 0) {
> +
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +	ret = clock_gettime_monotonic(&cur_time);
> +#else
> +	ret = clock_gettime(CLOCK_TYPE_ID, &cur_time);
> +#endif
> +	if (ret == 0) {

I suggest to use a single wrapped defined using #ifdef to avoid an #ifdef
each time it is called (although it's the only occurrence for now).

[...]
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index 6b4df335f..0fb03b9f9 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -696,7 +696,11 @@ pkt_copy_split(const struct rte_mbuf *pkt)
>  	mp = current_fwd_lcore()->mbp;
>  
>  	if (tx_pkt_split == TX_PKT_SPLIT_RND)
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  		nb_seg = random() % tx_pkt_nb_segs + 1;
> +#else
> +		nb_seg = rand() % tx_pkt_nb_segs + 1;
> +#endif

Can rte_rand() be exported from EAL and used here?

[...]
>  
> +#ifndef RTE_EXEC_ENV_WINDOWS
>  	ret = rte_eal_cleanup();
>  	if (ret != 0)
>  		rte_exit(EXIT_FAILURE,
>  			 "EAL cleanup failed: %s\n", strerror(-ret));
> -
> +#endif
>  	return EXIT_SUCCESS;
>  }

rte_eal_cleanup() is fixed, this can be removed.