[Adeos-main] [PATCH] clean up latest ipipe-tracing changes

[Adeos-main] [PATCH] clean up latest ipipe-tracing changes

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 765 bytes --]

Hi,

here is a suggestion to do a tiny cleanup of Gilles' cpuid-fix for the
tracer. Sorry that I didn't comment on this earlier. Nothing critical
though.

I think it's better to drag the include/linux/compiler.h changes into
the main patch instead of starting to use
__attribute__((no_instrument_function)) all over the place just to avoid
notrace (especially as I expect that Ingo's tracer will get merged
anyway in the future). Moreover, local_irq_restore_hw_notrace & frieds
are available even for !CONFIG_IPIPE_TRACE, so no need to differentiate
here. Finally, I do not see a need for marking a static inline function
non-traced.

The patch does not seem to produce different binary code (I checked
kernel/ipipe/tracer.o) - as expected.

Jan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: tracer-cleanup.patch --]
[-- Type: text/x-patch; name="tracer-cleanup.patch", Size: 1868 bytes --]

Index: linux-2.6.16-ipipe/arch/i386/kernel/ipipe-root.c
===================================================================
--- linux-2.6.16-ipipe.orig/arch/i386/kernel/ipipe-root.c	2006-04-17 15:21:58.000000000 +0200
+++ linux-2.6.16-ipipe/arch/i386/kernel/ipipe-root.c	2006-04-17 17:08:19.000000000 +0200
@@ -85,25 +85,13 @@ static void __ipipe_null_handler(unsigne
 
 #ifdef CONFIG_SMP
 
-static __attribute__((no_instrument_function)) int __ipipe_boot_cpuid(void)
+static notrace int __ipipe_boot_cpuid(void)
 {
 	return 0;
 }
 
 u8 __ipipe_apicid_2_cpu[IPIPE_NR_CPUS];
 
-#ifndef CONFIG_IPIPE_TRACE
-static int __ipipe_hard_cpuid(void)
-{
-	unsigned long flags;
-	int cpu;
-
-	local_irq_save_hw(flags);
-	cpu = __ipipe_apicid_2_cpu[GET_APIC_ID(apic_read(APIC_ID))];
-	local_irq_restore_hw(flags);
-	return cpu;
-}
-#else /* CONFIG_IPIPE_TRACE */
 static notrace int __ipipe_hard_cpuid(void)
 {
 	unsigned long flags;
@@ -114,7 +102,6 @@ static notrace int __ipipe_hard_cpuid(vo
 	local_irq_restore_hw_notrace(flags);
 	return cpu;
 }
-#endif /* CONFIG_IPIPE_TRACE */
 
 int (*__ipipe_logical_cpuid)(void) = &__ipipe_boot_cpuid;
 
Index: linux-2.6.16-ipipe/include/asm-i386/ipipe.h
===================================================================
--- linux-2.6.16-ipipe.orig/include/asm-i386/ipipe.h	2006-04-17 15:21:58.000000000 +0200
+++ linux-2.6.16-ipipe/include/asm-i386/ipipe.h	2006-04-17 17:08:18.000000000 +0200
@@ -97,7 +97,7 @@
 #define IPIPE_CRITICAL_VECTOR  0xf9	/* Used by ipipe_critical_enter/exit() */
 #define IPIPE_CRITICAL_IPI     (IPIPE_CRITICAL_VECTOR - FIRST_EXTERNAL_VECTOR)
 
-static inline __attribute__((no_instrument_function)) int ipipe_processor_id(void)
+static inline int ipipe_processor_id(void)
 {
 	extern int (*__ipipe_logical_cpuid)(void);
 	return __ipipe_logical_cpuid();

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

[Adeos-main] Re: [PATCH] clean up latest ipipe-tracing changes

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > (...). Finally, I do not see a need for marking a static inline function
 > non-traced. (...)

Marking a function inline does not guarantee that the said function will
always be inlined, especially since with 2.6.16, inline no longer
necessary means __attribute__((always_inline)).

-- 


					    Gilles Chanteperdrix.

[Adeos-main] Re: [PATCH] clean up latest ipipe-tracing changes

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > (...). Finally, I do not see a need for marking a static inline function
>  > non-traced. (...)
> 
> Marking a function inline does not guarantee that the said function will
> always be inlined, especially since with 2.6.16, inline no longer
> necessary means __attribute__((always_inline)).
> 

Yep, correct, though this particular function should only be non-inlined
by a confused compiler. So, shouldn't we rather convert this function
into a macro or mark it as always_inline? A review of the tracer for
further clarifications is required, I guess.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Adeos-main] Re: [PATCH] clean up latest ipipe-tracing changes

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > Gilles Chanteperdrix wrote:
 > > Jan Kiszka wrote:
 > >  > (...). Finally, I do not see a need for marking a static inline function
 > >  > non-traced. (...)
 > > 
 > > Marking a function inline does not guarantee that the said function will
 > > always be inlined, especially since with 2.6.16, inline no longer
 > > necessary means __attribute__((always_inline)).
 > > 
 > 
 > Yep, correct, though this particular function should only be non-inlined
 > by a confused compiler. So, shouldn't we rather convert this function
 > into a macro or mark it as always_inline? A review of the tracer for
 > further clarifications is required, I guess.

In my humble opinion, the function should not be instrumented. This way,
the tracer will continue to work if the function happens to not been
inlined.

-- 


					    Gilles Chanteperdrix.

Re: [Adeos-main] [PATCH] clean up latest ipipe-tracing changes

[Philippe Gerum]

Jan Kiszka wrote:
> Hi,
> 
> here is a suggestion to do a tiny cleanup of Gilles' cpuid-fix for the
> tracer. Sorry that I didn't comment on this earlier. Nothing critical
> though.
> 
> I think it's better to drag the include/linux/compiler.h changes into
> the main patch instead of starting to use
> __attribute__((no_instrument_function)) all over the place just to avoid
> notrace (especially as I expect that Ingo's tracer will get merged
> anyway in the future). Moreover, local_irq_restore_hw_notrace & frieds
> are available even for !CONFIG_IPIPE_TRACE, so no need to differentiate
> here. Finally, I do not see a need for marking a static inline function
> non-traced.
> 
> The patch does not seem to produce different binary code (I checked
> kernel/ipipe/tracer.o) - as expected.
> 

Applied, leaving the ipipe_processor_id() unchanged though. Thanks.

-- 

Philippe.

Re: [Adeos-main] [PATCH] clean up latest ipipe-tracing changes

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 1026 bytes --]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Hi,
>>
>> here is a suggestion to do a tiny cleanup of Gilles' cpuid-fix for the
>> tracer. Sorry that I didn't comment on this earlier. Nothing critical
>> though.
>>
>> I think it's better to drag the include/linux/compiler.h changes into
>> the main patch instead of starting to use
>> __attribute__((no_instrument_function)) all over the place just to avoid
>> notrace (especially as I expect that Ingo's tracer will get merged
>> anyway in the future). Moreover, local_irq_restore_hw_notrace & frieds
>> are available even for !CONFIG_IPIPE_TRACE, so no need to differentiate
>> here. Finally, I do not see a need for marking a static inline function
>> non-traced.
>>
>> The patch does not seem to produce different binary code (I checked
>> kernel/ipipe/tracer.o) - as expected.
>>
> 
> Applied, leaving the ipipe_processor_id() unchanged though. Thanks.
> 

I still think this makes more sense :) (otherwise, at least switch to
"notrace").

