[Xenomai] [PULL, PATCH 0/3] ipipe tracer fixes

[Xenomai] [PULL, PATCH 0/3] ipipe tracer fixes

[Wolfgang Mauerer]

The first two patches are required to build core-4 on x86. The last patch
is necessary when the base kernel is >= 3.1. It can be applied to
3.x with a trivial offset.

The following changes since commit d21e8cdbdcf21ade9878c038e8bd55555ae1d210:

  ipipe: ipipe_timers_request -> ipipe_select_timers (2012-09-03 11:15:35 +0200)

are available in the git repository at:
  https://github.com/siemens/ipipe.git for-upstream

Wolfgang Mauerer (3):
      ipipe: Remove superfluous symbol export of irq_to_desc
      ipipe,x86: Introduce hard_irqs_disabled_flags
      Fix IRQs-off-tracer for x86_64

 arch/x86/include/asm/irqflags.h |    2 ++
 arch/x86/kernel/entry_64.S      |    8 ++++++--
 arch/x86/kernel/ipipe.c         |    3 ---
 3 files changed, 8 insertions(+), 5 deletions(-)

[Xenomai] [PATCH 1/3] ipipe: Remove superfluous symbol export of irq_to_desc

[Wolfgang Mauerer]

It's exported in mainline from 3911ff30f5 onwards.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
---
 arch/x86/kernel/ipipe.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/ipipe.c b/arch/x86/kernel/ipipe.c
index 30515a2..1a56b5f 100644
--- a/arch/x86/kernel/ipipe.c
+++ b/arch/x86/kernel/ipipe.c
@@ -645,9 +645,6 @@ void update_vsyscall_tz(void)
 }
 #endif /* CONFIG_X86_32 */
 
-#ifdef CONFIG_SPARSE_IRQ
-EXPORT_SYMBOL_GPL(irq_to_desc);
-#endif
 struct task_struct *__switch_to(struct task_struct *prev_p,
 				struct task_struct *next_p);
 EXPORT_SYMBOL_GPL(__switch_to);
-- 
1.7.6.4

[Xenomai] [PATCH 2/3] ipipe, x86: Introduce hard_irqs_disabled_flags

[Wolfgang Mauerer]

The function is required by the ipipe tracer.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
---
 arch/x86/include/asm/irqflags.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
index cb58e32..65567b6 100644
--- a/arch/x86/include/asm/irqflags.h
+++ b/arch/x86/include/asm/irqflags.h
@@ -256,6 +256,8 @@ static inline int hard_irqs_disabled(void)
 	return native_irqs_disabled();
 }
 
+#define hard_irqs_disabled_flags(flags)	arch_irqs_disabled_flags(flags)
+
 #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
 
 static inline void hard_local_irq_disable(void)
-- 
1.7.6.4

[Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Wolfgang Mauerer]

Earlier kernels abused the frame pointer to store the
old stack pointer in SAVE_ARGS_IRQ. This behaviour was
changed in upstream commit a2bbe75089d, which breaks
the IRQs-off tracing code for x86_64.

Fix this by basing the argument calculations for
ipipe_trace_begin and ipipe_trace_end on the base
pointer stored in %rsi.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Tested-by: Gernot Hillier <gernot.hillier@siemens.com>
---
 arch/x86/kernel/entry_64.S |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index b523908..693ad08 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -869,6 +869,7 @@ END(interrupt)
 	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
 	SAVE_ARGS_IRQ
 #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
+	/* SAVE_ARGS_IRQ places the frame pointer into %rdi */
 	pushq %rbp
 	leaq RIP-8(%rdi), %rbp	# make interrupted address show up in trace
 	pushq %rdi
@@ -877,13 +878,16 @@ END(interrupt)
 	call ipipe_trace_begin
 	popq %rdi
 	popq %rbp
+	pushq %rdi # Preserve the frame pointer...
 
 	call \func
 
+	popq %rdi # ...  across the function call
 	pushq %rbp
 	pushq %rax
-	movq 8-ARGOFFSET+ORIG_RAX(%rbp), %rdi
-	leaq 8-ARGOFFSET+RIP-8(%rbp), %rbp
+	/* Show interrupted address and IRQ number in the trace, as before */
+	leaq RIP-8(%rdi), %rbp
+	movq ORIG_RAX(%rdi), %rdi
 	notq %rdi
 	call ipipe_trace_end
 	popq %rax
-- 
1.7.6.4

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Gilles Chanteperdrix]

On 09/12/2012 07:00 PM, Wolfgang Mauerer wrote:

> Earlier kernels abused the frame pointer to store the
> old stack pointer in SAVE_ARGS_IRQ. This behaviour was
> changed in upstream commit a2bbe75089d, which breaks
> the IRQs-off tracing code for x86_64.
> 
> Fix this by basing the argument calculations for
> ipipe_trace_begin and ipipe_trace_end on the base
> pointer stored in %rsi.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
> Tested-by: Gernot Hillier <gernot.hillier@siemens.com>
> ---
>  arch/x86/kernel/entry_64.S |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index b523908..693ad08 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -869,6 +869,7 @@ END(interrupt)
>  	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>  	SAVE_ARGS_IRQ
>  #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
> +	/* SAVE_ARGS_IRQ places the frame pointer into %rdi */
>  	pushq %rbp
>  	leaq RIP-8(%rdi), %rbp	# make interrupted address show up in trace
>  	pushq %rdi
> @@ -877,13 +878,16 @@ END(interrupt)
>  	call ipipe_trace_begin
>  	popq %rdi
>  	popq %rbp
> +	pushq %rdi # Preserve the frame pointer...


Can't we do better than that? This looks redundant
pushq %rbp
pushq %rdi
...
popq %rdi
popq %rbp
pushq %rdi

It seems if %rdi was pushed before %rbp we would not need to pop it then
repush it, we could have:

pushq %rdi
pushq %rbp
...
popq %rbp
/* %rdi is fine here */


-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Gilles Chanteperdrix]

On 09/12/2012 08:05 PM, Gilles Chanteperdrix wrote:

> On 09/12/2012 07:00 PM, Wolfgang Mauerer wrote:
> 
>> Earlier kernels abused the frame pointer to store the
>> old stack pointer in SAVE_ARGS_IRQ. This behaviour was
>> changed in upstream commit a2bbe75089d, which breaks
>> the IRQs-off tracing code for x86_64.
>>
>> Fix this by basing the argument calculations for
>> ipipe_trace_begin and ipipe_trace_end on the base
>> pointer stored in %rsi.
>>
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
>> Tested-by: Gernot Hillier <gernot.hillier@siemens.com>
>> ---
>>  arch/x86/kernel/entry_64.S |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
>> index b523908..693ad08 100644
>> --- a/arch/x86/kernel/entry_64.S
>> +++ b/arch/x86/kernel/entry_64.S
>> @@ -869,6 +869,7 @@ END(interrupt)
>>  	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>>  	SAVE_ARGS_IRQ
>>  #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
>> +	/* SAVE_ARGS_IRQ places the frame pointer into %rdi */
>>  	pushq %rbp
>>  	leaq RIP-8(%rdi), %rbp	# make interrupted address show up in trace
>>  	pushq %rdi
>> @@ -877,13 +878,16 @@ END(interrupt)
>>  	call ipipe_trace_begin
>>  	popq %rdi
>>  	popq %rbp
>> +	pushq %rdi # Preserve the frame pointer...
> 
> 
> Can't we do better than that? This looks redundant
> pushq %rbp
> pushq %rdi
> ...
> popq %rdi
> popq %rbp
> pushq %rdi
> 
> It seems if %rdi was pushed before %rbp we would not need to pop it then
> repush it, we could have:
> 
> pushq %rdi
> pushq %rbp
> ...
> popq %rbp
> /* %rdi is fine here */
> 
> 

Ah ok, rdi is the first function argument. Nevertheless, we can
mov (%rsp), %rdi

instead of pop + push

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Gilles Chanteperdrix]

On 09/12/2012 07:00 PM, Wolfgang Mauerer wrote:

> Earlier kernels abused the frame pointer to store the
> old stack pointer in SAVE_ARGS_IRQ. This behaviour was
> changed in upstream commit a2bbe75089d, which breaks
> the IRQs-off tracing code for x86_64.
> 
> Fix this by basing the argument calculations for
> ipipe_trace_begin and ipipe_trace_end on the base
> pointer stored in %rsi.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
> Tested-by: Gernot Hillier <gernot.hillier@siemens.com>
> ---
>  arch/x86/kernel/entry_64.S |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index b523908..693ad08 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -869,6 +869,7 @@ END(interrupt)
>  	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>  	SAVE_ARGS_IRQ
>  #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
> +	/* SAVE_ARGS_IRQ places the frame pointer into %rdi */
>  	pushq %rbp
>  	leaq RIP-8(%rdi), %rbp	# make interrupted address show up in trace
>  	pushq %rdi
> @@ -877,13 +878,16 @@ END(interrupt)
>  	call ipipe_trace_begin
>  	popq %rdi
>  	popq %rbp
> +	pushq %rdi # Preserve the frame pointer...
>  
>  	call \func
>  
> +	popq %rdi # ...  across the function call
>  	pushq %rbp
>  	pushq %rax
> -	movq 8-ARGOFFSET+ORIG_RAX(%rbp), %rdi
> -	leaq 8-ARGOFFSET+RIP-8(%rbp), %rbp
> +	/* Show interrupted address and IRQ number in the trace, as before */
> +	leaq RIP-8(%rdi), %rbp
> +	movq ORIG_RAX(%rdi), %rdi
>  	notq %rdi
>  	call ipipe_trace_end
>  	popq %rax


Looking at that again, it seems we do not need to push %rbp or %rdi, 
%rdi can be computed from %rsp and %rbp was already saved on stack by 
SAVE_ARGS_IRQ.

So, I propose the following:
/* 0(%rsp): ~(interrupt number) */
	.macro interrupt func
	/* reserve pt_regs for scratch regs and rbp */
	subq $ORIG_RAX-RBP, %rsp
	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
	SAVE_ARGS_IRQ
#ifdef CONFIG_IPIPE_TRACE_IRQSOFF
	/* pt_regs is %rdi or %rsp + 8-RBP */
	movq ORIG_RAX+8-RBP(%rsp), %rdi # IRQ number
	leaq RIP-8+8-RBP(%rsp), %rbp  # make interrupted address show up in trace
	notq %rdi			# IRQ number is inverted, fix up
	call ipipe_trace_begin
	leaq 8-RBP(%rsp),%rdi
	movq RBP+8-RBP(%rsp), %rbp

	call \func

	/* Show interrupted address and IRQ number in the trace, as before */
	movq ORIG_RAX+8-RBP(%rsp), %rdi
	leaq RIP-8+8-RBP(%rsp), %rbp
	pushq %rax
	notq %rdi
	call ipipe_trace_end
	popq %rax
	movq RBP+8-RBP(%rsp), %rbp
#else
	call \func
#endif
	.endm


-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Wolfgang Mauerer]

On 13/09/12 08:43, Gilles Chanteperdrix wrote:
> On 09/12/2012 07:00 PM, Wolfgang Mauerer wrote:
> 
>> Earlier kernels abused the frame pointer to store the
>> old stack pointer in SAVE_ARGS_IRQ. This behaviour was
>> changed in upstream commit a2bbe75089d, which breaks
>> the IRQs-off tracing code for x86_64.
>>
>> Fix this by basing the argument calculations for
>> ipipe_trace_begin and ipipe_trace_end on the base
>> pointer stored in %rsi.
>>
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
>> Tested-by: Gernot Hillier <gernot.hillier@siemens.com>
>> ---
>>  arch/x86/kernel/entry_64.S |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
>> index b523908..693ad08 100644
>> --- a/arch/x86/kernel/entry_64.S
>> +++ b/arch/x86/kernel/entry_64.S
>> @@ -869,6 +869,7 @@ END(interrupt)
>>  	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>>  	SAVE_ARGS_IRQ
>>  #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
>> +	/* SAVE_ARGS_IRQ places the frame pointer into %rdi */
>>  	pushq %rbp
>>  	leaq RIP-8(%rdi), %rbp	# make interrupted address show up in trace
>>  	pushq %rdi
>> @@ -877,13 +878,16 @@ END(interrupt)
>>  	call ipipe_trace_begin
>>  	popq %rdi
>>  	popq %rbp
>> +	pushq %rdi # Preserve the frame pointer...
>>  
>>  	call \func
>>  
>> +	popq %rdi # ...  across the function call
>>  	pushq %rbp
>>  	pushq %rax
>> -	movq 8-ARGOFFSET+ORIG_RAX(%rbp), %rdi
>> -	leaq 8-ARGOFFSET+RIP-8(%rbp), %rbp
>> +	/* Show interrupted address and IRQ number in the trace, as before */
>> +	leaq RIP-8(%rdi), %rbp
>> +	movq ORIG_RAX(%rdi), %rdi
>>  	notq %rdi
>>  	call ipipe_trace_end
>>  	popq %rax
> 
> 
> Looking at that again, it seems we do not need to push %rbp or %rdi, 
> %rdi can be computed from %rsp and %rbp was already saved on stack by 
> SAVE_ARGS_IRQ.
> 
> So, I propose the following:
> /* 0(%rsp): ~(interrupt number) */
> 	.macro interrupt func
> 	/* reserve pt_regs for scratch regs and rbp */
> 	subq $ORIG_RAX-RBP, %rsp
> 	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
> 	SAVE_ARGS_IRQ
> #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
> 	/* pt_regs is %rdi or %rsp + 8-RBP */
> 	movq ORIG_RAX+8-RBP(%rsp), %rdi # IRQ number
> 	leaq RIP-8+8-RBP(%rsp), %rbp  # make interrupted address show up in trace
> 	notq %rdi			# IRQ number is inverted, fix up
> 	call ipipe_trace_begin
> 	leaq 8-RBP(%rsp),%rdi
> 	movq RBP+8-RBP(%rsp), %rbp
> 
> 	call \func
> 
> 	/* Show interrupted address and IRQ number in the trace, as before */
> 	movq ORIG_RAX+8-RBP(%rsp), %rdi
> 	leaq RIP-8+8-RBP(%rsp), %rbp
> 	pushq %rax
> 	notq %rdi
> 	call ipipe_trace_end
> 	popq %rax
> 	movq RBP+8-RBP(%rsp), %rbp
> #else
> 	call \func
> #endif
> 	.endm
> 

that fixes the bug, too -- thanks. However, I deliberately spent some
extra assembler code (5 insns compared to your approach) to make the
code easier to  understand (the overall size of the resulting kernel
binary is identical for both approaches on my machine, btw.). Since the
overhead of 5 instructions is negligible compared to the two calls of
__ipipe_trace that are necessary in any case, I'd argue that the more
straightforward code is beneficial in the long run, but that's for the
maintainers to decide.

Best regards, Wolfgang

--
Siemens AG, Corporate Technology
Corporate Competence Centre Embedded Linux

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Gilles Chanteperdrix]

