On 09/18/2012 09:10 PM, Jan Kiszka wrote:
> On 2012-09-18 21:01, Gilles Chanteperdrix wrote:
>> On 09/18/2012 08:55 PM, Jan Kiszka wrote:
>>
>>> On 2012-09-18 20:31, Gilles Chanteperdrix wrote:
>>>> On 09/18/2012 08:25 PM, Jan Kiszka wrote:
>>>>
>>>>> On 2012-09-18 19:28, Gilles Chanteperdrix wrote:
>>>>>> On 09/18/2012 07:25 PM, Jan Kiszka wrote:
>>>>>>> On 2012-09-17 10:13, Gilles Chanteperdrix wrote:
>>>>>>>> On 09/16/2012 10:50 AM, Philippe Gerum wrote:
>>>>>>>>> On 09/16/2012 12:26 AM, Gilles Chanteperdrix wrote:
>>>>>>>>>> On 09/11/2012 05:56 PM, Gernot Hillier wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi there!
>>>>>>>>>>>
>>>>>>>>>>> While testing ipipe-core3.2 on an SMP x86 machine, I found a
>>>>>>>>>>> reproducible
>>>>>>>>>>> kernel BUG after some seconds after starting irqbalance:
>>>>>>>>>>>
>>>>>>>>>>> ------------[ cut here ]------------
>>>>>>>>>>> kernel BUG at arch/x86/kernel/ipipe.c:592!
>>>>>>>>>>> invalid opcode: 0000 [#1] SMP
>>>>>>>>>>> CPU 0
>>>>>>>>>>> Modules linked in: des_generic md4 i7core_edac psmouse nls_cp437
>>>>>>>>>>> edac_core cifs serio_raw joydev raid10 raid456 async_pq async_xor
>>>>>>>>>>> xor async_memcpy async_raid6_recov usbhid hid mpt2sas
>>>>>>>>>>> scsi_transport_sas raid_class igb raid6_pq async_tx raid1 raid0
>>>>>>>>>>> multipath linear
>>>>>>>>>>>
>>>>>>>>>>> Pid: 0, comm: swapper/0 Not tainted 3.2.21-9-xenomai #3 Siemens AG
>>>>>>>>>>> Healthcare Sector MARS 2.1/X8DTH
>>>>>>>>>>> RIP: 0010:[<ffffffff8101edec>] [<ffffffff8101edec>]
>>>>>>>>>>> __ipipe_handle_irq+0x1bc/0x1d0
>>>>>>>>>>> RSP: 0018:ffffffff8177bbe0 EFLAGS: 00010086
>>>>>>>>>>> RAX: 000000000000d880 RBX: 00000000ffffffff RCX: 0000000000000092
>>>>>>>>>>> RDX: ffffffffffffffdf RSI: ffffffff8177bc18 RDI: ffffffff8177bbf8
>>>>>>>>>>> RBP: ffffffff8177bc00 R08: 0000000000000001 R09: 0000000000000000
>>>>>>>>>>> R10: ffff880624ebaef8 R11: 000000000029fbc4 R12: 000000000000d880
>>>>>>>>>>> R13: ffffffff8177bbf8 R14: ffff880624e00000 R15: ffff880624e0d880
>>>>>>>>>>> FS: 0000000000000000(0000) GS:ffff880624e00000(0000)
>>>>>>>>>>> knlGS:0000000000000000
>>>>>>>>>>> CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
>>>>>>>>>>> CR2: 00007f452a2efb80 CR3: 0000000c114d3000 CR4: 00000000000006f0
>>>>>>>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>>>>>>>>>> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
>>>>>>>>>>> Process swapper/0 (pid: 0, threadinfo ffffffff81778000, task
>>>>>>>>>>> ffffffff81787020)
>>>>>>>>>>> Stack:
>>>>>>>>>>> 0000000000000000 ffffffff8177bfd8 0000000000000063 ffff880624e1f9a8
>>>>>>>>>>> ffffffff8177bca8 ffffffff815a44dd ffffffff8177bc18 ffffffff8177bca8
>>>>>>>>>>> 00000000815a373b 000000000029fbc4 ffff880624eba570 0000000000000000
>>>>>>>>>>> Call Trace:
>>>>>>>>>>> [<ffffffff815a44dd>] irq_move_cleanup_interrupt+0x5d/0x90
>>>>>>>>>>> [<ffffffff815a6649>] ? call_softirq+0x19/0x30
>>>>>>>>>>> [<ffffffff810043d5>] do_softirq+0xc5/0x100
>>>>>>>>>>> [<ffffffff81050955>] irq_exit+0xd5/0xf0
>>>>>>>>>>> [<ffffffff815a6e1f>] do_IRQ+0x6f/0x100
>>>>>>>>>>> [<ffffffff815a6db0>] ? __entry_text_end+0x5/0x5
>>>>>>>>>>> [<ffffffff8101f4d3>] __ipipe_do_IRQ+0x83/0xa0
>>>>>>>>>>> [<ffffffff8101f4d9>] ? __ipipe_do_IRQ+0x89/0xa0
>>>>>>>>>>> [<ffffffff810b7aaa>] __ipipe_dispatch_irq_fast+0x16a/0x170
>>>>>>>>>>> [<ffffffff810b87b9>] __ipipe_dispatch_irq+0xe9/0x210
>>>>>>>>>>> [<ffffffff8101eca1>] __ipipe_handle_irq+0x71/0x1d0
>>>>>>>>>>> [<ffffffff8159bae0>] common_interrupt+0x60/0x81
>>>>>>>>>>> [<ffffffff8101f3c4>] ? __ipipe_halt_root+0x34/0x50
>>>>>>>>>>> [<ffffffff8101f3b7>] ? __ipipe_halt_root+0x27/0x50
>>>>>>>>>>> [<ffffffff8100b146>] default_idle+0x66/0x1a0
>>>>>>>>>>> [<ffffffff810020cf>] cpu_idle+0xaf/0x100
>>>>>>>>>>> [<ffffffff8157ec22>] rest_init+0x72/0x80
>>>>>>>>>>> [<ffffffff81aeacca>] start_kernel+0x3b4/0x3bf
>>>>>>>>>>> [<ffffffff81aea321>] x86_64_start_reservations+0x131/0x135
>>>>>>>>>>> [<ffffffff81aea456>] x86_64_start_kernel+0x131/0x138
>>>>>>>>>>> Code: ff ff 0f 1f 44 00 00 48 83 a0 98 06 00 00 fe 4c 89 ee bf 20
>>>>>>>>>>> 00 00 00 e8 63 83 09 00 e9 f6 fe ff ff be 01 00 00 00 e9 ab fe ff
>>>>>>>>>>> ff <0f> 0b 66 90 eb fc 66 66 66 66 66 2e 0f 1f 84 00 00 00 00 00 55
>>>>>>>>>>> RIP [<ffffffff8101edec>] __ipipe_handle_irq+0x1bc/0x1d0
>>>>>>>>>>> RSP <ffffffff8177bbe0>
>>>>>>>>>>>
>>>>>>>>>>> This seems to be caused by a missing entry for
>>>>>>>>>>> IRQ_MOVE_CLEANUP_VECTOR
>>>>>>>>>>> in the per_cpu array vector_irq[].
>>>>>>>>>>>
>>>>>>>>>>> I found that ipipe_init_vector_irq() (which used to add the needed
>>>>>>>>>>> entry) was factored out from arch/x86/kernel/ipipe.c. This likely
>>>>>>>>>>> happened when porting from 2.6.38 to 3.1 - at least I can still see
>>>>>>>>>>> the
>>>>>>>>>>> code in ipipe-2.6.38-x86 and missed it in ipipe-core3.1 (and didn't
>>>>>>>>>>> find
>>>>>>>>>>> any x86-branch in-between).
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If I understand correctly, ipipe_init_vector_irq is no longer needed
>>>>>>>>>> because the I-pipe core uses ipipe_apic_vector_irq for vectors above
>>>>>>>>>> FIRST_SYSTEM_VECTOR. All system vectors are above this limit...
>>>>>>>>>> except
>>>>>>>>>> IRQ_MOVE_CLEANUP_VECTOR. So, your patch is correct. Another one which
>>>>>>>>>> should work is to handle the special case IRQ_MOVE_CLEANUP_IRQ in
>>>>>>>>>> __ipipe_handle_irq as well.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is correct, but unfortunately, upstream reshuffles the IRQ vector
>>>>>>>>> space every so often, and does not restrict the special vectors to the
>>>>>>>>> system range anymore. Therefore, we should re-introduce a post-setup
>>>>>>>>> routine for the vector->irq map, grouping all fixups we need in one
>>>>>>>>> place.
>>>>>>>>>
>>>>>>>> I propose the following change:
>>>>>>>> http://git.xenomai.org/?p=ipipe-gch.git;a=commitdiff;h=9d131dc33080cda3f7e40342210d9338dc0c3d02
>>>>>>>>
>>>>>>>> Which avoids listing explicitely the vectors we want to intercept, and
>>>>>>>> so, should allow some changes to happen in the kernel without having to
>>>>>>>> care too much (except for vectors sur as IRQ_MOVE_CLEANUP_VECTOR which
>>>>>>>> do not go through alloc_intr_gate, but this vector is the only
>>>>>>>> exception,
>>>>>>>> for now).
>>>>>>>
>>>>>>> Crashes on boot, SMP at least. Investigating.
>>>>>>
>>>>>> Well, I tested it in SMP, so, the crash is probably due to some option I
>>>>>> could not activate (such as IRQ_REMAP, the one triggering the use of
>>>>>> IRQ_MOVE_CLEANUP_VECTOR). One thing is sure however, some #ifdef is
>>>>>> missing, because it does not compile in UP mode without APIC/IO-APIC.
>>>>>
>>>>> Not sure what the precise option difference is, but the bug is that you
>>>>> write to vector_irq before __setup_vector_irq is finished.
>>>>
>>>>
>>>> Well, yes, the writes to vector_irq should happen even before it is
>>>> started, that is the point. The test at the end of setup_vector_irq skip
>>>> the... Ah, wait, a hunk is missing (I did test this modification in the
>>>> middle of the LAPIC TPR modification, so the commit is not actually
>>>> tested).
>>>>
>>>> diff --git a/arch/x86/kernel/apic/io_apic.c
>>>> b/arch/x86/kernel/apic/io_apic.c
>>>> index 93f84c9..91d1ee9 100644
>>>> --- a/arch/x86/kernel/apic/io_apic.c
>>>> +++ b/arch/x86/kernel/apic/io_apic.c
>>>> @@ -1290,9 +1290,15 @@ static void __clear_irq_vector(int irq, struct
>>>> irq_cfg *cfg)
>>>> void __setup_vector_irq(int cpu)
>>>> {
>>>> /* Initialize vector_irq on a new cpu */
>>>> - int irq, vector;
>>>> + int irq, vector, vectors_limit;
>>>> struct irq_cfg *cfg;
>>>>
>>>> +#ifndef CONFIG_IPIPE
>>>> + vectors_limit = NR_VECTORS;
>>>> +#else
>>>> + vectors_limit = first_system_vector;
>>>> +#endif /* CONFIG_IPIPE */
>>>> +
>>>> /*
>>>> * vector_lock will make sure that we don't run into irq vector
>>>> * assignments that might be happening on another cpu in parallel,
>>>> @@ -1317,7 +1323,10 @@ void __setup_vector_irq(int cpu)
>>>> per_cpu(vector_irq, cpu)[vector] = irq;
>>>> }
>>>> /* Mark the free vectors */
>>>> - for (vector = 0; vector < NR_VECTORS; ++vector) {
>>>> + for (vector = 0; vector < first_system_vector; ++vector) {
>>>> + if (vector == IRQ_MOVE_CLEANUP_VECTOR)
>>>> + continue;
>>>> +
>>>> irq = per_cpu(vector_irq, cpu)[vector];
>>>> if (irq < 0)
>>>> continue;
>>>>
>>>>
>>>
>>> Err, that's not really getting cleaner or easier maintainable than the
>>> original version, does it? Also, some loop below this clears out certain
>>> vector_irq entries again - are you sure that this is what we want? I
>>> vaguely recall that there was a very good reason to patch vector_irq
>>> afterward...
>>>
>>> Will post the old variant, at least for comparison.
>>
>>
>> The point is to avoid having to enumerate the vectors by hand. If a
>> system vector is added, no further modification is needed.
>
> I see your intention, but this comes at the price of changing other
> kernel code that works against us due to the initialization order.
>
> Also, we only have two ranges to address: MOVE_CLEANUP and everything >
> first_system_vector. Your patch also have to deal with both of them
> separately.
Yes, and on the other hand, the function you propose could use a loop
from system_vector to NR_VECTORS, so, basically not depend on a
systematic enumeration of the used system vectors. We simply have to
ensure that it is called after the calls to alloc_intr_gate.
--
Gilles.
_______________________________________________
Xenomai mailing list
Xenomai@xenomai.org
http://www.xenomai.org/mailman/listinfo/xenomai
