Re: [Xen-devel] [PATCH] x86/apic/x2apic: Share IRQ vector between cluster members only when no cpumask is specified

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

>>>> @@ -72,9 +73,13 @@ static void __init clustered_apic_check_x2apic(void)
>>>> {
>>>> }
>>>  >
>>>> -static const cpumask_t *vector_allocation_cpumask_x2apic_cluster(int cpu)
>>>> +static const cpumask_t *vector_allocation_cpumask_x2apic_cluster(int cpu,
>>>> +    const cpumask_t *cpumask)
>>>> {
>>>> -    return per_cpu(cluster_cpus, cpu);
>>>> +    if ( cpumask != TARGET_CPUS )
>>> Is a pointer comparison (rather than a content one) here really correct in
>>> the general case?
>> When the caller is using TARGET_CPUS this is an indication that it
>> explicitly didn't care about sharing (in assign_irq_vector()). A caller
>> might want to have sharing on and provide a mask that has the same
>> content as TARGET_CPUS but is stored in a different location. This will
>> allow vector_allocation_cpumask_x2apic_cluster() to distinguish it from
>> a "don't care" case.
> Yes, I can see that intention. But you still wouldn't e.g. distinguish a 
> caller
> using TARGET_CPUS() from one passing &cpu_online_map.

True.

>
>>>> --- a/xen/include/asm-x86/mach-generic/mach_apic.h
>>>> +++ b/xen/include/asm-x86/mach-generic/mach_apic.h
>>>> @@ -13,10 +13,11 @@
>>>> #define INT_DELIVERY_MODE (genapic->int_delivery_mode)
>>>> #define INT_DEST_MODE (genapic->int_dest_mode)
>>>> #define TARGET_CPUS      (genapic->target_cpus())
>>>> -#define init_apic_ldr (genapic->init_apic_ldr)
>>>> -#define clustered_apic_check (genapic->clustered_apic_check) 
>>>> -#define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)
>>>> -#define vector_allocation_cpumask(cpu) 
>>>> (genapic->vector_allocation_cpumask(cpu))
>>>> +#define INIT_APIC_LDR (genapic->init_apic_ldr)
>>>> +#define CLUSTERED_APIC_CHECK (genapic->clustered_apic_check) 
>>>> +#define CPU_MASK_TO_APICID (genapic->cpu_mask_to_apicid)
>>>> +#define VECTOR_ALLOCATION_CPUMASK(cpu, mask) \
>>>> +    (genapic->vector_allocation_cpumask(cpu, mask))
>>>  
>>> I don't see the connection of the change in case of all of these to the 
>>> purpose
>>> of this patch.
>> I need to include asm-x86/mach-generic/mach_apic.h in x2apic.c and
> That's solely because of the use of TARGET_CPUS, isn't it? With the above
> comment in mind, this could equally well be &cpu_online_map I think, making
> explicit what is implicit right now.

Yes, but it it won't prevent a caller from explicitly passing
&cpu_online_map neither.

How about allowing assign_irq_vector() pass NULL mask to
__assign_irq_vector() (I think this is the only place where it matters)?
__assign_irq_vector() can then turn it into TARGET_CPUS and pass NULL to
vector_allocation_cpumask().

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel