[Xenomai] [GIT PULL] core-5 for x86

[Xenomai] [GIT PULL] core-5 for x86

[Wolfgang Mauerer]

Dear all,

here's a rebase of the x86-specific bits of core-4 to core-5. I've
included all x86 specific changes that are not yet in core-5, and
also added the patches I sent earlier for core-4. I did not include a
separate patch for the mechanical changes required to apply the
x86 base patch on top of core-5, but can surely do so if desired.

Cheers, Wolfgang

The following changes since commit 9f90a923093f411908e3536088dfdb1d936f3fe8:                                                

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

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

Gilles Chanteperdrix (5):
      ipipe/x86: provide ipipe_head_switch_mm
      ipipe/x86: do not restore during context switch
      ipipe/x86: mask io_apic EOI irq before EOI     
      ipipe/x86: fix IOAPIC with CONFIG_IRQ_REMAP    
      ipipe/x86: fix compilation for AMD processors  

Philippe Gerum (1):
      x86/ipipe: ipipe_head_switch_mm -> ipipe_switch_mm_head

Wolfgang Mauerer (5):
      ipipe-core4-x86 applied to core-5
      x86/ipipe: Make io_apic_level_ack_pending available for ipipe
      ipipe: Remove superfluous symbol export of irq_to_desc       
      ipipe,x86: Introduce hard_irqs_disabled_flags                
      Fix IRQs-off-tracer for x86_64                               

 .gitignore                            |    1 +
 arch/x86/Kconfig                      |   26 +-
 arch/x86/include/asm/apic.h           |    6 + 
 arch/x86/include/asm/apicdef.h        |    3 + 
 arch/x86/include/asm/fpu-internal.h   |   10 + 
 arch/x86/include/asm/hw_irq.h         |   10 + 
 arch/x86/include/asm/i8259.h          |    2 +-
 arch/x86/include/asm/ipi.h            |   23 +-
 arch/x86/include/asm/ipipe.h          |  105 ++++++
 arch/x86/include/asm/ipipe_32.h       |   86 +++++ 
 arch/x86/include/asm/ipipe_64.h       |   90 +++++ 
 arch/x86/include/asm/ipipe_base.h     |  226 +++++++++++
 arch/x86/include/asm/irq_vectors.h    |   11 +          
 arch/x86/include/asm/irqflags.h       |  210 +++++++++++
 arch/x86/include/asm/mmu_context.h    |   21 +-         
 arch/x86/include/asm/page_64_types.h  |    4 +          
 arch/x86/include/asm/processor.h      |    1 +          
 arch/x86/include/asm/special_insns.h  |   12 +          
 arch/x86/include/asm/switch_to.h      |    7 +-         
 arch/x86/include/asm/thread_info.h    |    2 +          
 arch/x86/include/asm/traps.h          |    2 +-         
 arch/x86/include/asm/tsc.h            |    1 +          
 arch/x86/kernel/Makefile              |    1 +          
 arch/x86/kernel/apic/apic.c           |   39 ++-        
 arch/x86/kernel/apic/apic_flat_64.c   |    4 +-         
 arch/x86/kernel/apic/io_apic.c        |  219 ++++++++++--
 arch/x86/kernel/apic/ipi.c            |   20 +-          
 arch/x86/kernel/apic/x2apic_cluster.c |    4 +-          
 arch/x86/kernel/apic/x2apic_phys.c    |    4 +-          
 arch/x86/kernel/cpu/mtrr/cyrix.c      |   12 +-          
 arch/x86/kernel/cpu/mtrr/generic.c    |   12 +-          
 arch/x86/kernel/dumpstack_32.c        |    3 +           
 arch/x86/kernel/dumpstack_64.c        |    5 +           
 arch/x86/kernel/entry_32.S            |  147 ++++++--    
 arch/x86/kernel/entry_64.S            |  247 +++++++++++--
 arch/x86/kernel/hpet.c                |   27 ++-          
 arch/x86/kernel/i387.c                |    3 +
 arch/x86/kernel/i8259.c               |   30 ++-
 arch/x86/kernel/ipipe.c               |  664 +++++++++++++++++++++++++++++++++
 arch/x86/kernel/irq.c                 |    7 +-
 arch/x86/kernel/irqinit.c             |    7 +
 arch/x86/kernel/process.c             |   21 +-
 arch/x86/kernel/process_32.c          |    4 +-
 arch/x86/kernel/process_64.c          |    9 +-
 arch/x86/kernel/ptrace.c              |    5 +
 arch/x86/kernel/smp.c                 |    4 +-
 arch/x86/kernel/smpboot.c             |   10 +-
 arch/x86/kernel/traps.c               |    4 +
 arch/x86/kernel/tsc.c                 |   12 +-
 arch/x86/kernel/vm86_32.c             |    4 +
 arch/x86/kernel/vsyscall_64.c         |    4 +
 arch/x86/kvm/svm.c                    |    4 +-
 arch/x86/kvm/vmx.c                    |   13 +-
 arch/x86/kvm/x86.c                    |   69 +++-
 arch/x86/lib/mmx_32.c                 |    2 +-
 arch/x86/lib/thunk_64.S               |   21 +
 arch/x86/mm/fault.c                   |   56 +++-
 arch/x86/mm/tlb.c                     |    7 +
 arch/x86/platform/uv/tlb_uv.c         |    5 +
 59 files changed, 2384 insertions(+), 184 deletions(-)
 create mode 100644 arch/x86/include/asm/ipipe.h
 create mode 100644 arch/x86/include/asm/ipipe_32.h
 create mode 100644 arch/x86/include/asm/ipipe_64.h
 create mode 100644 arch/x86/include/asm/ipipe_base.h
 create mode 100644 arch/x86/kernel/ipipe.c

Re: [Xenomai] [GIT PULL] core-5 for x86

[Gilles Chanteperdrix]

On 09/18/2012 04:11 PM, Wolfgang Mauerer wrote:
> Dear all,
> 
> here's a rebase of the x86-specific bits of core-4 to core-5. I've
> included all x86 specific changes that are not yet in core-5, and
> also added the patches I sent earlier for core-4. I did not include a
> separate patch for the mechanical changes required to apply the
> x86 base patch on top of core-5, but can surely do so if desired.

I am not quite finished with x86 on 3.4. So, I would like to start 3.5
from the finishing point on 3.4. There are already commits in my branch
which you did not take:

http://git.xenomai.org/?p=ipipe-gch.git;a=shortlog;h=refs/heads/for-core-3.4

This is assuming that I am the (flaky subsitute for a) maintainer of the
x86 architecture. Of course, if someone wants to take over the
maintenance of the x86 architecture, I am gladly returning to the ARMs.


4-x86 applied to core-5
>       x86/ipipe: Make io_apic_level_ack_pending available for ipipe

What is this commit? Neither the text nor the diff are very explicit.

-- 
					    Gilles.

Re: [Xenomai] [GIT PULL] core-5 for x86

[Wolfgang Mauerer]

On 18/09/12 16:25, Gilles Chanteperdrix wrote:
> On 09/18/2012 04:11 PM, Wolfgang Mauerer wrote:
>> Dear all,
>>
>> here's a rebase of the x86-specific bits of core-4 to core-5. I've
>> included all x86 specific changes that are not yet in core-5, and
>> also added the patches I sent earlier for core-4. I did not include a
>> separate patch for the mechanical changes required to apply the
>> x86 base patch on top of core-5, but can surely do so if desired.
> 
> I am not quite finished with x86 on 3.4. So, I would like to start 3.5
> from the finishing point on 3.4. There are already commits in my branch
> which you did not take:
> 
> http://git.xenomai.org/?p=ipipe-gch.git;a=shortlog;h=refs/heads/for-core-3.4
that's true; my last pull was too old. I'll add the corresponding
commits to the tree (FYI, the purpose of this tree is mainly to do some
experiments with the latest ipipe release and the latest kernel, and
I wanted to make sure that work is not duplicated in case someone else
is pursuing similar goals)
> 
> This is assuming that I am the (flaky subsitute for a) maintainer of the
> x86 architecture. Of course, if someone wants to take over the
> maintenance of the x86 architecture, I am gladly returning to the ARMs.
> 
> 4-x86 applied to core-5
>>       x86/ipipe: Make io_apic_level_ack_pending available for ipipe
> 
> What is this commit? Neither the text nor the diff are very explicit.
yes, this commit is fairly big considering its small effect. I've updated
the description as follows:

