Re: [PATCH 05/10] x86: use vector_desc instead of vector_irq

[ebiederm@xmission.com (Eric W. Biederman)]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Sun, 21 Mar 2010, Yinghai Lu wrote:
>
>> Eric pointed out that radix tree version of irq_to_desc will magnify delay on
>> the path of handle_irq.
>> 
>> use vector_desc to reduce the calling of irq_to_desc.
>> 
>> next step: need to change all ack, mask, umask, eoi for all irq_chip to take irq_desc
>
> That's not relevant for this change.
>  
>>  
>> -typedef int vector_irq_t[NR_VECTORS];
>> -DECLARE_PER_CPU(vector_irq_t, vector_irq);
>> -extern void setup_vector_irq(int cpu);
>> +typedef struct irq_desc *vector_desc_t[NR_VECTORS];
>
>   Why do we need that typedef ? Please use plain struct irq_desc *

Well at least originally DECLARE_PER_CPU chocked when given a complex
type. Does:
DECLARE_PER_CPU(struct irq_desc *[NR_VECTORS], vector_desc);
work?


>> +DECLARE_PER_CPU(vector_desc_t, vector_desc);
>> +extern void setup_vector_desc(int cpu);
> ...
>>  void destroy_irq(unsigned int irq)
>>  {
>>  	unsigned long flags;
>> +	struct irq_desc *desc;
>> +	struct irq_cfg *cfg;
>>  
>>  	dynamic_irq_cleanup_keep_chip_data(irq);
>>  
>>  	free_irte(irq);
>>  	raw_spin_lock_irqsave(&vector_lock, flags);
>> -	__clear_irq_vector(irq, get_irq_chip_data(irq));
>> +	desc = irq_to_desc(irq);
>> +	cfg = desc->chip_data;
>> +	__clear_irq_vector(desc, cfg);
>
> 	__clear_irq_vector(desc, desc->chip_data);
>
>  should be sufficient, right ?

You want to deliberately loose a modicum of type safety?


>
>>  	raw_spin_unlock_irqrestore(&vector_lock, flags);
>>  }
>>  
>> @@ -3377,6 +3376,7 @@ void destroy_irq(unsigned int irq)
>>  static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
>>  			   struct msi_msg *msg, u8 hpet_id)
>>  {
>> +	struct irq_desc *desc;
>>  	struct irq_cfg *cfg;
>>  	int err;
>>  	unsigned dest;
>> @@ -3384,8 +3384,9 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
>>  	if (disable_apic)
>>  		return -ENXIO;
>>  
>> -	cfg = irq_cfg(irq);
>> -	err = assign_irq_vector(irq, cfg, apic->target_cpus());
>> +	desc = irq_to_desc(irq);
>> +	cfg = desc->chip_data;
>> +	err = assign_irq_vector(desc, cfg, apic->target_cpus());
>
>   Ditto
>
>>  	if (err)
>>  		return err;
>>  
>> @@ -3876,14 +3877,16 @@ static struct irq_chip ht_irq_chip = {
>>  
>>  int arch_setup_ht_irq(unsigned int irq, struct pci_dev *dev)
>>  {
>> +	struct irq_desc *desc;
>>  	struct irq_cfg *cfg;
>>  	int err;
>>  
>>  	if (disable_apic)
>>  		return -ENXIO;
>>  
>> -	cfg = irq_cfg(irq);
>> -	err = assign_irq_vector(irq, cfg, apic->target_cpus());
>> +	desc = irq_to_desc(irq);
>> +	cfg = desc->chip_data;
>> +	err = assign_irq_vector(desc, cfg, apic->target_cpus());
>
>   Ditto
>
>>  	if (!err) {
>>  		struct ht_irq_msg msg;
>>  		unsigned dest;
>> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
>> index 91fd0c7..f71625c 100644
>> --- a/arch/x86/kernel/irq.c
>> +++ b/arch/x86/kernel/irq.c
>> @@ -229,19 +229,19 @@ unsigned int __irq_entry do_IRQ(struct pt_regs *regs)
>>  
>>  	/* high bit used in ret_from_ code  */
>>  	unsigned vector = ~regs->orig_ax;
>> -	unsigned irq;
>> +	struct irq_desc *desc;
>>  
>>  	exit_idle();
>>  	irq_enter();
>>  
>> -	irq = __get_cpu_var(vector_irq)[vector];
>> +	desc = __get_cpu_var(vector_desc)[vector];
>>  
>> -	if (!handle_irq(irq, regs)) {
>> +	if (!handle_irq(desc, regs)) {
>>  		ack_APIC_irq();
>>  
>>  		if (printk_ratelimit())
>> -			pr_emerg("%s: %d.%d No irq handler for vector (irq %d)\n",
>> -				__func__, smp_processor_id(), vector, irq);
>> +			pr_emerg("%s: %d.%d No irq handler for vector\n",
>
>   That printk is confusing. It's not lacking an irq handler. The
>   vector is simply not assigned.

Long evolution.  Do you have a suggestion of better wording?

Eric