On 03/29 11:20:37, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> Couple of high level comments:
>
> 1) Too many #ifdefs
> * Code is hard to read and maintain when every 10th line is #ifdef or #else
> * Are all #ifdef combinations tested? Or meant to be supported?
> * It seems that some design trade-offs would be needed for better code
> maintainability
>
> 2) Too many arch dependent #ifdefs
> * ARM_ARCH ifdefs and assembly in main .c / .h files should be replaced
> with arch specific functions defined under platform/linux-generic/arch/xxx.
> That's the only directory which should contain inline assembly.
This is the current status quo and there is opportunity to do better.
The first angle is general organization and code duplication. This picture
may help:
~/api-next/platform/linux-generic/arch (api-next)$ tree
.
├── arm
│ ├── odp
│ │ └── api
│ │ └── cpu_arch.h
│ ├── odp_cpu_arch.c
│ └── odp_sysinfo_parse.c
├── default
│ ├── odp
│ │ └── api
│ │ └── cpu_arch.h
│ ├── odp_cpu_arch.c
│ └── odp_sysinfo_parse.c
├── mips64
│ ├── odp
│ │ └── api
│ │ └── cpu_arch.h
│ ├── odp_cpu_arch.c
│ └── odp_sysinfo_parse.c
├── powerpc
│ ├── odp
│ │ └── api
│ │ └── cpu_arch.h
│ ├── odp_cpu_arch.c
│ └── odp_sysinfo_parse.c
└── x86
├── odp
│ └── api
│ └── cpu_arch.h
├── odp_cpu_arch.c
└── odp_sysinfo_parse.c
There is not enough actual code to warrant the current situation,
and I prefer to see a single file containing code for multiple
architectures, e.g.:
cpu.h
static inline void odp_cpu_pause(void)
{
#if defined(__ARCH_ARM)
asm volatile("isb" ::: "memory");
#elif defined(__mips__)
asm volatile("nop");
asm volatile("nop");
asm volatile("nop");
asm volatile("nop");
#elif defined(__x86_64__) || defined(__i386__)
asm volatile("pause");
#else
#error Please add support for your CPU in cpu.h
#endif
}
If architecture specific code must be split into architecture
specific files, an alternative is:
cpu.h
#if defined(__ARCH_ARM)
#include "cpu_arm.h"
#elif defined(__mips__)
#include "cpu_mips.h"
..
cpu_arm.h
static inline void odp_cpu_pause(void)
{
asm volatile("isb" ::: "memory");
}
..
Notice how this eliminates the dependency on Autotools to setup
proper include paths and to do architecture detection. The compiler's
system-specific macros are a superior way to conditionally compile
code.
Some of the code isn't even CPU or architecture specific and does not
even belong here, e.g. parsing sysfs.
The second angle is that not all inline assembly applies to all
architectures. So, the current way of doing things will continue to
add more duplication and technical debt. I don't think anyone would
feel good about seeing LL/SC related code in an x86 file even if
it's just a bunch of empty functions. We can do better. I am curious
to know your opinion here.
> 3) Keep and improve modularity
> * #ifdef ODP_SCHEDULE_SCALABLE should not show up common pktio/queue/etc
> files. Only in makefiles and interface selection.
Will see what can be done.
> * Main build time config options (config_internal.h) should be high level
> (== max number of XXX), not algorithmic details of one scheduler. Move those
> into local .c/.h files.
This is a pre-existing condition. There are some component-specific
build time config in this file (e.g. burst size, Christophe's work). It is also
easy to locate build time config because it's in one file. I don't know
what the long term approach should be, but I don't think this is a blocker.
> * Keep definitions local when used only by one file. Check common headers
> before adding new definitions into common headers. E.g. CHECK_IS_POWER2 is
> already defined in odp_align_internal.h.
Sure
> * Improve e.g. queue interface modularity first with current code base.
> After that's done, hook the new scheduler to the interface.
Will see what can be done, but may take a few iterations.
> 4) Split 4/4 into couple of patches
> * First prepare the current code base with couple of patches
> * Proper interfaces / modifications for modularity (see previous bullet)
> * This enables us to see and test how current code base is impacted
> * Also git history is easier to work with when single patch does not
> touch many files / features
> * Bring in the new scheduler code as the last patch of the series
Will see what can be done, but may take a few iterations.
>
>
> -Petri
>
>