Make sure that io_apic_level_ack_pending() is compiled in when ipipe is
configured. Also move the implementation downwards so that it is
not referenced before it is defined.

Best regards, Wolfgang

Re: [Xenomai] [GIT PULL] core-5 for x86

[Gilles Chanteperdrix]

On 09/18/2012 05:27 PM, Wolfgang Mauerer wrote:

> On 18/09/12 16:25, Gilles Chanteperdrix wrote:
>> On 09/18/2012 04:11 PM, Wolfgang Mauerer wrote:
>>> Dear all,
>>>
>>> here's a rebase of the x86-specific bits of core-4 to core-5. I've
>>> included all x86 specific changes that are not yet in core-5, and
>>> also added the patches I sent earlier for core-4. I did not include a
>>> separate patch for the mechanical changes required to apply the
>>> x86 base patch on top of core-5, but can surely do so if desired.
>>
>> I am not quite finished with x86 on 3.4. So, I would like to start 3.5
>> from the finishing point on 3.4. There are already commits in my branch
>> which you did not take:
>>
>> http://git.xenomai.org/?p=ipipe-gch.git;a=shortlog;h=refs/heads/for-core-3.4
> that's true; my last pull was too old. I'll add the corresponding
> commits to the tree (FYI, the purpose of this tree is mainly to do some
> experiments with the latest ipipe release and the latest kernel, and
> I wanted to make sure that work is not duplicated in case someone else
> is pursuing similar goals)


Ok. We have a currently pending issue on x86 which you should be
informed about before discovering it during your tests: using
rthal_supported_cpus is broken in I-pipe core patches when using the
LAPIC timer: since there is only one irq handler for all the LAPIC
timers, the handler is registered on all cpus, but on non started cpus,
the handler will do nothing at best, and not foward the LAPIC ticks to
Linux (which is still in control of the LAPIC timer on these cpus).

This problem is due to the fact that we keep the same vector as Linux,
and so the same irq. There are two ways out of this:

- change the LAPIC vector when xenomai takes the control of the LAPIC
timer, like we use to do, this is racy with current code because the
timer is taken by Xenomai but still used a bit by Linux, before it is
programmed by Xenomai, and Xenomai assumes that the host tick irq is the
same as the timer irq. All this can be fixed, but the last drawback of
this approach is that it does not fix the issue on architectures where
the local timer irq is the same on all cpus, but can not be changed,
hence the second approach;
- the second approach is to add a test at the beginning of
xnintr_clock_handler and forward the irq to the root domain if the
current cpu does not belong to xnarch_supported_cpus. This means some
patching of I-pipe timers so that ipipe_percpu.hrtimer_irq also gets
defined for non supported cpus when they use the timer shared with other
cpus, essentially what this patch tries (but fails) to achieve:

http://www.xenomai.org/pipermail/xenomai/2012-September/026066.html


-- 
                                                                Gilles.

Re: [Xenomai] [GIT PULL] core-5 for x86

[Wolfgang Mauerer]

On 18/09/12 21:36, Gilles Chanteperdrix wrote:
> On 09/18/2012 05:27 PM, Wolfgang Mauerer wrote:
> 
>> On 18/09/12 16:25, Gilles Chanteperdrix wrote:
>>> On 09/18/2012 04:11 PM, Wolfgang Mauerer wrote:
>>>> Dear all,
>>>>
>>>> here's a rebase of the x86-specific bits of core-4 to core-5. I've
>>>> included all x86 specific changes that are not yet in core-5, and
>>>> also added the patches I sent earlier for core-4. I did not include a
>>>> separate patch for the mechanical changes required to apply the
>>>> x86 base patch on top of core-5, but can surely do so if desired.
>>>
>>> I am not quite finished with x86 on 3.4. So, I would like to start 3.5
>>> from the finishing point on 3.4. There are already commits in my branch
>>> which you did not take:
>>>
>>> http://git.xenomai.org/?p=ipipe-gch.git;a=shortlog;h=refs/heads/for-core-3.4
>> that's true; my last pull was too old. I'll add the corresponding
>> commits to the tree (FYI, the purpose of this tree is mainly to do some
>> experiments with the latest ipipe release and the latest kernel, and
>> I wanted to make sure that work is not duplicated in case someone else
>> is pursuing similar goals)
> 
> 
> Ok. We have a currently pending issue on x86 which you should be
> informed about before discovering it during your tests: using
> rthal_supported_cpus is broken in I-pipe core patches when using the
> LAPIC timer: since there is only one irq handler for all the LAPIC
> timers, the handler is registered on all cpus, but on non started cpus,
> the handler will do nothing at best, and not foward the LAPIC ticks to
> Linux (which is still in control of the LAPIC timer on these cpus).
> 
> This problem is due to the fact that we keep the same vector as Linux,
> and so the same irq. There are two ways out of this:
> 
> - change the LAPIC vector when xenomai takes the control of the LAPIC
> timer, like we use to do, this is racy with current code because the
> timer is taken by Xenomai but still used a bit by Linux, before it is
> programmed by Xenomai, and Xenomai assumes that the host tick irq is the
> same as the timer irq. All this can be fixed, but the last drawback of
> this approach is that it does not fix the issue on architectures where
> the local timer irq is the same on all cpus, but can not be changed,
> hence the second approach;
> - the second approach is to add a test at the beginning of
> xnintr_clock_handler and forward the irq to the root domain if the
> current cpu does not belong to xnarch_supported_cpus. This means some
> patching of I-pipe timers so that ipipe_percpu.hrtimer_irq also gets
> defined for non supported cpus when they use the timer shared with other
> cpus, essentially what this patch tries (but fails) to achieve:
> 
> http://www.xenomai.org/pipermail/xenomai/2012-September/026066.html

thanks for the info! Unfortunately, I was not able to reproduce this
issue quickly using two latency instances bound to different CPUs on a
machine booted with isolcpus=1, though.

I did also not see the issue with core-5, but that's because
bdeaf76ba30a517 (ipipe/x86: use the vector_irq for system vectors too)
leads to a stuck CPU bringup for >1 CPUs. Will maybe look into that a
bit further.

Best regards, Wolfgang

Re: [Xenomai] [GIT PULL] core-5 for x86

[Gilles Chanteperdrix]

Wolfgang Mauerer wrote:
> On 18/09/12 21:36, Gilles Chanteperdrix wrote:
>> On 09/18/2012 05:27 PM, Wolfgang Mauerer wrote:
>>
>>> On 18/09/12 16:25, Gilles Chanteperdrix wrote:
>>>> On 09/18/2012 04:11 PM, Wolfgang Mauerer wrote:
>>>>> Dear all,
>>>>>
>>>>> here's a rebase of the x86-specific bits of core-4 to core-5. I've
>>>>> included all x86 specific changes that are not yet in core-5, and
>>>>> also added the patches I sent earlier for core-4. I did not include a
>>>>> separate patch for the mechanical changes required to apply the
>>>>> x86 base patch on top of core-5, but can surely do so if desired.
>>>>
>>>> I am not quite finished with x86 on 3.4. So, I would like to start 3.5
>>>> from the finishing point on 3.4. There are already commits in my
>>>> branch
>>>> which you did not take:
>>>>
>>>> http://git.xenomai.org/?p=ipipe-gch.git;a=shortlog;h=refs/heads/for-core-3.4
>>> that's true; my last pull was too old. I'll add the corresponding
>>> commits to the tree (FYI, the purpose of this tree is mainly to do some
>>> experiments with the latest ipipe release and the latest kernel, and
>>> I wanted to make sure that work is not duplicated in case someone else
>>> is pursuing similar goals)
>>
>>
>> Ok. We have a currently pending issue on x86 which you should be
>> informed about before discovering it during your tests: using
>> rthal_supported_cpus is broken in I-pipe core patches when using the
>> LAPIC timer: since there is only one irq handler for all the LAPIC
>> timers, the handler is registered on all cpus, but on non started cpus,
>> the handler will do nothing at best, and not foward the LAPIC ticks to
>> Linux (which is still in control of the LAPIC timer on these cpus).
>>
>> This problem is due to the fact that we keep the same vector as Linux,
>> and so the same irq. There are two ways out of this:
>>
>> - change the LAPIC vector when xenomai takes the control of the LAPIC
>> timer, like we use to do, this is racy with current code because the
>> timer is taken by Xenomai but still used a bit by Linux, before it is
>> programmed by Xenomai, and Xenomai assumes that the host tick irq is the
>> same as the timer irq. All this can be fixed, but the last drawback of
>> this approach is that it does not fix the issue on architectures where
>> the local timer irq is the same on all cpus, but can not be changed,
>> hence the second approach;
>> - the second approach is to add a test at the beginning of
>> xnintr_clock_handler and forward the irq to the root domain if the
>> current cpu does not belong to xnarch_supported_cpus. This means some
>> patching of I-pipe timers so that ipipe_percpu.hrtimer_irq also gets
>> defined for non supported cpus when they use the timer shared with other
>> cpus, essentially what this patch tries (but fails) to achieve:
>>
>> http://www.xenomai.org/pipermail/xenomai/2012-September/026066.html
>
> thanks for the info! Unfortunately, I was not able to reproduce this
> issue quickly using two latency instances bound to different CPUs on a
> machine booted with isolcpus=1, though.
>
> I did also not see the issue with core-5, but that's because
> bdeaf76ba30a517 (ipipe/x86: use the vector_irq for system vectors too)
> leads to a stuck CPU bringup for >1 CPUs. Will maybe look into that a
> bit further.
>
> Best regards, Wolfgang

Yes, the commit is broken. We discussed this with Jan extensively.

-- 
                    Gilles.

Re: [Xenomai] [GIT PULL] core-5 for x86

[Gilles Chanteperdrix]

On 09/19/2012 02:15 PM, Wolfgang Mauerer wrote:
> On 18/09/12 21:36, Gilles Chanteperdrix wrote:
>> On 09/18/2012 05:27 PM, Wolfgang Mauerer wrote:
>>
>>> On 18/09/12 16:25, Gilles Chanteperdrix wrote:
>>>> On 09/18/2012 04:11 PM, Wolfgang Mauerer wrote:
>>>>> Dear all,
>>>>>
>>>>> here's a rebase of the x86-specific bits of core-4 to core-5. I've
>>>>> included all x86 specific changes that are not yet in core-5, and
>>>>> also added the patches I sent earlier for core-4. I did not include a
>>>>> separate patch for the mechanical changes required to apply the
>>>>> x86 base patch on top of core-5, but can surely do so if desired.
>>>>
>>>> I am not quite finished with x86 on 3.4. So, I would like to start 3.5
>>>> from the finishing point on 3.4. There are already commits in my branch
>>>> which you did not take:
>>>>
>>>> http://git.xenomai.org/?p=ipipe-gch.git;a=shortlog;h=refs/heads/for-core-3.4
>>> that's true; my last pull was too old. I'll add the corresponding
>>> commits to the tree (FYI, the purpose of this tree is mainly to do some
>>> experiments with the latest ipipe release and the latest kernel, and
>>> I wanted to make sure that work is not duplicated in case someone else
>>> is pursuing similar goals)
>>
>>
>> Ok. We have a currently pending issue on x86 which you should be
>> informed about before discovering it during your tests: using
>> rthal_supported_cpus is broken in I-pipe core patches when using the
>> LAPIC timer: since there is only one irq handler for all the LAPIC
>> timers, the handler is registered on all cpus, but on non started cpus,
>> the handler will do nothing at best, and not foward the LAPIC ticks to
>> Linux (which is still in control of the LAPIC timer on these cpus).
>>
>> This problem is due to the fact that we keep the same vector as Linux,
>> and so the same irq. There are two ways out of this:
>>
>> - change the LAPIC vector when xenomai takes the control of the LAPIC
>> timer, like we use to do, this is racy with current code because the
>> timer is taken by Xenomai but still used a bit by Linux, before it is
>> programmed by Xenomai, and Xenomai assumes that the host tick irq is the
>> same as the timer irq. All this can be fixed, but the last drawback of
>> this approach is that it does not fix the issue on architectures where
>> the local timer irq is the same on all cpus, but can not be changed,
>> hence the second approach;
>> - the second approach is to add a test at the beginning of
>> xnintr_clock_handler and forward the irq to the root domain if the
>> current cpu does not belong to xnarch_supported_cpus. This means some
>> patching of I-pipe timers so that ipipe_percpu.hrtimer_irq also gets
>> defined for non supported cpus when they use the timer shared with other
>> cpus, essentially what this patch tries (but fails) to achieve:
>>
>> http://www.xenomai.org/pipermail/xenomai/2012-September/026066.html
> 
> thanks for the info! Unfortunately, I was not able to reproduce this
> issue quickly using two latency instances bound to different CPUs on a
> machine booted with isolcpus=1, though.

The issue is not with isolcpus, but with rthal_supported_cpus, that is
xenomai "supported_cpus" parameter. And maybe Linux is running fine with
the bug, but if you do a cat /proc/interrupts, you should see the local
timer interrupt no longer incrementing on cpus not intercepted by xenomai.


-- 
					    Gilles.

Re: [Xenomai] [GIT PULL] core-5 for x86

[Wolfgang Mauerer]

On 20/09/12 18:11, Gilles Chanteperdrix wrote:
> On 09/19/2012 02:15 PM, Wolfgang Mauerer wrote:
>> On 18/09/12 21:36, Gilles Chanteperdrix wrote:
>>> On 09/18/2012 05:27 PM, Wolfgang Mauerer wrote:

>>> Ok. We have a currently pending issue on x86 which you should be
>>> informed about before discovering it during your tests: using
>>> rthal_supported_cpus is broken in I-pipe core patches when using the
>>> LAPIC timer: since there is only one irq handler for all the LAPIC
>>> timers, the handler is registered on all cpus, but on non started cpus,
>>> the handler will do nothing at best, and not foward the LAPIC ticks to
>>> Linux (which is still in control of the LAPIC timer on these cpus).
>>>
>>> This problem is due to the fact that we keep the same vector as Linux,
>>> and so the same irq. There are two ways out of this:
>>>
>>> - change the LAPIC vector when xenomai takes the control of the LAPIC
>>> timer, like we use to do, this is racy with current code because the
>>> timer is taken by Xenomai but still used a bit by Linux, before it is
>>> programmed by Xenomai, and Xenomai assumes that the host tick irq is the
>>> same as the timer irq. All this can be fixed, but the last drawback of
>>> this approach is that it does not fix the issue on architectures where
>>> the local timer irq is the same on all cpus, but can not be changed,
>>> hence the second approach;
>>> - the second approach is to add a test at the beginning of
>>> xnintr_clock_handler and forward the irq to the root domain if the
>>> current cpu does not belong to xnarch_supported_cpus. This means some
>>> patching of I-pipe timers so that ipipe_percpu.hrtimer_irq also gets
>>> defined for non supported cpus when they use the timer shared with other
>>> cpus, essentially what this patch tries (but fails) to achieve:
>>>
>>> http://www.xenomai.org/pipermail/xenomai/2012-September/026066.html
>>
>> thanks for the info! Unfortunately, I was not able to reproduce this
>> issue quickly using two latency instances bound to different CPUs on a
>> machine booted with isolcpus=1, though.
> 
> The issue is not with isolcpus, but with rthal_supported_cpus, that is
> xenomai "supported_cpus" parameter. And maybe Linux is running fine with
> the bug, but if you do a cat /proc/interrupts, you should see the local
> timer interrupt no longer incrementing on cpus not intercepted by xenomai.
 
