Hamish Guthrie hamish@prodigi.ch writes:
Eric,
I totally agree that constructive criticism is in order. Please look at the original version of the code I submitted a few months ago, shortly after the cpu_relax issue first came into being:
Thank you.
I do not want to harp on ad infinitum on this, but, there are a number of people in the embedded space using LinuxBIOS, and generally there are very few SMP devices in the embedded space. I would also just like to ask how many devices are out there using LinuxBIOS with SMP?
With dual cores becoming the path to performance you can't make a new system that isn't SMP. From the cluster side most interesting commodity are dual processor SMP.
I am sure that I could multiply that number by 10 000 to get to the number of embedded uniprocessor devices using LinuxBIOS. Right now, I am in the process of rolling out some replacement firmware for some 120 000 devices previously running WinCE, which are now running Linux with LinuxBIOS as the loader.
Cool. I like the thought of 100 million devices running LinuxBIOS :)
Do we need to get to the worst-case scenario and fork LinuxBIOS for uniprocessor/SMP> - I would like to think not.
Agreed.
My impression is usually the SMP/Desktop work uses the same hardware as the embedded systems but a couple of years sooner. There is also the challenge that a lot of embedded systems are single shot deals many times embedded developers don't have a long term interest in the free software project they work with. So my impression is that in a 5 years or so we will start seeing multi core embedded systems.
The most maintainable fix for something like cpu_relax() is to have one definition that compiles SMP and another definition that works non-smp.
#ifdef CONFIG_SMP static inline void cpu_relax(void) { __asm__ __volatile__ ("rep ; nop"); } #else static inline void cpu_relax(void) { /* do nothing */ } #endif
Although in this particular case I don't know it even makes sense to bracket cpu_relax();
My point being that putting the #ifdef code in header files or in the definition of the function being called it is frequently more maintainable, than having it in the inline body of the function. Then so long as calling cpu_relax() is a reasonable thing to do you don't have to worry about it in that code.
For this particular piece of code cpu_relax() largely overkill because we don't do much on secondary processors.
With the growing developer base the conflicts between different developers have become a problem, this is not isolated to embedded work/SMP work conflicting. Getting a functioning code review and release process in place is one of the things we need to talk about at the LinuxBIOS summit.
I know Jason tested the build on a least one uniprocessor system but that may not have tested this code path.
Jason before committing will you please double check that cpu_relax() has a definition of CONFIG_SMP is 0 or not set?
Eric