[PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Rakib Mullick]

  Impact: Reduce text space through efficient coding

 Determine apic is integrated or not - by calling
lapic_is_integrated() once from APIC_init_uniprocessor() and keep it
in a variable integrated_lapic. Thus we can determine apic is
integrated or not, by checking the variable instead of calling
lapic_is_integrated() on and on. Marking lapic_is_integrated() as
__init, which reduces some text space. Also allows us to get away from
messy #ifdef and inline.

---
Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>

--- linus/arch/x86/kernel/apic/apic.c	2009-05-21 22:22:15.000000000 +0600
+++ rakib/arch/x86/kernel/apic/apic.c	2009-05-21 23:27:26.000000000 +0600
@@ -127,6 +127,9 @@ early_param("nox2apic", setup_nox2apic);

 unsigned long mp_lapic_addr;
 int disable_apic;
+
+static int integrated_lapic;
+
 /* Disable local APIC timer from the kernel commandline or via dmi quirk */
 static int disable_apic_timer __cpuinitdata;
 /* Local APIC timer works in C2 */
@@ -188,13 +191,12 @@ static inline int lapic_get_version(void
 /*
  * Check, if the APIC is integrated or a separate chip
  */
-static inline int lapic_is_integrated(void)
+void __init lapic_is_integrated(void)
 {
-#ifdef CONFIG_X86_64
-	return 1;
-#else
-	return APIC_INTEGRATED(lapic_get_version());
-#endif
+	if (APIC_INTEGRATED(lapic_get_version()))
+		integrated_lapic = 1;
+	else
+		integrated_lapic = 0;
 }

 /*
@@ -258,7 +260,7 @@ void __cpuinit enable_NMI_through_LVT0(v
 	v = APIC_DM_NMI;

 	/* Level triggered for 82489DX (32bit mode) */
-	if (!lapic_is_integrated())
+	if (!integrated_lapic)
 		v |= APIC_LVT_LEVEL_TRIGGER;

 	apic_write(APIC_LVT0, v);
@@ -313,7 +315,7 @@ static void __setup_APIC_LVTT(unsigned i
 	lvtt_value = LOCAL_TIMER_VECTOR;
 	if (!oneshot)
 		lvtt_value |= APIC_LVT_TIMER_PERIODIC;
-	if (!lapic_is_integrated())
+	if (!integrated_lapic)
 		lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);

 	if (!irqen)
@@ -869,7 +871,7 @@ void clear_local_APIC(void)
 		apic_write(APIC_LVTPC, APIC_LVT_MASKED);

 	/* Integrated APIC (!82489DX) ? */
-	if (lapic_is_integrated()) {
+	if (integrated_lapic) {
 		if (maxlvt > 3)
 			/* Clear ESR due to Pentium errata 3AP and 11AP */
 			apic_write(APIC_ESR, 0);
@@ -1064,7 +1066,7 @@ void __init init_bsp_APIC(void)
 	 */
 	apic_write(APIC_LVT0, APIC_DM_EXTINT);
 	value = APIC_DM_NMI;
-	if (!lapic_is_integrated())		/* 82489DX */
+	if (!integrated_lapic)		/* 82489DX */
 		value |= APIC_LVT_LEVEL_TRIGGER;
 	apic_write(APIC_LVT1, value);
 }
@@ -1073,7 +1075,7 @@ static void __cpuinit lapic_setup_esr(vo
 {
 	unsigned int oldvalue, value, maxlvt;

-	if (!lapic_is_integrated()) {
+	if (!integrated_lapic) {
 		pr_info("No ESR for 82489DX.\n");
 		return;
 	}
@@ -1126,7 +1128,7 @@ void __cpuinit setup_local_APIC(void)

 #ifdef CONFIG_X86_32
 	/* Pound the ESR really hard over the head with a big hammer - mbligh */
-	if (lapic_is_integrated() && apic->disable_esr) {
+	if (integrated_lapic && apic->disable_esr) {
 		apic_write(APIC_ESR, 0);
 		apic_write(APIC_ESR, 0);
 		apic_write(APIC_ESR, 0);
@@ -1251,7 +1253,7 @@ void __cpuinit setup_local_APIC(void)
 		value = APIC_DM_NMI;
 	else
 		value = APIC_DM_NMI | APIC_LVT_MASKED;
-	if (!lapic_is_integrated())		/* 82489DX */
+	if (!integrated_lapic)		/* 82489DX */
 		value |= APIC_LVT_LEVEL_TRIGGER;
 	apic_write(APIC_LVT1, value);

@@ -1585,6 +1587,8 @@ int __init APIC_init_uniprocessor(void)
 		pr_info("Apic disabled by BIOS\n");
 		return -1;
 	}
+	/* Since CONFIG_X86_64, so it's integrated.	*/
+	integrated_lapic = 1;
 #else
 	if (!smp_found_config && !cpu_has_apic)
 		return -1;
@@ -1599,6 +1603,8 @@ int __init APIC_init_uniprocessor(void)
 		clear_cpu_cap(&boot_cpu_data, X86_FEATURE_APIC);
 		return -1;
 	}
+	/* Determine APIC is integrated or not	*/
+	lapic_is_integrated();
 #endif

 	enable_IR_x2apic();

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Cyrill Gorcunov]

[Rakib Mullick - Fri, May 22, 2009 at 07:31:29PM +0600]
|   Impact: Reduce text space through efficient coding
| 
|  Determine apic is integrated or not - by calling
| lapic_is_integrated() once from APIC_init_uniprocessor() and keep it
| in a variable integrated_lapic. Thus we can determine apic is
| integrated or not, by checking the variable instead of calling
| lapic_is_integrated() on and on. Marking lapic_is_integrated() as
| __init, which reduces some text space. Also allows us to get away from
| messy #ifdef and inline.
| 
| ---
| Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
| 
| --- linus/arch/x86/kernel/apic/apic.c	2009-05-21 22:22:15.000000000 +0600
| +++ rakib/arch/x86/kernel/apic/apic.c	2009-05-21 23:27:26.000000000 +0600
| @@ -127,6 +127,9 @@ early_param("nox2apic", setup_nox2apic);
| 
|  unsigned long mp_lapic_addr;
|  int disable_apic;
| +
| +static int integrated_lapic;
| +
|  /* Disable local APIC timer from the kernel commandline or via dmi quirk */
|  static int disable_apic_timer __cpuinitdata;
|  /* Local APIC timer works in C2 */
| @@ -188,13 +191,12 @@ static inline int lapic_get_version(void
|  /*
|   * Check, if the APIC is integrated or a separate chip
|   */
| -static inline int lapic_is_integrated(void)
| +void __init lapic_is_integrated(void)
|  {
| -#ifdef CONFIG_X86_64
| -	return 1;
| -#else
| -	return APIC_INTEGRATED(lapic_get_version());
| -#endif
| +	if (APIC_INTEGRATED(lapic_get_version()))
| +		integrated_lapic = 1;
| +	else
| +		integrated_lapic = 0;
|  }

Hi Rakib,

actually this change could be dangerous. I don't
remember if I saw mixed configuration at all but
I would not be that sure that we will never met it.

Peter? Ingo? Yinghai? Maciej?

| 
|  /*
| @@ -258,7 +260,7 @@ void __cpuinit enable_NMI_through_LVT0(v
|  	v = APIC_DM_NMI;
| 
|  	/* Level triggered for 82489DX (32bit mode) */
| -	if (!lapic_is_integrated())
| +	if (!integrated_lapic)
|  		v |= APIC_LVT_LEVEL_TRIGGER;
| 
|  	apic_write(APIC_LVT0, v);
| @@ -313,7 +315,7 @@ static void __setup_APIC_LVTT(unsigned i
|  	lvtt_value = LOCAL_TIMER_VECTOR;
|  	if (!oneshot)
|  		lvtt_value |= APIC_LVT_TIMER_PERIODIC;
| -	if (!lapic_is_integrated())
| +	if (!integrated_lapic)
|  		lvtt_value |= SET_APIC_TIMER_BASE(APIC_TIMER_BASE_DIV);
| 
|  	if (!irqen)
| @@ -869,7 +871,7 @@ void clear_local_APIC(void)
|  		apic_write(APIC_LVTPC, APIC_LVT_MASKED);
| 
|  	/* Integrated APIC (!82489DX) ? */
| -	if (lapic_is_integrated()) {
| +	if (integrated_lapic) {
|  		if (maxlvt > 3)
|  			/* Clear ESR due to Pentium errata 3AP and 11AP */
|  			apic_write(APIC_ESR, 0);
| @@ -1064,7 +1066,7 @@ void __init init_bsp_APIC(void)
|  	 */
|  	apic_write(APIC_LVT0, APIC_DM_EXTINT);
|  	value = APIC_DM_NMI;
| -	if (!lapic_is_integrated())		/* 82489DX */
| +	if (!integrated_lapic)		/* 82489DX */
|  		value |= APIC_LVT_LEVEL_TRIGGER;
|  	apic_write(APIC_LVT1, value);
|  }
| @@ -1073,7 +1075,7 @@ static void __cpuinit lapic_setup_esr(vo
|  {
|  	unsigned int oldvalue, value, maxlvt;
| 
| -	if (!lapic_is_integrated()) {
| +	if (!integrated_lapic) {
|  		pr_info("No ESR for 82489DX.\n");
|  		return;
|  	}
| @@ -1126,7 +1128,7 @@ void __cpuinit setup_local_APIC(void)
| 
|  #ifdef CONFIG_X86_32
|  	/* Pound the ESR really hard over the head with a big hammer - mbligh */
| -	if (lapic_is_integrated() && apic->disable_esr) {
| +	if (integrated_lapic && apic->disable_esr) {
|  		apic_write(APIC_ESR, 0);
|  		apic_write(APIC_ESR, 0);
|  		apic_write(APIC_ESR, 0);
| @@ -1251,7 +1253,7 @@ void __cpuinit setup_local_APIC(void)
|  		value = APIC_DM_NMI;
|  	else
|  		value = APIC_DM_NMI | APIC_LVT_MASKED;
| -	if (!lapic_is_integrated())		/* 82489DX */
| +	if (!integrated_lapic)		/* 82489DX */
|  		value |= APIC_LVT_LEVEL_TRIGGER;
|  	apic_write(APIC_LVT1, value);
| 
| @@ -1585,6 +1587,8 @@ int __init APIC_init_uniprocessor(void)
|  		pr_info("Apic disabled by BIOS\n");
|  		return -1;
|  	}
| +	/* Since CONFIG_X86_64, so it's integrated.	*/
| +	integrated_lapic = 1;
|  #else
|  	if (!smp_found_config && !cpu_has_apic)
|  		return -1;
| @@ -1599,6 +1603,8 @@ int __init APIC_init_uniprocessor(void)
|  		clear_cpu_cap(&boot_cpu_data, X86_FEATURE_APIC);
|  		return -1;
|  	}
| +	/* Determine APIC is integrated or not	*/
| +	lapic_is_integrated();
|  #endif
| 
|  	enable_IR_x2apic();
| 
	-- Cyrill

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> Hi Rakib,
> 
> actually this change could be dangerous. I don't
> remember if I saw mixed configuration at all but
> I would not be that sure that we will never met it.
> 
> Peter? Ingo? Yinghai? Maciej?
> 

That seems unlikely in the extreme.  To the best of my knowledge, only
486s ever used the external APICs.

	-hpa

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Cyrill Gorcunov]

[H. Peter Anvin - Fri, May 22, 2009 at 01:12:03PM -0700]
| Cyrill Gorcunov wrote:
| > 
| > Hi Rakib,
| > 
| > actually this change could be dangerous. I don't
| > remember if I saw mixed configuration at all but
| > I would not be that sure that we will never met it.
| > 
| > Peter? Ingo? Yinghai? Maciej?
| > 
| 
| That seems unlikely in the extreme.  To the best of my knowledge, only
| 486s ever used the external APICs.
| 
| 	-hpa
|

It was about a half year ago (need googling to be precise :)
but I was reported about Xeon processor which identify itself
as having "external" apic. So I hardly believe if real external
device with ICC bus were used.
 
	-- Cyrill

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> 
> It was about a half year ago (need googling to be precise :)
> but I was reported about Xeon processor which identify itself
> as having "external" apic. So I hardly believe if real external
> device with ICC bus were used.
>  

Details, please.  It might even be a bug in the test.

	-hpa

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Cyrill Gorcunov]

[H. Peter Anvin - Fri, May 22, 2009 at 01:23:53PM -0700]
| Cyrill Gorcunov wrote:
| > 
| > It was about a half year ago (need googling to be precise :)
| > but I was reported about Xeon processor which identify itself
| > as having "external" apic. So I hardly believe if real external
| > device with ICC bus were used.
| >  
| 
| Details, please.  It might even be a bug in the test.
| 
| 	-hpa
| 

yes, I'm trying to find it. wait a bit.

	-- Cyrill

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Cyrill Gorcunov]

[H. Peter Anvin - Fri, May 22, 2009 at 01:23:53PM -0700]
| Cyrill Gorcunov wrote:
| > 
| > It was about a half year ago (need googling to be precise :)
| > but I was reported about Xeon processor which identify itself
| > as having "external" apic. So I hardly believe if real external
| > device with ICC bus were used.
| >  
| 
| Details, please.  It might even be a bug in the test.
| 
| 	-hpa
| 

Here is it

	http://lkml.org/lkml/2008/10/17/147
	http://lkml.org/lkml/2008/10/20/144

and commit db96b0a in mainline

	-- Cyrill

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[H. Peter Anvin]

Cyrill Gorcunov wrote:
> [H. Peter Anvin - Fri, May 22, 2009 at 01:23:53PM -0700]
> | Cyrill Gorcunov wrote:
> | > 
> | > It was about a half year ago (need googling to be precise :)
> | > but I was reported about Xeon processor which identify itself
> | > as having "external" apic. So I hardly believe if real external
> | > device with ICC bus were used.
> | >  
> | 
> | Details, please.  It might even be a bug in the test.
> | 
> | 	-hpa
> | 
> 
> Here is it
> 
> 	http://lkml.org/lkml/2008/10/17/147
> 	http://lkml.org/lkml/2008/10/20/144
> 
> and commit db96b0a in mainline
> 

There isn't much detail about the system... no cpuinfo nor any DMI
information.  This makes me believe that the problem was misdiagnosed,
as you yourself speculate at the end of the thread.

It's also possible that this particular Xeon might have a different kind
of APIC, but if so, that's not the same thing as this being a
non-integrated APIC.  Unfortunately Xeon just means "Intel server chip"
and without a CPUID it's impossible to know what CPU that was.

	-hpa

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Maciej W. Rozycki]

On Fri, 22 May 2009, H. Peter Anvin wrote:

> > actually this change could be dangerous. I don't
> > remember if I saw mixed configuration at all but
> > I would not be that sure that we will never met it.

 It is a matter of question whether this change gives any performance 
benefit, but certainly it is not dangerous.  What's the difference in code 
generated?

 Also APIC accesses cannot be cached, so access to the variable if hot in 
the cache should be faster, but OTOH if cold, then main RAM access may 
actually be slower as the APIC is quite closely coupled to the CPU.  Have 
any figures indicating performance change been obtained?

> That seems unlikely in the extreme.  To the best of my knowledge, only
> 486s ever used the external APICs.

 Several Pentium-based systems used external APICs (both ones built around 
original P5 and ones using P54C chips), but mixed configurations were not 
possible because of ICC bus/protocol incompatibility.  First of all the 
number of lines was different (5 vs 3)...

  Maciej

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Cyrill Gorcunov]

[Maciej W. Rozycki - Fri, May 22, 2009 at 11:18:14PM +0100]
| On Fri, 22 May 2009, H. Peter Anvin wrote:
| 
| > > actually this change could be dangerous. I don't
| > > remember if I saw mixed configuration at all but
| > > I would not be that sure that we will never met it.
| 
|  It is a matter of question whether this change gives any performance 
| benefit, but certainly it is not dangerous.  What's the difference in code 
| generated?

About mixing conf -- I thought about integrated apic which behave like
discrete one, not physical mixing of devices which is hard to implement
due to differ bus used (though hw engineers do strange things sometime :)

| 
|  Also APIC accesses cannot be cached, so access to the variable if hot in 
| the cache should be faster, but OTOH if cold, then main RAM access may 
| actually be slower as the APIC is quite closely coupled to the CPU.  Have 
| any figures indicating performance change been obtained?
|

Good point! Though if I'm not missing something the only hot site
is lapic timer since other routines are called either as a part
of bootstrapping or suspend/resume.

| 
| > That seems unlikely in the extreme.  To the best of my knowledge, only
| > 486s ever used the external APICs.
| 
|  Several Pentium-based systems used external APICs (both ones built around 
| original P5 and ones using P54C chips), but mixed configurations were not 
| possible because of ICC bus/protocol incompatibility.  First of all the 
| number of lines was different (5 vs 3)...
| 
|   Maciej
| 

	-- Cyrill

Re: [PATCH] x86,APIC: Detect lapic_is_integrated() once - use on and on.

[Thomas Gleixner]

On Fri, 22 May 2009, Rakib Mullick wrote:
>   Impact: Reduce text space through efficient coding
> 
>  Determine apic is integrated or not - by calling
> lapic_is_integrated() once from APIC_init_uniprocessor() and keep it
> in a variable integrated_lapic. Thus we can determine apic is
> integrated or not, by checking the variable instead of calling
> lapic_is_integrated() on and on. Marking lapic_is_integrated() as
> __init, which reduces some text space. Also allows us to get away from
> messy #ifdef and inline.
> 
> ---
> Signed-off-by: Rakib Mullick <rakib.mullick@gmail.com>
> 
> --- linus/arch/x86/kernel/apic/apic.c	2009-05-21 22:22:15.000000000 +0600
> +++ rakib/arch/x86/kernel/apic/apic.c	2009-05-21 23:27:26.000000000 +0600
> @@ -127,6 +127,9 @@ early_param("nox2apic", setup_nox2apic);
> 
>  unsigned long mp_lapic_addr;
>  int disable_apic;
> +
> +static int integrated_lapic;
> +
>  /* Disable local APIC timer from the kernel commandline or via dmi quirk */
>  static int disable_apic_timer __cpuinitdata;
>  /* Local APIC timer works in C2 */
> @@ -188,13 +191,12 @@ static inline int lapic_get_version(void
>  /*
>   * Check, if the APIC is integrated or a separate chip
>   */
> -static inline int lapic_is_integrated(void)
> +void __init lapic_is_integrated(void)
>  {
> -#ifdef CONFIG_X86_64
> -	return 1;
> -#else
> -	return APIC_INTEGRATED(lapic_get_version());
> -#endif
> +	if (APIC_INTEGRATED(lapic_get_version()))
> +		integrated_lapic = 1;
> +	else
> +		integrated_lapic = 0;
>  }

I agree in general, but the patch is flawed as it prevents the
compiler to optimize the X86_64 case out.

What you really want is:

#ifdef CONFIG_X86_64
# define integrated_lapic (1)
#else
static int integrated_lapic __read_mostly;
#endif

static inline void lapic_is_integrated(void)
{
#ifdef CONFIG_X86_32
	integrate_lapic = APIC_INTEGRATED(lapic_get_version());
#endif
}

So that way the check is only done for 32bit systems and the compiler
will optimize out the code which depends on !integrated_lapic.

Thanks,

	tglx