On 09/13/2012 10:46 AM, Wolfgang Mauerer wrote:
> On 13/09/12 08:43, Gilles Chanteperdrix wrote:
>> On 09/12/2012 07:00 PM, Wolfgang Mauerer wrote:
>>
>>> Earlier kernels abused the frame pointer to store the
>>> old stack pointer in SAVE_ARGS_IRQ. This behaviour was
>>> changed in upstream commit a2bbe75089d, which breaks
>>> the IRQs-off tracing code for x86_64.
>>>
>>> Fix this by basing the argument calculations for
>>> ipipe_trace_begin and ipipe_trace_end on the base
>>> pointer stored in %rsi.
>>>
>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
>>> Tested-by: Gernot Hillier <gernot.hillier@siemens.com>
>>> ---
>>>  arch/x86/kernel/entry_64.S |    8 ++++++--
>>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
>>> index b523908..693ad08 100644
>>> --- a/arch/x86/kernel/entry_64.S
>>> +++ b/arch/x86/kernel/entry_64.S
>>> @@ -869,6 +869,7 @@ END(interrupt)
>>>  	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>>>  	SAVE_ARGS_IRQ
>>>  #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
>>> +	/* SAVE_ARGS_IRQ places the frame pointer into %rdi */
>>>  	pushq %rbp
>>>  	leaq RIP-8(%rdi), %rbp	# make interrupted address show up in trace
>>>  	pushq %rdi
>>> @@ -877,13 +878,16 @@ END(interrupt)
>>>  	call ipipe_trace_begin
>>>  	popq %rdi
>>>  	popq %rbp
>>> +	pushq %rdi # Preserve the frame pointer...
>>>  
>>>  	call \func
>>>  
>>> +	popq %rdi # ...  across the function call
>>>  	pushq %rbp
>>>  	pushq %rax
>>> -	movq 8-ARGOFFSET+ORIG_RAX(%rbp), %rdi
>>> -	leaq 8-ARGOFFSET+RIP-8(%rbp), %rbp
>>> +	/* Show interrupted address and IRQ number in the trace, as before */
>>> +	leaq RIP-8(%rdi), %rbp
>>> +	movq ORIG_RAX(%rdi), %rdi
>>>  	notq %rdi
>>>  	call ipipe_trace_end
>>>  	popq %rax
>>
>>
>> Looking at that again, it seems we do not need to push %rbp or %rdi, 
>> %rdi can be computed from %rsp and %rbp was already saved on stack by 
>> SAVE_ARGS_IRQ.
>>
>> So, I propose the following:
>> /* 0(%rsp): ~(interrupt number) */
>> 	.macro interrupt func
>> 	/* reserve pt_regs for scratch regs and rbp */
>> 	subq $ORIG_RAX-RBP, %rsp
>> 	CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>> 	SAVE_ARGS_IRQ
>> #ifdef CONFIG_IPIPE_TRACE_IRQSOFF
>> 	/* pt_regs is %rdi or %rsp + 8-RBP */
>> 	movq ORIG_RAX+8-RBP(%rsp), %rdi # IRQ number
>> 	leaq RIP-8+8-RBP(%rsp), %rbp  # make interrupted address show up in trace
>> 	notq %rdi			# IRQ number is inverted, fix up
>> 	call ipipe_trace_begin
>> 	leaq 8-RBP(%rsp),%rdi
>> 	movq RBP+8-RBP(%rsp), %rbp
>>
>> 	call \func
>>
>> 	/* Show interrupted address and IRQ number in the trace, as before */
>> 	movq ORIG_RAX+8-RBP(%rsp), %rdi
>> 	leaq RIP-8+8-RBP(%rsp), %rbp
>> 	pushq %rax
>> 	notq %rdi
>> 	call ipipe_trace_end
>> 	popq %rax
>> 	movq RBP+8-RBP(%rsp), %rbp
>> #else
>> 	call \func
>> #endif
>> 	.endm
>>
> 
> that fixes the bug, too -- thanks. However, I deliberately spent some
> extra assembler code (5 insns compared to your approach) to make the
> code easier to  understand (the overall size of the resulting kernel
> binary is identical for both approaches on my machine, btw.). Since the
> overhead of 5 instructions is negligible compared to the two calls of
> __ipipe_trace that are necessary in any case, I'd argue that the more
> straightforward code is beneficial in the long run, but that's for the
> maintainers to decide.

To the contrary, the profusion of pushq and popq looked confusing to me,
I wondered why so many were needed, so, I guess it is all a matter of taste.

-- 
					    Gilles.

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Wolfgang Mauerer]

On 13/09/12 11:40, Gilles Chanteperdrix wrote:
> On 09/13/2012 10:46 AM, Wolfgang Mauerer wrote:
>> On 13/09/12 08:43, Gilles Chanteperdrix wrote:

... (some code with varying amounts of pops and pushed)...

>> that fixes the bug, too -- thanks. However, I deliberately spent some
>> extra assembler code (5 insns compared to your approach) to make the
>> code easier to  understand (the overall size of the resulting kernel
>> binary is identical for both approaches on my machine, btw.). Since the
>> overhead of 5 instructions is negligible compared to the two calls of
>> __ipipe_trace that are necessary in any case, I'd argue that the more
>> straightforward code is beneficial in the long run, but that's for the
>> maintainers to decide.
> 
> To the contrary, the profusion of pushq and popq looked confusing to me,
> I wondered why so many were needed, so, I guess it is all a matter of taste.