the attached patch (against core-4), which is a slight modification of 
yours, resolves the issue for me when applied together with the xnintr 
patch (I've also rebased to core-5, see https://github.com/siemens/ipipe core-3.5_for-upstream)

Cheers, Wolfgang

########################################################################

Use ipipe_percpu.hrtimer_irq for non-RT CPUs when required

We need to be able to forward the timer interrupt to the
root domain on CPUs that are not used for real-time loads,
but use a timer shared with other CPUs. Based on a patch
by Gilles Chanteperdrix.

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

diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
index 7dcd725..67ae4a7 100644
--- a/kernel/ipipe/timer.c
+++ b/kernel/ipipe/timer.c
@@ -172,11 +172,23 @@ int ipipe_select_timers(const struct cpumask *mask)
 	hrclock_khz = tmp;
 
 	spin_lock_irqsave(&lock, flags);
-	for_each_cpu(cpu, mask) {
+	for_each_cpu(cpu, cpu_online_mask) {
 		list_for_each_entry(t, &timers, link) {
 			if (!cpumask_test_cpu(cpu, t->cpumask))
 				continue;
 
+			/*
+			 * When the CPU is not used for real-time
+			 * loads, we need to be able to forward the
+			 * IRQ for this CPU to the root domain in case
+			 * it is shared with other CPUs.
+			 */
+			if (!cpumask_test_cpu(cpu, mask)
+			    && t->irq == per_cpu(ipipe_percpu.hrtimer_irq, 0)) {
+				per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
+				goto found;
+			}
+
 			evtdev = t->host_timer;
 #ifdef CONFIG_GENERIC_CLOCKEVENTS
 			if (!evtdev
@@ -184,6 +196,9 @@ int ipipe_select_timers(const struct cpumask *mask)
 #endif /* CONFIG_GENERIC_CLOCKEVENTS */
 				goto found;
 		}
+		if (!cpumask_test_cpu(cpu, mask))
+			continue;
+
 		printk("I-pipe: could not find timer for cpu #%d\n", cpu);
 		goto err_remove_all;
 
-- 
1.7.1

Re: [Xenomai] [GIT PULL] core-5 for x86

[Gilles Chanteperdrix]

On 09/25/2012 04:45 PM, Wolfgang Mauerer wrote:
> +			if (!cpumask_test_cpu(cpu, mask)
> +			    && t->irq == per_cpu(ipipe_percpu.hrtimer_irq, 0)) {
> +				per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
> +				goto found;

This only works if cpu 0 is part of the "supported cpus" mask. I think
we need two loops.

-- 
					    Gilles.

Re: [Xenomai] [GIT PULL] core-5 for x86

[Wolfgang Mauerer]

On 25/09/12 16:57, Gilles Chanteperdrix wrote:
> On 09/25/2012 04:45 PM, Wolfgang Mauerer wrote:
>> +			if (!cpumask_test_cpu(cpu, mask)
>> +			    && t->irq == per_cpu(ipipe_percpu.hrtimer_irq, 0)) {
>> +				per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
>> +				goto found;
> 
> This only works if cpu 0 is part of the "supported cpus" mask. I think
> we need two loops.
> 
true, CPU 0 is not necessarily used for Xenomai, which was the case in
my test. Will fix that.

Thanks, Wolfgang

[Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Wolfgang Mauerer]

Make the control flow more readable. No functional changes.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
---
 kernel/ipipe/timer.c |   82 +++++++++++++++++++++++++++++++------------------
 1 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
index 8b2eb6f..765340e 100644
--- a/kernel/ipipe/timer.c
+++ b/kernel/ipipe/timer.c
@@ -154,21 +154,55 @@ static void ipipe_timer_request_sync(void)
 	timer->request(timer, steal);
 }
 
+/* Set up a timer as per-cpu timer for ipipe */
+static int install_pcpu_timer(unsigned cpu, unsigned hrclock_freq,
+			      struct ipipe_timer *t) {
+	unsigned hrtimer_freq;
+	unsigned long long tmp;
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+	struct clock_event_device *evtdev;
+	evtdev = t->host_timer;
+
+	if (evtdev && evtdev->mode == CLOCK_EVT_MODE_SHUTDOWN)
+		return 0;
+#endif /* CONFIG_GENERIC_CLOCKEVENTS */
+
+	if (__ipipe_hrtimer_freq == 0)
+		__ipipe_hrtimer_freq = t->freq;
+
+	per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
+	per_cpu(percpu_timer, cpu) = t;
+
+	hrtimer_freq = t->freq;
+	if (__ipipe_hrclock_freq > UINT_MAX)
+		hrtimer_freq /= 1000;
+
+	t->c2t_integ = hrtimer_freq / hrclock_freq;
+	tmp = (((unsigned long long)
+		(hrtimer_freq % hrclock_freq)) << 32)
+		+ hrclock_freq - 1;
+	do_div(tmp, hrclock_freq);
+	t->c2t_frac = tmp;
+
+	return 1;
+}
+
+
 /*
- * choose per-cpu timer: we walk the list, and find the timer with the
- * highest rating.
+ * Choose per-cpu timers with the highest rating by traversing the
+ * rating-sorted list for each CPU.
  */
 int ipipe_select_timers(const struct cpumask *mask)
 {
-	struct clock_event_device *evtdev;
-	unsigned hrclock_freq, hrtimer_freq;
+	unsigned hrclock_freq;
 	unsigned long long tmp;
 	struct ipipe_timer *t;
 	unsigned long flags;
-	unsigned cpu, khz;
+	unsigned cpu;
+	bool found;
 
-	khz = __ipipe_hrclock_freq > UINT_MAX;
-	if (khz) {
+	if (__ipipe_hrclock_freq > UINT_MAX) {
 		tmp = __ipipe_hrclock_freq;
 		do_div(tmp, 1000);
 		hrclock_freq = tmp;
@@ -177,34 +211,22 @@ int ipipe_select_timers(const struct cpumask *mask)
 
 	spin_lock_irqsave(&lock, flags);
 	for_each_cpu(cpu, mask) {
+		found = false;
 		list_for_each_entry(t, &timers, link) {
 			if (!cpumask_test_cpu(cpu, t->cpumask))
 				continue;
 
-			evtdev = t->host_timer;
-#ifdef CONFIG_GENERIC_CLOCKEVENTS
-			if (!evtdev
-			    || evtdev->mode != CLOCK_EVT_MODE_SHUTDOWN)
-#endif /* CONFIG_GENERIC_CLOCKEVENTS */
-				goto found;
+			if (install_pcpu_timer(cpu, hrclock_freq, t)) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			printk("I-pipe: could not find timer for cpu #%d\n",
+			       cpu);
+			goto err_remove_all;
 		}
-		printk("I-pipe: could not find timer for cpu #%d\n", cpu);
-		goto err_remove_all;
-
-	  found:
-		if (__ipipe_hrtimer_freq == 0)
-			__ipipe_hrtimer_freq = t->freq;
-		per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
-		per_cpu(percpu_timer, cpu) = t;
-		hrtimer_freq = t->freq;
-		if (khz)
-			hrtimer_freq /= 1000;
-		t->c2t_integ = hrtimer_freq / hrclock_freq;
-		tmp = (((unsigned long long)
-			(hrtimer_freq % hrclock_freq)) << 32)
-			+ hrclock_freq - 1;
-		do_div(tmp, hrclock_freq);
-		t->c2t_frac = tmp;
 	}
 	spin_unlock_irqrestore(&lock, flags);
 
-- 
1.7.1

[Xenomai] [PATCH 2/2] Register high-res timer irq for non-ipipe timers if necessary

[Wolfgang Mauerer]

If a CPU that is not supported by ipipe, but used by Linux
shares an IRQ with a timer utilised by ipipe, it must be
possible to forward IRQs received on behalf of the Linux CPU.
Based on a patch by Gilles Chanteperdrix.

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

diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
index 765340e..3b5e13e 100644
--- a/kernel/ipipe/timer.c
+++ b/kernel/ipipe/timer.c
@@ -188,6 +188,21 @@ static int install_pcpu_timer(unsigned cpu, unsigned hrclock_freq,
 	return 1;
 }
 
+static void select_root_only_timer(unsigned cpu, unsigned hrclock_khz,
+				   const struct cpumask *mask,
+				   struct ipipe_timer *t) {
+	unsigned icpu;
+
+	/*
+	 * If no ipipe-supported CPU shares an interrupt with the
+	 * timer, we do not need to care about it.
+	 */
+	for_each_cpu(icpu, mask) {
+		if (t->irq == per_cpu(ipipe_percpu.hrtimer_irq, icpu))
+			if (install_pcpu_timer(cpu, hrclock_khz, t))
+				break;
+	}
+}
 
 /*
  * Choose per-cpu timers with the highest rating by traversing the
@@ -201,6 +216,7 @@ int ipipe_select_timers(const struct cpumask *mask)
 	unsigned long flags;
 	unsigned cpu;
 	bool found;
+	cpumask_t fixup;
 
 	if (__ipipe_hrclock_freq > UINT_MAX) {
 		tmp = __ipipe_hrclock_freq;
@@ -210,6 +226,8 @@ int ipipe_select_timers(const struct cpumask *mask)
 		hrclock_freq = __ipipe_hrclock_freq;
 
 	spin_lock_irqsave(&lock, flags);
+
+	/* First, choose timers for the CPUs handled by ipipe */
 	for_each_cpu(cpu, mask) {
 		found = false;
 		list_for_each_entry(t, &timers, link) {
@@ -228,6 +246,24 @@ int ipipe_select_timers(const struct cpumask *mask)
 			goto err_remove_all;
 		}
 	}
+
+	/*
+	 * Second, check if we need to fix up any CPUs not supported
+	 * by ipipe (but by Linux) whose interrupt may need to be
+	 * forwarded because they have the same IRQ as an ipipe-enabled
+	 * timer.
+	 */
+	cpumask_andnot(&fixup, cpu_online_mask, mask);
+
+	for_each_cpu(cpu, &fixup) {
+		list_for_each_entry(t, &timers, link) {
+			if (!cpumask_test_cpu(cpu, t->cpumask))
+				continue;
+
+			select_root_only_timer(cpu, hrclock_freq, mask, t);
+		}
+	}
+
 	spin_unlock_irqrestore(&lock, flags);
 
 	flags = ipipe_critical_enter(ipipe_timer_request_sync);
-- 
1.7.1

Re: [Xenomai] [GIT PULL] core-5 for x86

[Wolfgang Mauerer]

On 25/09/12 16:57, Gilles Chanteperdrix wrote:
> On 09/25/2012 04:45 PM, Wolfgang Mauerer wrote:
>> +			if (!cpumask_test_cpu(cpu, mask)
>> +			    && t->irq == per_cpu(ipipe_percpu.hrtimer_irq, 0)) {
>> +				per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
>> +				goto found;
> 
> This only works if cpu 0 is part of the "supported cpus" mask. I think
> we need two loops.
> 
I've prepared a version that first handles all CPUs that run ipipe, and
then makes another pass at setting up the CPUs that may need to forward
ticks. Since the code is already deeply enough nested as is, I refactored
it a bit before. Since it runs only once at initialisation time, I did not
optimise for speed. Patches follow as reply.
Forward-port to core-5 is on https://github.com/siemens/ipipe.git core-3.5_for-upstream

Cheers, Wolfgang

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Gilles Chanteperdrix]

On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:

> Make the control flow more readable. No functional changes.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
> ---
>  kernel/ipipe/timer.c |   82 +++++++++++++++++++++++++++++++------------------
>  1 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
> index 8b2eb6f..765340e 100644
> --- a/kernel/ipipe/timer.c
> +++ b/kernel/ipipe/timer.c
> @@ -154,21 +154,55 @@ static void ipipe_timer_request_sync(void)
>  	timer->request(timer, steal);
>  }
>  
> +/* Set up a timer as per-cpu timer for ipipe */
> +static int install_pcpu_timer(unsigned cpu, unsigned hrclock_freq,
> +			      struct ipipe_timer *t) {
> +	unsigned hrtimer_freq;
> +	unsigned long long tmp;
> +
> +#ifdef CONFIG_GENERIC_CLOCKEVENTS
> +	struct clock_event_device *evtdev;
> +	evtdev = t->host_timer;
> +
> +	if (evtdev && evtdev->mode == CLOCK_EVT_MODE_SHUTDOWN)
> +		return 0;
> +#endif /* CONFIG_GENERIC_CLOCKEVENTS */
> +
> +	if (__ipipe_hrtimer_freq == 0)
> +		__ipipe_hrtimer_freq = t->freq;
> +
> +	per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
> +	per_cpu(percpu_timer, cpu) = t;
> +
> +	hrtimer_freq = t->freq;
> +	if (__ipipe_hrclock_freq > UINT_MAX)
> +		hrtimer_freq /= 1000;
> +
> +	t->c2t_integ = hrtimer_freq / hrclock_freq;
> +	tmp = (((unsigned long long)
> +		(hrtimer_freq % hrclock_freq)) << 32)
> +		+ hrclock_freq - 1;
> +	do_div(tmp, hrclock_freq);
> +	t->c2t_frac = tmp;
> +
> +	return 1;
> +}
> +
> +
>  /*
> - * choose per-cpu timer: we walk the list, and find the timer with the
> - * highest rating.
> + * Choose per-cpu timers with the highest rating by traversing the
> + * rating-sorted list for each CPU.
>   */
>  int ipipe_select_timers(const struct cpumask *mask)
>  {
> -	struct clock_event_device *evtdev;
> -	unsigned hrclock_freq, hrtimer_freq;
> +	unsigned hrclock_freq;
>  	unsigned long long tmp;
>  	struct ipipe_timer *t;
>  	unsigned long flags;
> -	unsigned cpu, khz;
> +	unsigned cpu;
> +	bool found;
>  
> -	khz = __ipipe_hrclock_freq > UINT_MAX;
> -	if (khz) {
> +	if (__ipipe_hrclock_freq > UINT_MAX) {
>  		tmp = __ipipe_hrclock_freq;
>  		do_div(tmp, 1000);
>  		hrclock_freq = tmp;
> @@ -177,34 +211,22 @@ int ipipe_select_timers(const struct cpumask *mask)
>  
>  	spin_lock_irqsave(&lock, flags);
>  	for_each_cpu(cpu, mask) {
> +		found = false;
>  		list_for_each_entry(t, &timers, link) {
>  			if (!cpumask_test_cpu(cpu, t->cpumask))
>  				continue;
>  
> -			evtdev = t->host_timer;
> -#ifdef CONFIG_GENERIC_CLOCKEVENTS
> -			if (!evtdev
> -			    || evtdev->mode != CLOCK_EVT_MODE_SHUTDOWN)
> -#endif /* CONFIG_GENERIC_CLOCKEVENTS */
> -				goto found;
> +			if (install_pcpu_timer(cpu, hrclock_freq, t)) {
> +				found = true;


Talking about readability, I find a goto with a clear label name much
more readable than a flag. So, NACK this patch, please keep the goto.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Wolfgang Mauerer]

On 26/09/12 23:28, Gilles Chanteperdrix wrote:
> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
> 
>> Make the control flow more readable. No functional changes.
>>
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
>> ---
>>  kernel/ipipe/timer.c |   82 +++++++++++++++++++++++++++++++------------------
>>  1 files changed, 52 insertions(+), 30 deletions(-)
>>
>> diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
>> index 8b2eb6f..765340e 100644
>> --- a/kernel/ipipe/timer.c
>> +++ b/kernel/ipipe/timer.c
>> @@ -154,21 +154,55 @@ static void ipipe_timer_request_sync(void)
>>  	timer->request(timer, steal);
>>  }
>>  
>> +/* Set up a timer as per-cpu timer for ipipe */
>> +static int install_pcpu_timer(unsigned cpu, unsigned hrclock_freq,
>> +			      struct ipipe_timer *t) {
>> +	unsigned hrtimer_freq;
>> +	unsigned long long tmp;
>> +
>> +#ifdef CONFIG_GENERIC_CLOCKEVENTS
>> +	struct clock_event_device *evtdev;
>> +	evtdev = t->host_timer;
>> +
>> +	if (evtdev && evtdev->mode == CLOCK_EVT_MODE_SHUTDOWN)
>> +		return 0;
>> +#endif /* CONFIG_GENERIC_CLOCKEVENTS */
>> +
>> +	if (__ipipe_hrtimer_freq == 0)
>> +		__ipipe_hrtimer_freq = t->freq;
>> +
>> +	per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
>> +	per_cpu(percpu_timer, cpu) = t;
>> +
>> +	hrtimer_freq = t->freq;
>> +	if (__ipipe_hrclock_freq > UINT_MAX)
>> +		hrtimer_freq /= 1000;
>> +
>> +	t->c2t_integ = hrtimer_freq / hrclock_freq;
>> +	tmp = (((unsigned long long)
>> +		(hrtimer_freq % hrclock_freq)) << 32)
>> +		+ hrclock_freq - 1;
>> +	do_div(tmp, hrclock_freq);
>> +	t->c2t_frac = tmp;
>> +
>> +	return 1;
>> +}
>> +
>> +
>>  /*
>> - * choose per-cpu timer: we walk the list, and find the timer with the
>> - * highest rating.
>> + * Choose per-cpu timers with the highest rating by traversing the
>> + * rating-sorted list for each CPU.
>>   */
>>  int ipipe_select_timers(const struct cpumask *mask)
>>  {
>> -	struct clock_event_device *evtdev;
>> -	unsigned hrclock_freq, hrtimer_freq;
>> +	unsigned hrclock_freq;
>>  	unsigned long long tmp;
>>  	struct ipipe_timer *t;
>>  	unsigned long flags;
>> -	unsigned cpu, khz;
>> +	unsigned cpu;
>> +	bool found;
>>  
>> -	khz = __ipipe_hrclock_freq > UINT_MAX;
>> -	if (khz) {
>> +	if (__ipipe_hrclock_freq > UINT_MAX) {
>>  		tmp = __ipipe_hrclock_freq;
>>  		do_div(tmp, 1000);
>>  		hrclock_freq = tmp;
>> @@ -177,34 +211,22 @@ int ipipe_select_timers(const struct cpumask *mask)
>>  
>>  	spin_lock_irqsave(&lock, flags);
>>  	for_each_cpu(cpu, mask) {
>> +		found = false;
>>  		list_for_each_entry(t, &timers, link) {
>>  			if (!cpumask_test_cpu(cpu, t->cpumask))
>>  				continue;
>>  
>> -			evtdev = t->host_timer;
>> -#ifdef CONFIG_GENERIC_CLOCKEVENTS
>> -			if (!evtdev
>> -			    || evtdev->mode != CLOCK_EVT_MODE_SHUTDOWN)
>> -#endif /* CONFIG_GENERIC_CLOCKEVENTS */
>> -				goto found;
>> +			if (install_pcpu_timer(cpu, hrclock_freq, t)) {
>> +				found = true;
> 
> 
> Talking about readability, I find a goto with a clear label name much
> more readable than a flag. So, NACK this patch, please keep the goto.
 
So you're against the refactoring, or only against using the flag?
Keeping the goto leads to something like

	if (install_pcpu_timer(cpu, hrclock_freq, t))
		goto found
(...)
found:    ;

since we need a statement for the label, but nothing is left to do.
I find this fairly ugly, but if you prefer it over a flag, then
so be it.

Thanks, Wolfgang

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Gilles Chanteperdrix]

On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
>>
>>> Make the control flow more readable. No functional changes.
>>>
>>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
>>> ---
>>>  kernel/ipipe/timer.c |   82 +++++++++++++++++++++++++++++++------------------
>>>  1 files changed, 52 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
>>> index 8b2eb6f..765340e 100644
>>> --- a/kernel/ipipe/timer.c
>>> +++ b/kernel/ipipe/timer.c
>>> @@ -154,21 +154,55 @@ static void ipipe_timer_request_sync(void)
>>>  	timer->request(timer, steal);
>>>  }
>>>  
>>> +/* Set up a timer as per-cpu timer for ipipe */
>>> +static int install_pcpu_timer(unsigned cpu, unsigned hrclock_freq,
>>> +			      struct ipipe_timer *t) {
>>> +	unsigned hrtimer_freq;
>>> +	unsigned long long tmp;
>>> +
>>> +#ifdef CONFIG_GENERIC_CLOCKEVENTS
>>> +	struct clock_event_device *evtdev;
>>> +	evtdev = t->host_timer;
>>> +
>>> +	if (evtdev && evtdev->mode == CLOCK_EVT_MODE_SHUTDOWN)
>>> +		return 0;
>>> +#endif /* CONFIG_GENERIC_CLOCKEVENTS */
>>> +
>>> +	if (__ipipe_hrtimer_freq == 0)
>>> +		__ipipe_hrtimer_freq = t->freq;
>>> +
>>> +	per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
>>> +	per_cpu(percpu_timer, cpu) = t;
>>> +
>>> +	hrtimer_freq = t->freq;
>>> +	if (__ipipe_hrclock_freq > UINT_MAX)
>>> +		hrtimer_freq /= 1000;
>>> +
>>> +	t->c2t_integ = hrtimer_freq / hrclock_freq;
>>> +	tmp = (((unsigned long long)
>>> +		(hrtimer_freq % hrclock_freq)) << 32)
>>> +		+ hrclock_freq - 1;
>>> +	do_div(tmp, hrclock_freq);
>>> +	t->c2t_frac = tmp;
>>> +
>>> +	return 1;
>>> +}
>>> +
>>> +
>>>  /*
>>> - * choose per-cpu timer: we walk the list, and find the timer with the
>>> - * highest rating.
>>> + * Choose per-cpu timers with the highest rating by traversing the
>>> + * rating-sorted list for each CPU.
>>>   */
>>>  int ipipe_select_timers(const struct cpumask *mask)
>>>  {
>>> -	struct clock_event_device *evtdev;
>>> -	unsigned hrclock_freq, hrtimer_freq;
>>> +	unsigned hrclock_freq;
>>>  	unsigned long long tmp;
>>>  	struct ipipe_timer *t;
>>>  	unsigned long flags;
>>> -	unsigned cpu, khz;
>>> +	unsigned cpu;
>>> +	bool found;
>>>  
>>> -	khz = __ipipe_hrclock_freq > UINT_MAX;
>>> -	if (khz) {
>>> +	if (__ipipe_hrclock_freq > UINT_MAX) {
>>>  		tmp = __ipipe_hrclock_freq;
>>>  		do_div(tmp, 1000);
>>>  		hrclock_freq = tmp;
>>> @@ -177,34 +211,22 @@ int ipipe_select_timers(const struct cpumask *mask)
>>>  
>>>  	spin_lock_irqsave(&lock, flags);
>>>  	for_each_cpu(cpu, mask) {
>>> +		found = false;
>>>  		list_for_each_entry(t, &timers, link) {
>>>  			if (!cpumask_test_cpu(cpu, t->cpumask))
>>>  				continue;
>>>  
>>> -			evtdev = t->host_timer;
>>> -#ifdef CONFIG_GENERIC_CLOCKEVENTS
>>> -			if (!evtdev
>>> -			    || evtdev->mode != CLOCK_EVT_MODE_SHUTDOWN)
>>> -#endif /* CONFIG_GENERIC_CLOCKEVENTS */
>>> -				goto found;
>>> +			if (install_pcpu_timer(cpu, hrclock_freq, t)) {
>>> +				found = true;
>>
>>
>> Talking about readability, I find a goto with a clear label name much
>> more readable than a flag. So, NACK this patch, please keep the goto.
>  
> So you're against the refactoring, or only against using the flag?
> Keeping the goto leads to something like
> 
> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
> 		goto found
> (...)
> found:    ;
> 
> since we need a statement for the label, but nothing is left to do.
> I find this fairly ugly, but if you prefer it over a flag, then
> so be it.

Then use return instead of goto...

-- 
					    Gilles.

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Wolfgang Mauerer]

On 27/09/12 14:04, Gilles Chanteperdrix wrote:
> On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
>> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
(...)

>>> Talking about readability, I find a goto with a clear label name much
>>> more readable than a flag. So, NACK this patch, please keep the goto.
>>  
>> So you're against the refactoring, or only against using the flag?
>> Keeping the goto leads to something like
>>
>> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
>> 		goto found
>> (...)
>> found:    ;
>>
>> since we need a statement for the label, but nothing is left to do.
>> I find this fairly ugly, but if you prefer it over a flag, then
>> so be it.
> 
> Then use return instead of goto...

Won't work -- that skips the rest of the enclosing per_cpu loop and
the second part of the function introduced in the follow-up commit
that does the actual bugfixing.

Since I take the flag is the issue and not the refactoring as such, 
please find an updated patch with a goto below.

Cheers, Wolfgang

######################################################################
>From 713f567194520ba6bc32935402218f629d1fa134 Mon Sep 17 00:00:00 2001
From: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
Date: Wed, 26 Sep 2012 13:38:44 +0200
Subject: [PATCH 1/1] Refactor ipipe_select_timers

Make the control flow more readable. No functional changes.

Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>
---
 kernel/ipipe/timer.c |   73 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/kernel/ipipe/timer.c b/kernel/ipipe/timer.c
index 8b2eb6f..360fc33 100644
--- a/kernel/ipipe/timer.c
+++ b/kernel/ipipe/timer.c
@@ -154,21 +154,54 @@ static void ipipe_timer_request_sync(void)
 	timer->request(timer, steal);
 }
 
+/* Set up a timer as per-cpu timer for ipipe */
+static int install_pcpu_timer(unsigned cpu, unsigned hrclock_freq,
+			      struct ipipe_timer *t) {
+	unsigned hrtimer_freq;
+	unsigned long long tmp;
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+	struct clock_event_device *evtdev;
+	evtdev = t->host_timer;
+
+	if (evtdev && evtdev->mode == CLOCK_EVT_MODE_SHUTDOWN)
+		return 0;
+#endif /* CONFIG_GENERIC_CLOCKEVENTS */
+
+	if (__ipipe_hrtimer_freq == 0)
+		__ipipe_hrtimer_freq = t->freq;
+
+	per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
+	per_cpu(percpu_timer, cpu) = t;
+
+	hrtimer_freq = t->freq;
+	if (__ipipe_hrclock_freq > UINT_MAX)
+		hrtimer_freq /= 1000;
+
+	t->c2t_integ = hrtimer_freq / hrclock_freq;
+	tmp = (((unsigned long long)
+		(hrtimer_freq % hrclock_freq)) << 32)
+		+ hrclock_freq - 1;
+	do_div(tmp, hrclock_freq);
+	t->c2t_frac = tmp;
+
+	return 1;
+}
+
+
 /*
- * choose per-cpu timer: we walk the list, and find the timer with the
- * highest rating.
+ * Choose per-cpu timers with the highest rating by traversing the
+ * rating-sorted list for each CPU.
  */
 int ipipe_select_timers(const struct cpumask *mask)
 {
-	struct clock_event_device *evtdev;
-	unsigned hrclock_freq, hrtimer_freq;
+	unsigned hrclock_freq;
 	unsigned long long tmp;
 	struct ipipe_timer *t;
 	unsigned long flags;
-	unsigned cpu, khz;
+	unsigned cpu;
 
-	khz = __ipipe_hrclock_freq > UINT_MAX;
-	if (khz) {
+	if (__ipipe_hrclock_freq > UINT_MAX) {
 		tmp = __ipipe_hrclock_freq;
 		do_div(tmp, 1000);
 		hrclock_freq = tmp;
@@ -181,30 +214,14 @@ int ipipe_select_timers(const struct cpumask *mask)
 			if (!cpumask_test_cpu(cpu, t->cpumask))
 				continue;
 
-			evtdev = t->host_timer;
-#ifdef CONFIG_GENERIC_CLOCKEVENTS
-			if (!evtdev
-			    || evtdev->mode != CLOCK_EVT_MODE_SHUTDOWN)
-#endif /* CONFIG_GENERIC_CLOCKEVENTS */
+			if (install_pcpu_timer(cpu, hrclock_freq, t))
 				goto found;
 		}
-		printk("I-pipe: could not find timer for cpu #%d\n", cpu);
-		goto err_remove_all;
 
-	  found:
-		if (__ipipe_hrtimer_freq == 0)
-			__ipipe_hrtimer_freq = t->freq;
-		per_cpu(ipipe_percpu.hrtimer_irq, cpu) = t->irq;
-		per_cpu(percpu_timer, cpu) = t;
-		hrtimer_freq = t->freq;
-		if (khz)
-			hrtimer_freq /= 1000;
-		t->c2t_integ = hrtimer_freq / hrclock_freq;
-		tmp = (((unsigned long long)
-			(hrtimer_freq % hrclock_freq)) << 32)
-			+ hrclock_freq - 1;
-		do_div(tmp, hrclock_freq);
-		t->c2t_frac = tmp;
+		printk("I-pipe: could not find timer for cpu #%d\n",
+		       cpu);
+		goto err_remove_all;
+found:		;
 	}
 	spin_unlock_irqrestore(&lock, flags);
 
-- 
1.7.1

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Gilles Chanteperdrix]

On 09/27/2012 02:47 PM, Wolfgang Mauerer wrote:
> On 27/09/12 14:04, Gilles Chanteperdrix wrote:
>> On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
>>> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>>>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
> (...)
> 
>>>> Talking about readability, I find a goto with a clear label name much
>>>> more readable than a flag. So, NACK this patch, please keep the goto.
>>>  
>>> So you're against the refactoring, or only against using the flag?
>>> Keeping the goto leads to something like
>>>
>>> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
>>> 		goto found
>>> (...)
>>> found:    ;
>>>
>>> since we need a statement for the label, but nothing is left to do.
>>> I find this fairly ugly, but if you prefer it over a flag, then
>>> so be it.
>>
>> Then use return instead of goto...
> 
> Won't work -- that skips the rest of the enclosing per_cpu loop and
> the second part of the function introduced in the follow-up commit
> that does the actual bugfixing.
> 
> Since I take the flag is the issue and not the refactoring as such, 
> please find an updated patch with a goto below.

Sorry, I do not get it, do you have a gitweb somewhere where I can see
the actual code?

-- 
					    Gilles.

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Wolfgang Mauerer]

On 27/09/12 14:54, Gilles Chanteperdrix wrote:
> On 09/27/2012 02:47 PM, Wolfgang Mauerer wrote:
>> On 27/09/12 14:04, Gilles Chanteperdrix wrote:
>>> On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
>>>> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>>>>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
>> (...)
>>
>>>>> Talking about readability, I find a goto with a clear label name much
>>>>> more readable than a flag. So, NACK this patch, please keep the goto.
>>>>  
>>>> So you're against the refactoring, or only against using the flag?
>>>> Keeping the goto leads to something like
>>>>
>>>> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
>>>> 		goto found
>>>> (...)
>>>> found:    ;
>>>>
>>>> since we need a statement for the label, but nothing is left to do.
>>>> I find this fairly ugly, but if you prefer it over a flag, then
>>>> so be it.
>>>
>>> Then use return instead of goto...
>>
>> Won't work -- that skips the rest of the enclosing per_cpu loop and
>> the second part of the function introduced in the follow-up commit
>> that does the actual bugfixing.
>>
>> Since I take the flag is the issue and not the refactoring as such, 
>> please find an updated patch with a goto below.
> 
> Sorry, I do not get it, do you have a gitweb somewhere where I can see
> the actual code?
> 
sure: https://github.com/siemens/ipipe/commits/core-3.5_for-upstream

Cheers, Wolfgang

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Gilles Chanteperdrix]

On 09/27/2012 02:47 PM, Wolfgang Mauerer wrote:

> On 27/09/12 14:04, Gilles Chanteperdrix wrote:
>> On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
>>> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>>>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
> (...)
> 
>>>> Talking about readability, I find a goto with a clear label name much
>>>> more readable than a flag. So, NACK this patch, please keep the goto.
>>>  
>>> So you're against the refactoring, or only against using the flag?
>>> Keeping the goto leads to something like
>>>
>>> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
>>> 		goto found
>>> (...)
>>> found:    ;
>>>
>>> since we need a statement for the label, but nothing is left to do.
>>> I find this fairly ugly, but if you prefer it over a flag, then
>>> so be it.
>>
>> Then use return instead of goto...
> 
> Won't work -- that skips the rest of the enclosing per_cpu loop and
> the second part of the function introduced in the follow-up commit
> that does the actual bugfixing.
> 
> Since I take the flag is the issue and not the refactoring as such, 
> please find an updated patch with a goto below.


Oh boy, you love functions, do you? What I would do is: keep the test
for evtdev->mode outside of the install_pcpu_timer function so that we
clearly see in the loop that it is the only reason for continuing the
loop. Then use the goto found, and at the found label, call the now void
function install_pcpu_timer. Ok for you?

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 2/2] Register high-res timer irq for non-ipipe timers if necessary

[Gilles Chanteperdrix]

On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:

> If a CPU that is not supported by ipipe, but used by Linux
> shares an IRQ with a timer utilised by ipipe, it must be
> possible to forward IRQs received on behalf of the Linux CPU.
> Based on a patch by Gilles Chanteperdrix.
> 
> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer@siemens.com>


That is much better than the original patch, you are even allowing to
have a mixed HPET/LAPIC timers configuration on x86. I guess you can now
suppress the test hpet_msi_capability_lookup and register an ipipe_timer
for each HPET timer unconditionally.

Regards.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Wolfgang Mauerer]

On 27/09/12 20:33, Gilles Chanteperdrix wrote:
> On 09/27/2012 02:47 PM, Wolfgang Mauerer wrote:
> 
>> On 27/09/12 14:04, Gilles Chanteperdrix wrote:
>>> On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
>>>> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>>>>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
>> (...)
>>
>>>>> Talking about readability, I find a goto with a clear label name much
>>>>> more readable than a flag. So, NACK this patch, please keep the goto.
>>>>  
>>>> So you're against the refactoring, or only against using the flag?
>>>> Keeping the goto leads to something like
>>>>
>>>> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
>>>> 		goto found
>>>> (...)
>>>> found:    ;
>>>>
>>>> since we need a statement for the label, but nothing is left to do.
>>>> I find this fairly ugly, but if you prefer it over a flag, then
>>>> so be it.
>>>
>>> Then use return instead of goto...
>>
>> Won't work -- that skips the rest of the enclosing per_cpu loop and
>> the second part of the function introduced in the follow-up commit
>> that does the actual bugfixing.
>>
>> Since I take the flag is the issue and not the refactoring as such, 
>> please find an updated patch with a goto below.
> 
> 
> Oh boy, you love functions, do you? What I would do is: keep the test
sure() :)
> for evtdev->mode outside of the install_pcpu_timer function so that we
> clearly see in the loop that it is the only reason for continuing the
> loop. Then use the goto found, and at the found label, call the now void
> function install_pcpu_timer. Ok for you?
> 
okay, changed the code as desired to introduce more labels and use the 
preprocessor more often. Please pick 23b2de46314 and b738a3b624 from 
https://github.com/siemens/ipipe core-3.5_for-upstream (apply also
cleanly to core-4).

Best regards, Wolfgang

Re: [Xenomai] [PATCH 1/2] Refactor ipipe_select_timers

[Gilles Chanteperdrix]

On 09/28/2012 10:32 AM, Wolfgang Mauerer wrote:

> On 27/09/12 20:33, Gilles Chanteperdrix wrote:
>> On 09/27/2012 02:47 PM, Wolfgang Mauerer wrote:
>>
>>> On 27/09/12 14:04, Gilles Chanteperdrix wrote:
>>>> On 09/27/2012 10:28 AM, Wolfgang Mauerer wrote:
>>>>> On 26/09/12 23:28, Gilles Chanteperdrix wrote:
>>>>>> On 09/26/2012 03:16 PM, Wolfgang Mauerer wrote:
>>> (...)
>>>
>>>>>> Talking about readability, I find a goto with a clear label name much
>>>>>> more readable than a flag. So, NACK this patch, please keep the goto.
>>>>>  
>>>>> So you're against the refactoring, or only against using the flag?
>>>>> Keeping the goto leads to something like
>>>>>
>>>>> 	if (install_pcpu_timer(cpu, hrclock_freq, t))
>>>>> 		goto found
>>>>> (...)
>>>>> found:    ;
>>>>>
>>>>> since we need a statement for the label, but nothing is left to do.
>>>>> I find this fairly ugly, but if you prefer it over a flag, then
>>>>> so be it.
>>>>
>>>> Then use return instead of goto...
>>>
>>> Won't work -- that skips the rest of the enclosing per_cpu loop and
>>> the second part of the function introduced in the follow-up commit
>>> that does the actual bugfixing.
>>>
>>> Since I take the flag is the issue and not the refactoring as such, 
>>> please find an updated patch with a goto below.
>>
>>
>> Oh boy, you love functions, do you? What I would do is: keep the test
> sure() :)
>> for evtdev->mode outside of the install_pcpu_timer function so that we
>> clearly see in the loop that it is the only reason for continuing the
>> loop. Then use the goto found, and at the found label, call the now void
>> function install_pcpu_timer. Ok for you?
>>
> okay, changed the code as desired to introduce more labels and use the 
> preprocessor more often. Please pick 23b2de46314 and b738a3b624 from 
> https://github.com/siemens/ipipe core-3.5_for-upstream (apply also
> cleanly to core-4).


Ok. Applied to my for-core-4 branch, thanks.

-- 
                                                                Gilles.