> -----Original Message----- > From: Brian Brooks [mailto:[email protected]] > Sent: Wednesday, April 19, 2017 10:16 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> > Cc: [email protected] > Subject: Re: [lng-odp] [API-NEXT PATCH 2/8] linux-gen: cpu_flags: added > x86 cpu flag read functions > > On 04/24 07:21:09, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > > > > > -----Original Message----- > > > From: Brian Brooks [mailto:[email protected]] > > > Sent: Friday, April 21, 2017 7:51 PM > > > To: Petri Savolainen <[email protected]> > > > Cc: [email protected] > > > Subject: Re: [lng-odp] [API-NEXT PATCH 2/8] linux-gen: cpu_flags: > added > > > x86 cpu flag read functions > > > > > > On 04/21 16:11:28, Petri Savolainen wrote: > > > > When building on x86 CPU flags can be used to determine which > > > > CPU features are supported. CPU flag definitions and the code > > > > to read the flags is from DPDK. > > > > > > > > Signed-off-by: Petri Savolainen <[email protected]> > > > > --- > > > > configure.ac | 1 + > > > > platform/linux-generic/Makefile.am | 4 + > > > > platform/linux-generic/arch/x86/cpu_flags.c | 349 > > > ++++++++++++++++++++++++++++ > > > > platform/linux-generic/arch/x86/cpu_flags.h | 20 ++ > > > > 4 files changed, 374 insertions(+) > > > > create mode 100644 platform/linux-generic/arch/x86/cpu_flags.c > > > > create mode 100644 platform/linux-generic/arch/x86/cpu_flags.h > > > > > > > > diff --git a/configure.ac b/configure.ac > > > > index e86e2dca..38129030 100644 > > > > --- a/configure.ac > > > > +++ b/configure.ac > > > > @@ -224,6 +224,7 @@ AM_CONDITIONAL([HAVE_DOXYGEN], [test > "x${DOXYGEN}" = > > > "xdoxygen"]) > > > > AM_CONDITIONAL([user_guide], [test "x${user_guides}" = "xyes" ]) > > > > AM_CONDITIONAL([HAVE_MSCGEN], [test "x${MSCGEN}" = "xmscgen"]) > > > > AM_CONDITIONAL([helper_linux], [test x$helper_linux = xyes ]) > > > > +AM_CONDITIONAL([ARCH_IS_X86], [test "x${ARCH_DIR}" = "xx86"]) > > > > > > > > > > > > ########################################################################## > > > > # Setup doxygen documentation > > > > diff --git a/platform/linux-generic/Makefile.am b/platform/linux- > > > generic/Makefile.am > > > > index 81a19011..60b7f849 100644 > > > > --- a/platform/linux-generic/Makefile.am > > > > +++ b/platform/linux-generic/Makefile.am > > > > @@ -252,6 +252,10 @@ __LIB__libodp_linux_la_SOURCES = \ > > > > arch/@ARCH_DIR@/odp_cpu_arch.c \ > > > > arch/@ARCH_DIR@/odp_sysinfo_parse.c > > > > > > > > +if ARCH_IS_X86 > > > > +__LIB__libodp_linux_la_SOURCES += arch/@ARCH_DIR@/cpu_flags.c > > > > +endif > > > > > > The pre-existing condition is that all source files are compiled and > > > this method of conditional compilation is rejected, as is the case > > > for schedulers. How do we want to proceed? > > > > This file (arch/x86/cpu_flags.c) contains x86 assembly and is built only > for x86. As you can see from the lines above this line, arch/@ARCH_DIR@/ > is used to select the arch files. > > > > This is how I have instructed you to split out ARM assembly from your > scheduler code. Place ASM code into a new file (e.g. arch/arm/ll_sc.c or > whatever) and select that file when building for ARM. No ifdefs in C > files. > > > > Current makefile ... > > > > arch/@ARCH_DIR@/odp_cpu_arch.c \ > > arch/@ARCH_DIR@/odp_sysinfo_parse.c > > > > ... expects that all archs have those files implemented. That part > should be actually changed to use arch specific files only for those arch > that do not use the default implementation (x86/mips today). But that's > not a job of this patch - it just adds cpu_flags for x86. > > This same argument can be applied to the scheduler i.e. it conditionally > compiles code as needed, and anything else is not a job of that patch. > > Here, you would need to actually add those files for other archs and find > suitible arch-agnostic names for identifiers. > > Which way do you want to go?
You didn't get my point. This patch does not clean out arch/@ARCH_DIR@/odp_cpu_arch.c part of the Makefile, which could be improved to do the same thing (as what I'm adding here). It's a job for another patch set to improve current build rules of those non-related files and remove unnecessary code duplication. Also arch specific code and scheduler code are not solving the same problem. By definition, we cannot not compile arch specific code (e.g. x86 assembly) for other archs (e.g. to ARM). But scheduler code is architecture agnostic, and thus should be always compiled even when not used (because it can be compiled). Your scheduler code should take example from this patch set. Time.c is always compiled. It uses arch specific time counter read function, which is implemented/compiled per arch. If such HW feature is not supported, a generic implementation (clock_gettime based) is used instead, which is also compiled always. -Petri