Jan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: tracer-cleanup2.patch --]
[-- Type: text/x-patch; name="tracer-cleanup2.patch", Size: 748 bytes --]

Index: linux-2.6.16-ipipe/include/asm-i386/ipipe.h
===================================================================
--- linux-2.6.16-ipipe.orig/include/asm-i386/ipipe.h
+++ linux-2.6.16-ipipe/include/asm-i386/ipipe.h
@@ -97,11 +97,11 @@
 #define IPIPE_CRITICAL_VECTOR  0xf9	/* Used by ipipe_critical_enter/exit() */
 #define IPIPE_CRITICAL_IPI     (IPIPE_CRITICAL_VECTOR - FIRST_EXTERNAL_VECTOR)
 
-static inline __attribute__((no_instrument_function)) int ipipe_processor_id(void)
-{
-	extern int (*__ipipe_logical_cpuid)(void);
-	return __ipipe_logical_cpuid();
-}
+#define ipipe_processor_id() \
+({ \
+	extern int (*__ipipe_logical_cpuid)(void); \
+	__ipipe_logical_cpuid(); \
+})
 
 extern u8 __ipipe_apicid_2_cpu[];
 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Adeos-main] [PATCH] clean up latest ipipe-tracing changes

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
>>Jan Kiszka wrote:
>>
>>>Hi,
>>>
>>>here is a suggestion to do a tiny cleanup of Gilles' cpuid-fix for the
>>>tracer. Sorry that I didn't comment on this earlier. Nothing critical
>>>though.
>>>
>>>I think it's better to drag the include/linux/compiler.h changes into
>>>the main patch instead of starting to use
>>>__attribute__((no_instrument_function)) all over the place just to avoid
>>>notrace (especially as I expect that Ingo's tracer will get merged
>>>anyway in the future). Moreover, local_irq_restore_hw_notrace & frieds
>>>are available even for !CONFIG_IPIPE_TRACE, so no need to differentiate
>>>here. Finally, I do not see a need for marking a static inline function
>>>non-traced.
>>>
>>>The patch does not seem to produce different binary code (I checked
>>>kernel/ipipe/tracer.o) - as expected.
>>>
>>
>>Applied, leaving the ipipe_processor_id() unchanged though. Thanks.
>>
> 
> 
> I still think this makes more sense :) (otherwise, at least switch to
> "notrace").
> 
> Jan
> 
> 
> ------------------------------------------------------------------------
> 
> Index: linux-2.6.16-ipipe/include/asm-i386/ipipe.h
> ===================================================================
> --- linux-2.6.16-ipipe.orig/include/asm-i386/ipipe.h
> +++ linux-2.6.16-ipipe/include/asm-i386/ipipe.h
> @@ -97,11 +97,11 @@
>  #define IPIPE_CRITICAL_VECTOR  0xf9	/* Used by ipipe_critical_enter/exit() */
>  #define IPIPE_CRITICAL_IPI     (IPIPE_CRITICAL_VECTOR - FIRST_EXTERNAL_VECTOR)
>  
> -static inline __attribute__((no_instrument_function)) int ipipe_processor_id(void)
> -{
> -	extern int (*__ipipe_logical_cpuid)(void);
> -	return __ipipe_logical_cpuid();
> -}
> +#define ipipe_processor_id() \
> +({ \
> +	extern int (*__ipipe_logical_cpuid)(void); \
> +	__ipipe_logical_cpuid(); \
> +})

Applied - moving the declaration out of the macro - Thanks.

-- 

Philippe.