most certainly a matter of taste, agreed. I've rearranged your
suggestion slightly, added a comment or two, and turned it into a
patch.  Repo at https://github.com/siemens/ipipe.git for-upstream
is also updated.

Best regards, Wolfgang

--
Siemens AG, Corporate Technology
Corporate Competence Centre Embedded Linux
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-IRQs-off-tracer-for-x86_64.patch
Type: text/x-patch
Size: 2271 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20120913/72bc651d/attachment.bin>

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Gilles Chanteperdrix]

On 09/13/2012 02:58 PM, Wolfgang Mauerer wrote:
> On 13/09/12 11:40, Gilles Chanteperdrix wrote:
>> On 09/13/2012 10:46 AM, Wolfgang Mauerer wrote:
>>> On 13/09/12 08:43, Gilles Chanteperdrix wrote:
> 
> ... (some code with varying amounts of pops and pushed)...
> 
>>> that fixes the bug, too -- thanks. However, I deliberately spent some
>>> extra assembler code (5 insns compared to your approach) to make the
>>> code easier to  understand (the overall size of the resulting kernel
>>> binary is identical for both approaches on my machine, btw.). Since the
>>> overhead of 5 instructions is negligible compared to the two calls of
>>> __ipipe_trace that are necessary in any case, I'd argue that the more
>>> straightforward code is beneficial in the long run, but that's for the
>>> maintainers to decide.
>>
>> To the contrary, the profusion of pushq and popq looked confusing to me,
>> I wondered why so many were needed, so, I guess it is all a matter of taste.
> 
> most certainly a matter of taste, agreed. I've rearranged your
> suggestion slightly, added a comment or two, and turned it into a
> patch.  Repo at https://github.com/siemens/ipipe.git for-upstream
> is also updated.

Ok. Small nit:

+	movq ORIG_RAX+8-RBP(%rsp), %rdi # IRQ number...
 	notq %rdi			# ...is inverted, fix up
+	leaq RIP-8+8-RBP(%rsp), %rbp    # Show interrupted address in trace

I would avoid to use a register as a source in an instruction right
after an instruction where it was used as a destination, because I
believe in processors which do not have out of order execution, such as
Intel Atoms, the processor would have to wait for an instruction to be
finished to start the next. Though I do not know enough of the x86
architecture to know if that is true. That is the reason why I put leaq
before notq.

-- 
					    Gilles.

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Wolfgang Mauerer]

On 13/09/12 15:26, Gilles Chanteperdrix wrote:
> On 09/13/2012 02:58 PM, Wolfgang Mauerer wrote:
>> On 13/09/12 11:40, Gilles Chanteperdrix wrote:
>>> On 09/13/2012 10:46 AM, Wolfgang Mauerer wrote:
>>>> On 13/09/12 08:43, Gilles Chanteperdrix wrote:
>> 
>> ... (some code with varying amounts of pops and pushed)...
>> 
>>>> that fixes the bug, too -- thanks. However, I deliberately
>>>> spent some extra assembler code (5 insns compared to your
>>>> approach) to make the code easier to  understand (the overall
>>>> size of the resulting kernel binary is identical for both
>>>> approaches on my machine, btw.). Since the overhead of 5
>>>> instructions is negligible compared to the two calls of 
>>>> __ipipe_trace that are necessary in any case, I'd argue that
>>>> the more straightforward code is beneficial in the long run,
>>>> but that's for the maintainers to decide.
>>> 
>>> To the contrary, the profusion of pushq and popq looked
>>> confusing to me, I wondered why so many were needed, so, I
>>> guess it is all a matter of taste.
>> 
>> most certainly a matter of taste, agreed. I've rearranged your 
>> suggestion slightly, added a comment or two, and turned it into
>> a patch.  Repo at https://github.com/siemens/ipipe.git
>> for-upstream is also updated.
> 
> Ok. Small nit:
> 
> +	movq ORIG_RAX+8-RBP(%rsp), %rdi # IRQ number... notq %rdi			#
> ...is inverted, fix up +	leaq RIP-8+8-RBP(%rsp), %rbp    # Show
> interrupted address in trace
> 
> I would avoid to use a register as a source in an instruction
> right after an instruction where it was used as a destination,
> because I believe in processors which do not have out of order
> execution, such as Intel Atoms, the processor would have to wait
> for an instruction to be finished to start the next. Though I do
> not know enough of the x86 architecture to know if that is true.
> That is the reason why I put leaq before notq.

