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:
void udelay(unsigned us) { unsigned long long count; unsigned long long stop; unsigned long long clocks;
init_timer(); clocks = us; clocks *= clocks_per_usec; count = rdtscll(); stop = clocks + count; while(stop > count) { #ifdef CONFIG_SMP #if CONFIG_SMP == 1 cpu_relax(); #endif #endif count = rdtscll(); } }
There was also a comment in the commit of that modification explaining exactly why it should be bracketed in the #ifdef CONFIG_SMP.
I would also guess that anyone reading that code should actually think that there may be a reason, however daft it may seem, for someone to go to the trouble of bracketing the cpu_relax statement in such a way.
Maybe I should add the following comment to the code, in order for it to be more explicit:
/* The cpu_relax function call is specific to the SMP environment, if * code is compiled without the CONFIG_SMP define, and the cpu_relax * call is encountered, the resulting code will break. Please, therefore * do not remove the enclosing defines because cpu_relax is an SMP * specific function. */
I will not, however add this to the code, I would expect the people who added the cpu_relax function to be considerate enough as to maybe bracket their function call in #ifdef CONFIG_SMP so as to not break the uniprocessor code.
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? 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.
Do we need to get to the worst-case scenario and fork LinuxBIOS for uniprocessor/SMP> - I would like to think not.
Hamish
Eric W. Biederman wrote:
Hamish Guthrie hamish@prodigi.ch writes:
I would like to apologise for my previous tirade, but I have fixed this problem a few times in the past and the SMP guys just ignore UNDERSTANDING THE CODE!!!
That is part of the point of the code review. So we can work out the issues.
In general it is much better to have something like cpu_relax conditionalized in the header than in the calling code. As it is the code is hard to read and it is not at all obvious why. As I recall cpu_relax simply expands to rep; nop which should be safe on any cpu.
In any event, the patch is crap!
How so?
The point is to communicate and if there is something non-obvious we need in a comment in the code so people do not forget what is going on.
Eric