Re: [RFC] Correct behaviour of irq affinity?

[Yinghai Lu]

Rusty Russell wrote:
> On Tuesday 24 March 2009 17:51:43 Yinghai Lu wrote:
>> On Mon, Mar 23, 2009 at 10:49 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
>>> The effect of setting desc->affinity (ie. from userspace via sysfs) has varied
>>> over time.  In 2.6.27, the 32-bit code anded the value with cpu_online_map,
>>> and both 32 and 64-bit did that anding whenever a cpu was unplugged.
>>>
>>> 2.6.29 consolidated this into one routine (and fixed hotplug) but introduced
>>> another variation: anding the affinity with cfg->domain.  Is this right, or
>>> should we just set it to what the user said?  Or as now, indicate that we're
>>> restricting it.
>>>
>>> If we should change it, here's what the patch looks like against x86 tip
>>> (cpu_mask_to_apicid_and already takes cpu_online_mask into account):
>>>
>>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>>> index 86827d8..30906cd 100644
>>> --- a/arch/x86/kernel/apic/io_apic.c
>>> +++ b/arch/x86/kernel/apic/io_apic.c
>>> @@ -592,10 +592,10 @@ set_desc_affinity(struct irq_desc *desc, const struct cpumask *mask)
>>>        if (assign_irq_vector(irq, cfg, mask))
>>>                return BAD_APICID;
>>>
>>> -       cpumask_and(desc->affinity, cfg->domain, mask);
>>> +       cpumask_copy(desc->affinity, mask);
>>>        set_extra_move_desc(desc, mask);
>>>
>>> -       return apic->cpu_mask_to_apicid_and(desc->affinity, cpu_online_mask);
>>> +       return apic->cpu_mask_to_apicid_and(desc->affinity, cfg->domain);
>>>  }
>>>
>>>  static void
>>>
>> cfg->domain for logical flat: will be ALL_CPUS
>> for phys flat (aka bigsmp on 32bit) will be one cpu set mask.
>>
>> so desc->affinity: for logical will be not changed, but
>> set_desc_affinity() return will be changed. ( not add with
>> cpu_online_mask anymore)
> 
> No, internally cpu_mask_to_apicid_and() does and with cpu_online_mask
> already, eg in include/asm/bigsmp/apic.h:
> 
> static inline unsigned int cpu_mask_to_apicid_and(const struct cpumask *cpumask,
> 						  const struct cpumask *andmask)
> {
> 	int cpu;
> 
> 	/*
> 	 * We're using fixed IRQ delivery, can only return one phys APIC ID.
> 	 * May as well be the first.
> 	 */
> 	for_each_cpu_and(cpu, cpumask, andmask)
> 		if (cpumask_test_cpu(cpu, cpu_online_mask))
> 			break;
> 	if (cpu < nr_cpu_ids)
> 		return cpu_to_logical_apicid(cpu);
> 
> 	return BAD_APICID;
> }
> 
>> when mask is 0x0f
>> for phys flat, desc->affinity will be changed to 0x0f from
>> 0x01/0x02/0x04/08, return set_desc_affinity is not changed.
>> so /proc/irq/xx/smp_affinity will be changed. and it does reflect that
>> actually affinity.
>>
>> so this patch looks not right.
> 
> Only change should be that smp_affinity will reflect actual affinity, not
> affinity user set.

ok.
how about
static unsigned int flat_cpu_mask_to_apicid_and(const struct cpumask *cpumask,
                                                const struct cpumask *andmask)
{
        unsigned long mask1 = cpumask_bits(cpumask)[0] & APIC_ALL_CPUS;
        unsigned long mask2 = cpumask_bits(andmask)[0] & APIC_ALL_CPUS;

        return mask1 & mask2;
}

change it use
default_cpu_mask_to_apicid_and ?

YH
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/