thanks for the suggestion. Indeed even on a 2.7GHz Xeon where all CPU
trickery is available, the assembler instruction reordering gives a
speed-up of about 0.35ns, so one cycle. However, for any
application with such extreme performance needs, tracing would be
turned off anyway, wouldn't it? For debugging purposes, it does make no
difference whatsoever, so I'd argue that code clarity is the more
valuable optimisation goal in this case.

Best regards, Wolfgang

Re: [Xenomai] [PATCH 3/3] Fix IRQs-off-tracer for x86_64

[Gilles Chanteperdrix]

On 09/13/2012 06:25 PM, Wolfgang Mauerer wrote:
> On 13/09/12 15:26, Gilles Chanteperdrix wrote:
>> On 09/13/2012 02:58 PM, Wolfgang Mauerer wrote:
>>> On 13/09/12 11:40, Gilles Chanteperdrix wrote:
>>>> On 09/13/2012 10:46 AM, Wolfgang Mauerer wrote:
>>>>> On 13/09/12 08:43, Gilles Chanteperdrix wrote:
>>>
>>> ... (some code with varying amounts of pops and pushed)...
>>>
>>>>> that fixes the bug, too -- thanks. However, I deliberately
>>>>> spent some extra assembler code (5 insns compared to your
>>>>> approach) to make the code easier to  understand (the overall
>>>>> size of the resulting kernel binary is identical for both
>>>>> approaches on my machine, btw.). Since the overhead of 5
>>>>> instructions is negligible compared to the two calls of 
>>>>> __ipipe_trace that are necessary in any case, I'd argue that
>>>>> the more straightforward code is beneficial in the long run,
>>>>> but that's for the maintainers to decide.
>>>>
>>>> To the contrary, the profusion of pushq and popq looked
>>>> confusing to me, I wondered why so many were needed, so, I
>>>> guess it is all a matter of taste.
>>>
>>> most certainly a matter of taste, agreed. I've rearranged your 
>>> suggestion slightly, added a comment or two, and turned it into
>>> a patch.  Repo at https://github.com/siemens/ipipe.git
>>> for-upstream is also updated.
>>
>> Ok. Small nit:
>>
>> +	movq ORIG_RAX+8-RBP(%rsp), %rdi # IRQ number... notq %rdi			#
>> ...is inverted, fix up +	leaq RIP-8+8-RBP(%rsp), %rbp    # Show
>> interrupted address in trace
>>
>> I would avoid to use a register as a source in an instruction
>> right after an instruction where it was used as a destination,
>> because I believe in processors which do not have out of order
>> execution, such as Intel Atoms, the processor would have to wait
>> for an instruction to be finished to start the next. Though I do
>> not know enough of the x86 architecture to know if that is true.
>> That is the reason why I put leaq before notq.
> 
> thanks for the suggestion. Indeed even on a 2.7GHz Xeon where all CPU
> trickery is available, the assembler instruction reordering gives a
> speed-up of about 0.35ns, so one cycle. However, for any
> application with such extreme performance needs, tracing would be
> turned off anyway, wouldn't it? For debugging purposes, it does make no
> difference whatsoever, so I'd argue that code clarity is the more
> valuable optimisation goal in this case.

The tracer speed is what makes the tracer usable or not, at all, on low
end platforms (where it is probably the most needed). I agree that a few
cycles at this point are not so important.

On the other hand, I do not see how inverting two instructions makes the
code more or less readable. It is assembly, it is not readable, no
matter what. And it is so easy to put the instructions in the right
order that I see no reason to avoid doing it.

Anyway, I will merge your patches, thanks for taking care of this issue.

-- 
					    Gilles.