| Message ID | 1455620740-10110-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Can that patch be reviewed? It's simple ./configure option. Maxim. On 02/16/16 14:05, Maxim Uvarov wrote: > With more options to enable/disable features it's more easy to isolate > and debug problem if any. For now I have suspitious that in CI make check > runs in parallel with other make checks which share the same pool memory. > I.e. one process can corrupt memory for other process. Moving IPC to option > it will be easy to debug that. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > configure.ac | 3 ++- > platform/linux-generic/m4/configure.m4 | 1 + > platform/linux-generic/m4/odp_ipc.m4 | 29 +++++++++++++++++++++++++++++ > platform/linux-generic/odp_pool.c | 4 ++++ > platform/linux-generic/pktio/io_ops.c | 2 ++ > platform/linux-generic/test/Makefile.am | 7 +++++-- > 6 files changed, 43 insertions(+), 3 deletions(-) > create mode 100644 platform/linux-generic/m4/odp_ipc.m4 > > diff --git a/configure.ac b/configure.ac > index 257f8c3..f7225cb 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -85,6 +85,7 @@ AC_SUBST([platform_with_platform_test], ["platform/${with_platform}/test"]) > # Prepare default values for platform specific optional features > ########################################################################## > netmap_support=no > +pktio_ipc_support=no > > ########################################################################## > # Run platform specific checks and settings > @@ -102,7 +103,7 @@ fi > ########################################################################## > AM_CONDITIONAL([netmap_support], [test x$netmap_support = xyes ]) > AM_CONDITIONAL([HAVE_PCAP], [test $have_pcap = yes]) > - > +AM_CONDITIONAL([pktio_ipc_support], [test x$pktio_ipc_support = yes]) > > AC_ARG_WITH([sdk-install-path], > AC_HELP_STRING([--with-sdk-install-path=DIR path to external libs and headers], > diff --git a/platform/linux-generic/m4/configure.m4 b/platform/linux-generic/m4/configure.m4 > index 2ac4799..b8c1c52 100644 > --- a/platform/linux-generic/m4/configure.m4 > +++ b/platform/linux-generic/m4/configure.m4 > @@ -21,6 +21,7 @@ m4_include([platform/linux-generic/m4/odp_openssl.m4]) > m4_include([platform/linux-generic/m4/odp_netmap.m4]) > m4_include([platform/linux-generic/m4/odp_dpdk.m4]) > m4_include([platform/linux-generic/m4/odp_pcap.m4]) > +m4_include([platform/linux-generic/m4/odp_ipc.m4]) > > AC_CONFIG_FILES([platform/linux-generic/Makefile > platform/linux-generic/test/Makefile > diff --git a/platform/linux-generic/m4/odp_ipc.m4 b/platform/linux-generic/m4/odp_ipc.m4 > new file mode 100644 > index 0000000..29506a0 > --- /dev/null > +++ b/platform/linux-generic/m4/odp_ipc.m4 > @@ -0,0 +1,29 @@ > +########################################################################## > +# Enable IPC pktio support > +########################################################################## > +AC_ARG_ENABLE([pktio_ipc_support], > + [ --enable-pktio_ipc-support include ipc IO support], > + [if test x$enableval = xyes; then > + pktio_ipc_support=yes > + fi]) > + > +########################################################################## > +# Save and set temporary compilation flags > +########################################################################## > +OLD_CPPFLAGS=$CPPFLAGS > +CPPFLAGS="$AM_CPPFLAGS $CPPFLAGS" > + > +########################################################################## > +# Check for DPDK availability > +########################################################################## > +if test x$pktio_ipc_support = xyes > +then > + ODP_CFLAGS="$ODP_CFLAGS -DODP_PKTIO_IPC" > +else > + pktio_ipc_support=no > +fi > + > +########################################################################## > +# Restore old saved variables > +########################################################################## > +CPPFLAGS=$OLD_CPPFLAGS > diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c > index 78ccc0f..9eaa0c8 100644 > --- a/platform/linux-generic/odp_pool.c > +++ b/platform/linux-generic/odp_pool.c > @@ -421,7 +421,11 @@ odp_pool_t _pool_create(const char *name, > odp_pool_t odp_pool_create(const char *name, > odp_pool_param_t *params) > { > +#ifdef ODP_PKTIO_IPC > return _pool_create(name, params, ODP_SHM_PROC); > +#else > + return _pool_create(name, params, 0); > +#endif > } > > odp_pool_t odp_pool_lookup(const char *name) > diff --git a/platform/linux-generic/pktio/io_ops.c b/platform/linux-generic/pktio/io_ops.c > index 15aa160..5824785 100644 > --- a/platform/linux-generic/pktio/io_ops.c > +++ b/platform/linux-generic/pktio/io_ops.c > @@ -21,7 +21,9 @@ const pktio_if_ops_t * const pktio_if_ops[] = { > #ifdef HAVE_PCAP > &pcap_pktio_ops, > #endif > +#ifdef ODP_PKTIO_IPC > &ipc_pktio_ops, > +#endif > &tap_pktio_ops, > &sock_mmap_pktio_ops, > &sock_mmsg_pktio_ops, > diff --git a/platform/linux-generic/test/Makefile.am b/platform/linux-generic/test/Makefile.am > index 1011437..09ace38 100644 > --- a/platform/linux-generic/test/Makefile.am > +++ b/platform/linux-generic/test/Makefile.am > @@ -1,12 +1,11 @@ > include $(top_srcdir)/test/Makefile.inc > TESTS_ENVIRONMENT += TEST_DIR=${top_builddir}/test/validation > > -ODP_MODULES = pktio pktio_ipc ring > +ODP_MODULES = pktio ring > > if test_vald > TESTS = pktio/pktio_run \ > pktio/pktio_run_tap \ > - pktio_ipc/pktio_ipc_run \ > ring/ringtest$(EXEEXT) \ > ${top_builddir}/test/validation/atomic/atomic_main$(EXEEXT) \ > ${top_builddir}/test/validation/barrier/barrier_main$(EXEEXT) \ > @@ -38,6 +37,10 @@ SUBDIRS = $(ODP_MODULES) > if HAVE_PCAP > TESTS += pktio/pktio_run_pcap > endif > +if pktio_ipc_support > +TESTS += pktio_ipc/pktio_ipc_run > +SUBDIRS += pktio_ipc > +endif > endif > > dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
On 02/17/16 09:59, Elo, Matias (Nokia - FI/Espoo) wrote: > Comments below. > > -Matias > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT Maxim >> Uvarov >> Sent: Tuesday, February 16, 2016 1:06 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [API-NEXT PATCH] linux-generic: pktio: add option to enable >> and disable ipc pktio >> >> With more options to enable/disable features it's more easy to isolate >> and debug problem if any. For now I have suspitious that in CI make check >> runs in parallel with other make checks which share the same pool memory. >> I.e. one process can corrupt memory for other process. Moving IPC to option >> it will be easy to debug that. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> configure.ac | 3 ++- >> platform/linux-generic/m4/configure.m4 | 1 + >> platform/linux-generic/m4/odp_ipc.m4 | 29 >> +++++++++++++++++++++++++++++ >> platform/linux-generic/odp_pool.c | 4 ++++ >> platform/linux-generic/pktio/io_ops.c | 2 ++ >> platform/linux-generic/test/Makefile.am | 7 +++++-- >> 6 files changed, 43 insertions(+), 3 deletions(-) >> create mode 100644 platform/linux-generic/m4/odp_ipc.m4 >> >> diff --git a/configure.ac b/configure.ac >> index 257f8c3..f7225cb 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -85,6 +85,7 @@ AC_SUBST([platform_with_platform_test], >> ["platform/${with_platform}/test"]) >> # Prepare default values for platform specific optional features >> >> ############################################################## >> ############ >> netmap_support=no >> +pktio_ipc_support=no >> > We should probably choose a common naming scheme for these variables. > 'pktio_<type>_support' sounds OK to me. yes, I also would like to use something common. >> ############################################################## >> ############ >> # Run platform specific checks and settings >> @@ -102,7 +103,7 @@ fi >> >> ############################################################## >> ############ >> AM_CONDITIONAL([netmap_support], [test x$netmap_support = xyes ]) >> AM_CONDITIONAL([HAVE_PCAP], [test $have_pcap = yes]) >> - >> +AM_CONDITIONAL([pktio_ipc_support], [test x$pktio_ipc_support = yes]) > Same thing as above. AM_CONDITIONAL naming convention seems to be all caps. I'm > currently working on a similar patch for DPDK, so I'd like to use the same naming scheme. > >> AC_ARG_WITH([sdk-install-path], >> AC_HELP_STRING([--with-sdk-install-path=DIR path to external libs and >> headers], >> diff --git a/platform/linux-generic/m4/configure.m4 b/platform/linux- >> generic/m4/configure.m4 >> index 2ac4799..b8c1c52 100644 >> --- a/platform/linux-generic/m4/configure.m4 >> +++ b/platform/linux-generic/m4/configure.m4 >> @@ -21,6 +21,7 @@ m4_include([platform/linux-generic/m4/odp_openssl.m4]) >> m4_include([platform/linux-generic/m4/odp_netmap.m4]) >> m4_include([platform/linux-generic/m4/odp_dpdk.m4]) >> m4_include([platform/linux-generic/m4/odp_pcap.m4]) >> +m4_include([platform/linux-generic/m4/odp_ipc.m4]) >> >> AC_CONFIG_FILES([platform/linux-generic/Makefile >> platform/linux-generic/test/Makefile >> diff --git a/platform/linux-generic/m4/odp_ipc.m4 b/platform/linux- >> generic/m4/odp_ipc.m4 >> new file mode 100644 >> index 0000000..29506a0 >> --- /dev/null >> +++ b/platform/linux-generic/m4/odp_ipc.m4 >> @@ -0,0 +1,29 @@ >> +############################################################# >> ############# >> +# Enable IPC pktio support >> +############################################################# >> ############# >> +AC_ARG_ENABLE([pktio_ipc_support], >> + [ --enable-pktio_ipc-support include ipc IO support], >> + [if test x$enableval = xyes; then >> + pktio_ipc_support=yes >> + fi]) >> + >> +############################################################# >> ############# >> +# Save and set temporary compilation flags >> +############################################################# >> ############# >> +OLD_CPPFLAGS=$CPPFLAGS >> +CPPFLAGS="$AM_CPPFLAGS $CPPFLAGS" >> + >> +############################################################# >> ############# >> +# Check for DPDK availability >> +############################################################# > Wrong header. Thanks, will send in v3. Maxim. >> ############# >> +if test x$pktio_ipc_support = xyes >> +then >> + ODP_CFLAGS="$ODP_CFLAGS -DODP_PKTIO_IPC" >> +else >> + pktio_ipc_support=no >> +fi >> + >> +############################################################# >> ############# >> +# Restore old saved variables >> +############################################################# >> ############# >> +CPPFLAGS=$OLD_CPPFLAGS >> diff --git a/platform/linux-generic/odp_pool.c b/platform/linux- >> generic/odp_pool.c >> index 78ccc0f..9eaa0c8 100644 >> --- a/platform/linux-generic/odp_pool.c >> +++ b/platform/linux-generic/odp_pool.c >> @@ -421,7 +421,11 @@ odp_pool_t _pool_create(const char *name, >> odp_pool_t odp_pool_create(const char *name, >> odp_pool_param_t *params) >> { >> +#ifdef ODP_PKTIO_IPC >> return _pool_create(name, params, ODP_SHM_PROC); >> +#else >> + return _pool_create(name, params, 0); >> +#endif >> } >> >> odp_pool_t odp_pool_lookup(const char *name) >> diff --git a/platform/linux-generic/pktio/io_ops.c b/platform/linux- >> generic/pktio/io_ops.c >> index 15aa160..5824785 100644 >> --- a/platform/linux-generic/pktio/io_ops.c >> +++ b/platform/linux-generic/pktio/io_ops.c >> @@ -21,7 +21,9 @@ const pktio_if_ops_t * const pktio_if_ops[] = { >> #ifdef HAVE_PCAP >> &pcap_pktio_ops, >> #endif >> +#ifdef ODP_PKTIO_IPC >> &ipc_pktio_ops, >> +#endif >> &tap_pktio_ops, >> &sock_mmap_pktio_ops, >> &sock_mmsg_pktio_ops, >> diff --git a/platform/linux-generic/test/Makefile.am b/platform/linux- >> generic/test/Makefile.am >> index 1011437..09ace38 100644 >> --- a/platform/linux-generic/test/Makefile.am >> +++ b/platform/linux-generic/test/Makefile.am >> @@ -1,12 +1,11 @@ >> include $(top_srcdir)/test/Makefile.inc >> TESTS_ENVIRONMENT += TEST_DIR=${top_builddir}/test/validation >> >> -ODP_MODULES = pktio pktio_ipc ring >> +ODP_MODULES = pktio ring >> >> if test_vald >> TESTS = pktio/pktio_run \ >> pktio/pktio_run_tap \ >> - pktio_ipc/pktio_ipc_run \ >> ring/ringtest$(EXEEXT) \ >> ${top_builddir}/test/validation/atomic/atomic_main$(EXEEXT) \ >> ${top_builddir}/test/validation/barrier/barrier_main$(EXEEXT) \ >> @@ -38,6 +37,10 @@ SUBDIRS = $(ODP_MODULES) >> if HAVE_PCAP >> TESTS += pktio/pktio_run_pcap >> endif >> +if pktio_ipc_support >> +TESTS += pktio_ipc/pktio_ipc_run >> +SUBDIRS += pktio_ipc >> +endif >> endif >> >> dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER) >> -- >> 2.7.1.250.gff4ea60 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
On 02/17/16 10:43, Savolainen, Petri (Nokia - FI/Espoo) wrote: >>> ############################################################## >>> ############ >>> netmap_support=no >>> +pktio_ipc_support=no >>> >> We should probably choose a common naming scheme for these variables. >> 'pktio_<type>_support' sounds OK to me. >> >>> ############################################################## >>> ############ >>> # Run platform specific checks and settings >>> @@ -102,7 +103,7 @@ fi >>> >>> ############################################################## >>> ############ >>> AM_CONDITIONAL([netmap_support], [test x$netmap_support = xyes ]) >>> AM_CONDITIONAL([HAVE_PCAP], [test $have_pcap = yes]) > > The variable naming convention should be harmonized. E.g. above we have "have_pcap" defined in both lower and upper case. Also "xnetmap_support" is set to "xyes" ?? Are all these variables (with and without x) needed ? > > I'd suggest to define makefile variables in upper case and use PKTIO_ prefix for all these variables (e.g. PKTIO_NETMAP, PKTIO_NETMAP_XYZ, ...) which select various pktio options. > > -Petri > Let's do that harmonization in separate patch. There are bunch of other places and it will be more easy to review all renaming in one patch. Maxim. > >>> - >>> +AM_CONDITIONAL([pktio_ipc_support], [test x$pktio_ipc_support = yes]) >> Same thing as above. AM_CONDITIONAL naming convention seems to be all >> caps. I'm >> currently working on a similar patch for DPDK, so I'd like to use the same >> naming scheme. >>
diff --git a/configure.ac b/configure.ac index 257f8c3..f7225cb 100644 --- a/configure.ac +++ b/configure.ac @@ -85,6 +85,7 @@ AC_SUBST([platform_with_platform_test], ["platform/${with_platform}/test"]) # Prepare default values for platform specific optional features ########################################################################## netmap_support=no +pktio_ipc_support=no ########################################################################## # Run platform specific checks and settings @@ -102,7 +103,7 @@ fi ########################################################################## AM_CONDITIONAL([netmap_support], [test x$netmap_support = xyes ]) AM_CONDITIONAL([HAVE_PCAP], [test $have_pcap = yes]) - +AM_CONDITIONAL([pktio_ipc_support], [test x$pktio_ipc_support = yes]) AC_ARG_WITH([sdk-install-path], AC_HELP_STRING([--with-sdk-install-path=DIR path to external libs and headers], diff --git a/platform/linux-generic/m4/configure.m4 b/platform/linux-generic/m4/configure.m4 index 2ac4799..b8c1c52 100644 --- a/platform/linux-generic/m4/configure.m4 +++ b/platform/linux-generic/m4/configure.m4 @@ -21,6 +21,7 @@ m4_include([platform/linux-generic/m4/odp_openssl.m4]) m4_include([platform/linux-generic/m4/odp_netmap.m4]) m4_include([platform/linux-generic/m4/odp_dpdk.m4]) m4_include([platform/linux-generic/m4/odp_pcap.m4]) +m4_include([platform/linux-generic/m4/odp_ipc.m4]) AC_CONFIG_FILES([platform/linux-generic/Makefile platform/linux-generic/test/Makefile diff --git a/platform/linux-generic/m4/odp_ipc.m4 b/platform/linux-generic/m4/odp_ipc.m4 new file mode 100644 index 0000000..29506a0 --- /dev/null +++ b/platform/linux-generic/m4/odp_ipc.m4 @@ -0,0 +1,29 @@ +########################################################################## +# Enable IPC pktio support +########################################################################## +AC_ARG_ENABLE([pktio_ipc_support], + [ --enable-pktio_ipc-support include ipc IO support], + [if test x$enableval = xyes; then + pktio_ipc_support=yes + fi]) + +########################################################################## +# Save and set temporary compilation flags +########################################################################## +OLD_CPPFLAGS=$CPPFLAGS +CPPFLAGS="$AM_CPPFLAGS $CPPFLAGS" + +########################################################################## +# Check for DPDK availability +########################################################################## +if test x$pktio_ipc_support = xyes +then + ODP_CFLAGS="$ODP_CFLAGS -DODP_PKTIO_IPC" +else + pktio_ipc_support=no +fi + +########################################################################## +# Restore old saved variables +########################################################################## +CPPFLAGS=$OLD_CPPFLAGS diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c index 78ccc0f..9eaa0c8 100644 --- a/platform/linux-generic/odp_pool.c +++ b/platform/linux-generic/odp_pool.c @@ -421,7 +421,11 @@ odp_pool_t _pool_create(const char *name, odp_pool_t odp_pool_create(const char *name, odp_pool_param_t *params) { +#ifdef ODP_PKTIO_IPC return _pool_create(name, params, ODP_SHM_PROC); +#else + return _pool_create(name, params, 0); +#endif } odp_pool_t odp_pool_lookup(const char *name) diff --git a/platform/linux-generic/pktio/io_ops.c b/platform/linux-generic/pktio/io_ops.c index 15aa160..5824785 100644 --- a/platform/linux-generic/pktio/io_ops.c +++ b/platform/linux-generic/pktio/io_ops.c @@ -21,7 +21,9 @@ const pktio_if_ops_t * const pktio_if_ops[] = { #ifdef HAVE_PCAP &pcap_pktio_ops, #endif +#ifdef ODP_PKTIO_IPC &ipc_pktio_ops, +#endif &tap_pktio_ops, &sock_mmap_pktio_ops, &sock_mmsg_pktio_ops, diff --git a/platform/linux-generic/test/Makefile.am b/platform/linux-generic/test/Makefile.am index 1011437..09ace38 100644 --- a/platform/linux-generic/test/Makefile.am +++ b/platform/linux-generic/test/Makefile.am @@ -1,12 +1,11 @@ include $(top_srcdir)/test/Makefile.inc TESTS_ENVIRONMENT += TEST_DIR=${top_builddir}/test/validation -ODP_MODULES = pktio pktio_ipc ring +ODP_MODULES = pktio ring if test_vald TESTS = pktio/pktio_run \ pktio/pktio_run_tap \ - pktio_ipc/pktio_ipc_run \ ring/ringtest$(EXEEXT) \ ${top_builddir}/test/validation/atomic/atomic_main$(EXEEXT) \ ${top_builddir}/test/validation/barrier/barrier_main$(EXEEXT) \ @@ -38,6 +37,10 @@ SUBDIRS = $(ODP_MODULES) if HAVE_PCAP TESTS += pktio/pktio_run_pcap endif +if pktio_ipc_support +TESTS += pktio_ipc/pktio_ipc_run +SUBDIRS += pktio_ipc +endif endif dist_check_SCRIPTS = run-test tests-validation.env $(LOG_COMPILER)
With more options to enable/disable features it's more easy to isolate and debug problem if any. For now I have suspitious that in CI make check runs in parallel with other make checks which share the same pool memory. I.e. one process can corrupt memory for other process. Moving IPC to option it will be easy to debug that. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- configure.ac | 3 ++- platform/linux-generic/m4/configure.m4 | 1 + platform/linux-generic/m4/odp_ipc.m4 | 29 +++++++++++++++++++++++++++++ platform/linux-generic/odp_pool.c | 4 ++++ platform/linux-generic/pktio/io_ops.c | 2 ++ platform/linux-generic/test/Makefile.am | 7 +++++-- 6 files changed, 43 insertions(+), 3 deletions(-) create mode 100644 platform/linux-generic/m4/odp_ipc.m4