[LinuxBIOS] LNXI Merge: lnxi-patch-06/16

[Hamish Guthrie]

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 at 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
> 
>