[dpdk-dev] [PATCH v3 6/6] app/testpmd: enable testpmd on Windows

[Jie Zhou]

On Tue, Apr 13, 2021 at 03:22:57PM -0700, Jie Zhou wrote:
> On Tue, Apr 13, 2021 at 11:10:00PM +0300, Dmitry Kozlyuk wrote:
> > 2021-04-13 10:19 (UTC-0700), Jie Zhou:
> > [...]
> > > diff --git a/app/meson.build b/app/meson.build
> > > index 50a53dbde..b40b04ca7 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',
> > > @@ -19,7 +15,11 @@ apps = [
> > >  	'test-pipeline',
> > >  	'test-pmd',
> > >  	'test-regex',
> > > -	'test-sad']
> > > +	'test-sad'
> > > +]
> > > +
> > > +# for BSD only
> > > +lib_execinfo = cc.find_library('execinfo', required: false)
> > 
> > Why is this needed?
> 
> Seems the internal fork I worked on is out of sync with the upstream main. Did not add these but just being there. Will remove it.
> 
> > 
> > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> > > index f44116b08..335ed534d 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
> > 
> > You seem to have missed my comment to v2, that #include <termios.h> line can
> > just be removed, because <cmdline_*.h> provide everything necessary.
> 
> Yeah, sorry, seems missed some of your comments. Thanks Dmitry. Will fix in V4.
> 
> > 
> > [...]
> > > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > > index fb7a3a8bd..9945adcf1 100644
> > > --- a/app/test-pmd/cmdline_flow.c
> > > +++ b/app/test-pmd/cmdline_flow.c
> > > @@ -31,6 +31,12 @@
> > >  
> > >  #include "testpmd.h"
> > >  
> > > +#ifdef RTE_EXEC_ENV_WINDOWS
> > > +#ifndef IPDEFTTL
> > > +#define IPDEFTTL 64
> > > +#endif
> > > +#endif
> > > +
> > >  /** Parser token indices. */
> > >  enum index {
> > >  	/* Special tokens. */
> > 
> > IPDEFTTL is used in some other apps and examples.
> > If you follow Tal's advice and base your work on "Do not expose POSIX
> > symbols" series, then please move this to <rte_os_shim.h>.
> 
> I applied "eal/windows:do not expose POSIX symbols" per your and Tal's advices, but hit error LNK2019: unresolved external symbol strdup reference in function log_save_level". I am using clang version 10.0.0.

[Update] Seems the problem is at applying the patch series, even though it showed all sucessfully applied, but somehow in eal_common_log.c the include of <rte_os_shim.h> was not added. After add include rte_os_shim.h, build completes.

> 
> 
> > 
> > > diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
> > > index 7e9c7bdd6..02aea0255 100644
> > > --- a/app/test-pmd/meson.build
> > > +++ b/app/test-pmd/meson.build
> > > @@ -4,6 +4,10 @@
> > >  # override default name to drop the hyphen
> > >  name = 'testpmd'
> > >  cflags += '-Wno-deprecated-declarations'
> > > +
> > > +# Enable using internal APIs in testpmd
> > > +cflags += ['-DALLOW_INTERNAL_API']
> > 
> > Apps except unit tests should not use internal API, which is for DPDK
> > components only. See below about memory mapping API.
> 
> Good to know. Thanks.
> 
> > 
> > [...]
> > > @@ -688,13 +691,11 @@ alloc_mem(size_t memsz, size_t pgsz, bool huge)
> > >  	int flags;
> > >  
> > >  	/* allocate anonymous hugepages */
> > > -	flags = MAP_ANONYMOUS | MAP_PRIVATE;
> > > +	flags = RTE_MAP_ANONYMOUS | RTE_MAP_PRIVATE;
> > >  	if (huge)
> > >  		flags |= HUGE_FLAG | pagesz_flags(pgsz);
> > 
> > It is incorrect to mix RTE_ and POSIX flags.
> > I suggest leaving "extmem" part Unix-only for now an not change it.
> 
> Thanks for the advice. I will exclude extmem part on Windows then. In this case, seems Patch 5/6 is unnecessary any more.
> 
> 
> > 
> 
> 
> > [...]
> > > @@ -3824,8 +3827,8 @@ main(int argc, char** argv)
> > >  	latencystats_enabled = 0;
> > >  #endif
> > >  
> > > -	/* on FreeBSD, mlockall() is disabled by default */
> > > -#ifdef RTE_EXEC_ENV_FREEBSD
> > > +	/* on FreeBSD and Window, mlockall() is disabled by default */
> > > +#if (defined(RTE_EXEC_ENV_FREEBSD) || defined (RTE_EXEC_ENV_WINDOWS))
> > >  	do_mlockall = 0;
> > >  #else
> > >  	do_mlockall = 1;
> > 
> > Redundant braces and a typo: "Window".
> 
> Will fix in V4.
> 
> > 
> > [...]
> > > @@ -3971,10 +3978,11 @@ main(int argc, char** argv)
> > >  			return 1;
> > >  	}
> > >  
> > > +#ifndef RTE_EXEC_ENV_WINDOWS
> > >  	ret = rte_eal_cleanup();
> > >  	if (ret != 0)
> > >  		rte_exit(EXIT_FAILURE,
> > >  			 "EAL cleanup failed: %s\n", strerror(-ret));
> > > -
> > > +#endif
> > 
> > You seem to have missed my comment on v2 that bypassing cleanup is no longer
> > required after b2f24588b5 ("mem: fix cleanup when multi-process is disabled").
> 
> Missed this one as well. Will remove the bypassing cleanup in V4. Thanks.
> 
> > 
> > > diff --git a/lib/librte_eal/windows/include/rte_os.h b/lib/librte_eal/windows/include/rte_os.h
> > > index 60a623d4c..095f5f13b 100644
> > > --- a/lib/librte_eal/windows/include/rte_os.h
> > > +++ b/lib/librte_eal/windows/include/rte_os.h
> > > @@ -24,6 +24,14 @@ extern "C" {
> > >  #define PATH_MAX _MAX_PATH
> > >  #endif
> > >  
> > > +#define strcasecmp _stricmp
> > > +#define open _open
> > > +#define read _read
> > > +
> > > +#ifndef S_ISREG
> > > +#define S_ISREG(mode)  (((mode)&S_IFMT) == S_IFREG)
> > > +#endif
> > > +
> > >  #ifndef sleep
> > >  #define sleep(x) Sleep(1000 * (x))
> > >  #endif
> > 
> > This duplicates patch 3/6, doesn't it?
> 
> Yeah, seems messed this up at spliting the patch. Will fix in V4.