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

[jason schildt]

DESCRIPTION:
----------------------------------------------

## lnxi-patch-6 ##
src/cpu/x86/tsc/delay_tsc.c
    cpu_relax() gets called unconditionally.

DIFFSTAT:
----------------------------------------------
  delay_tsc.c |    4 ----
  1 files changed, 4 deletions(-)

PATCH:
----------------------------------------------

Index: delay_tsc.c
===================================================================
--- delay_tsc.c (revision 1105)
+++ delay_tsc.c (working copy)
@@ -159,11 +159,7 @@
          count = rdtscll();
          stop = clocks + count;
          while(stop > count) {
-#ifdef CONFIG_SMP
-#if CONFIG_SMP == 1
                 cpu_relax();
-#endif
-#endif
                 count = rdtscll();
          }
  }

Jason W. Schildt
LinuxBIOS Software Engineer
Linux Networx

[Hamish Guthrie]

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

In any event, the patch is crap!

Hamish

jason schildt wrote:
> DESCRIPTION:
>
> ## lnxi-patch-6 ##
> src/cpu/x86/tsc/delay_tsc.c
>     cpu_relax() gets called unconditionally.
> DIFFSTAT:
>
> delay_tsc.c |    4 ----
>  1 files changed, 4 deletions(-)
> PATCH:
> Index: delay_tsc.c
>
> --- delay_tsc.c (revision 1105)
> +++ delay_tsc.c (working copy)
> @@ -159,11 +159,7 @@
>          count = rdtscll();
>          stop = clocks + count;
>          while(stop > count) {
> -#ifdef CONFIG_SMP
> -#if CONFIG_SMP == 1
>                 cpu_relax();
> -#endif
> -#endif
>                 count = rdtscll();
>          }
>  }
>
> Jason W. Schildt
> LinuxBIOS Software Engineer
> Linux Networx

Hamish Guthrie  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

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

Hamish Guthrie  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

[Peter Karlsson]

On Tue, 6 Sep 2005, Eric W. Biederman wrote:
> 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.

http://www.embedded.com/showArticle.jhtml?articleID=169600382

Best regards

Peter K

[Hamish Guthrie]

This patch is CRAP!!!!

This unconditionally breaks tsc usage for single-processor systems.

PLEASE leave the original code as it was - it is generic code for both 
SMP and uni-processor environments. The code as it is does not break 
your SMP code, so why break our uni-processor code?????

Thanks

Hamish

jason schildt wrote:
> DESCRIPTION:
>
> ## lnxi-patch-6 ##
> src/cpu/x86/tsc/delay_tsc.c
>     cpu_relax() gets called unconditionally.
> DIFFSTAT:
>
> delay_tsc.c |    4 ----
>  1 files changed, 4 deletions(-)
> PATCH:
> Index: delay_tsc.c
>
> --- delay_tsc.c (revision 1105)
> +++ delay_tsc.c (working copy)
> @@ -159,11 +159,7 @@
>          count = rdtscll();
>          stop = clocks + count;
>          while(stop > count) {
> -#ifdef CONFIG_SMP
> -#if CONFIG_SMP == 1
>                 cpu_relax();
> -#endif
> -#endif
>                 count = rdtscll();
>          }
>  }
>
> Jason W. Schildt
> LinuxBIOS Software Engineer
> Linux Networx