Re: [Xenomai] [PATCH] ipipe: x86: Populate vector_irq for all dispatched vectors

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-09-19 17:02, Gilles Chanteperdrix wrote:
> 
> Jan Kiszka wrote:
>> On 2012-09-19 16:46, Gilles Chanteperdrix wrote:
>>>
>>> Jan Kiszka wrote:
>>>> On 2012-09-19 16:28, Gilles Chanteperdrix wrote:
>>>>>
>>>>> Jan Kiszka wrote:
>>>>>>  	for (vector = 0; vector < NR_VECTORS; ++vector) {
>>>>>> +		/* I-pipe requires initialized vector_irq for system vectors */
>>>>>> +		if (test_bit(vector, used_vectors))
>>>>>> +			continue;
>>>>>>  		irq = per_cpu(vector_irq, cpu)[vector];
>>>>>>  		if (irq < 0)
>>>>>>  			continue;
>>>>>
>>>>> This looks wrong to me: you are skipping a part a code that was
>>>>> setting
>>>>> to
>>>>> -1 vectors that were allocated but not used on this cpu.
>>>>
>>>> I'm skipping only used_vectors, i.e system vectors. They will be set up
>>>> for I-pipe for that well-known reason. I'm no longer skipping to clear
>>>> IRQ vectors that are not used on the current CPU
>>>> (!cpumask_test_cpu(cpu,
>>>> cfg->domain)). That is what was missing in both approaches so far.
>>>>
>>>> Note: vector_free = (vector_irq == -1) && !used_vector
>>>
>>> Let us go back to the obvious loop:
>>>
>>>>         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;
>>>>
>>>>                 cfg = irq_cfg(irq);
>>>>                 if (!cpumask_test_cpu(cpu, cfg->domain))
>>>>                         per_cpu(vector_irq, cpu)[vector] = -1;
>>>>         }
>>>
>>> >From what I understand, this function is called when a CPU is started,
>>> so, for instance, with hotplug cpu, on a system that has been running
>>> for days and so had a lot of time to allocate a lot of vectors.
>>>
>>> Now, some of these vectors are known not to be needed on the cpu we
>>> are currently starting. What this loop does, is for such vectors, to
>>> set the vector_irq entry on the cpu we are currently starting to -1.
>>>
>>> Now, such vectors will certainly have the used_vectors bit to 1.
>>
>> used_vectors should be called "used_for_system_vector".
> 
> What about TLB flush vectors, are they not restricted to some cpus?

OK, let's try this step by step:

So far, the original loop ("Mark the free vectors" went over all
vectors, clearing those out in vector_irq that were filled for *external
IRQs* by the first loop ("Mark the inuse vector") but are actually not
in use on the current CPU. Linux does not fill any system vector into
vector_irq, as we know.

Now that we do enter certain system vectors here (MOVE_CLEANUP and those
above first_system_vector), we must avoid that this clearing happens for
them. My old version did this by overwriting what was cleared with that
simply 1:1 mapping again. Your version avoided any clearing for
MOVE_CLEANUP and >first_system_vector. Both were wrong as both left
vectors marked as in use which are actually free.

What we need is to still clear unused IRQ vectors, i.e. still run the
full loop, but skip used system vectors. And the latter can be achieved
by looking at used_vectors which is valid for all CPUs.

Is this clearer?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux