linux-kernel - Re: [PATCH 04/12] irqdomain: Eliminate dedicated radix lookup functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 16 Jun 2012 15:56:23 +1000
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	linux-kernel@...r.kernel.org, Milton Miller <miltonm@....com>,
	Paul Mundt <lethal@...ux-sh.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Rob Herring <rob.herring@...xeda.com>
Subject: Re: [PATCH 04/12] irqdomain: Eliminate dedicated radix lookup
 functions

On Fri, 2012-06-15 at 23:01 -0600, Grant Likely wrote:
> In preparation to remove the slow revmap path, eliminate the public
> radix revmap lookup functions.  This simplifies the code and makes the
> slowpath removal patch a lot simpler.

The idea here was to avoid a test (performance in the fast path) since
the controller knows the type of revmap it uses. You could just remove
the fallback to the slow path if there's a mismatch and keep the WARN_ON
no ? No biggie, it's just a switch/case with fairly predictable outcome
I suppose ...

A few other nits:

> 	return irq;
> diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
> index cd1d18d..9049d9f 100644
> --- a/arch/powerpc/sysdev/xics/xics-common.c
> +++ b/arch/powerpc/sysdev/xics/xics-common.c
> @@ -329,9 +329,6 @@ static int xics_host_map(struct irq_domain *h, unsigned int virq,
>  
>  	pr_devel("xics: map virq %d, hwirq 0x%lx\n", virq, hw);
>  
> -	/* Insert the interrupt mapping into the radix tree for fast lookup */
> -	irq_radix_revmap_insert(xics_host, virq, hw);
> -
>  	/* They aren't all level sensitive but we just don't really know */
>  	irq_set_status_flags(virq, IRQ_LEVEL);

This looks like it belongs in a different patch, possibly "[02/12]
irqdomain: Always update revmap when setting up a virq" no ? 

> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 3425631..d8b88c5 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -169,10 +169,6 @@ static inline int irq_create_identity_mapping(struct irq_domain *host,
>  	return irq_create_strict_mappings(host, hwirq, hwirq, 1);
>  }
>  
> -extern void irq_radix_revmap_insert(struct irq_domain *host, unsigned int virq,
> -				    irq_hw_number_t hwirq);
> -extern unsigned int irq_radix_revmap_lookup(struct irq_domain *host,
> -					    irq_hw_number_t hwirq);
>  extern unsigned int irq_linear_revmap(struct irq_domain *host,
>  				      irq_hw_number_t hwirq);
>  
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 1a8f3d2..79a7711 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -415,7 +415,9 @@ int irq_domain_associate_many(struct irq_domain *domain, unsigned int irq_base,
>  				domain->revmap_data.linear.revmap[hwirq] = virq;
>  			break;
>  		case IRQ_DOMAIN_MAP_TREE:
> -			irq_radix_revmap_insert(domain, virq, hwirq);
> +			mutex_lock(&revmap_trees_mutex);
> +			radix_tree_insert(&domain->revmap_data.tree, hwirq, irq_data);
> +			mutex_unlock(&revmap_trees_mutex);
>  			break;
>  		}

That too looks like it belongs in another patch.

> @@ -688,64 +690,6 @@ unsigned int irq_find_mapping(struct irq_domain *domain,
>  EXPORT_SYMBOL_GPL(irq_find_mapping);
>  
>  /**
> - * irq_radix_revmap_lookup() - Find a linux irq from a hw irq number.
> - * @domain: domain owning this hardware interrupt
> - * @hwirq: hardware irq number in that domain space
> - *
> - * This is a fast path, for use by irq controller code that uses radix tree
> - * revmaps
> - */
> -unsigned int irq_radix_revmap_lookup(struct irq_domain *domain,
> -				     irq_hw_number_t hwirq)
> -{
> -	struct irq_data *irq_data;
> -
> -	if (WARN_ON_ONCE(domain->revmap_type != IRQ_DOMAIN_MAP_TREE))
> -		return irq_find_mapping(domain, hwirq);
> -
> -	/*
> -	 * Freeing an irq can delete nodes along the path to
> -	 * do the lookup via call_rcu.
> -	 */
> -	rcu_read_lock();
> -	irq_data = radix_tree_lookup(&domain->revmap_data.tree, hwirq);
> -	rcu_read_unlock();
> -
> -	/*
> -	 * If found in radix tree, then fine.
> -	 * Else fallback to linear lookup - this should not happen in practice
> -	 * as it means that we failed to insert the node in the radix tree.
> -	 */
> -	return irq_data ? irq_data->irq : irq_find_mapping(domain, hwirq);
> -}
> -EXPORT_SYMBOL_GPL(irq_radix_revmap_lookup);
> -
> -/**
> - * irq_radix_revmap_insert() - Insert a hw irq to linux irq number mapping.
> - * @domain: domain owning this hardware interrupt
> - * @virq: linux irq number
> - * @hwirq: hardware irq number in that domain space
> - *
> - * This is for use by irq controllers that use a radix tree reverse
> - * mapping for fast lookup.
> - */
> -void irq_radix_revmap_insert(struct irq_domain *domain, unsigned int virq,
> -			     irq_hw_number_t hwirq)
> -{
> -	struct irq_data *irq_data = irq_get_irq_data(virq);
> -
> -	if (WARN_ON(domain->revmap_type != IRQ_DOMAIN_MAP_TREE))
> -		return;
> -
> -	if (virq) {
> -		mutex_lock(&revmap_trees_mutex);
> -		radix_tree_insert(&domain->revmap_data.tree, hwirq, irq_data);
> -		mutex_unlock(&revmap_trees_mutex);
> -	}
> -}
> -EXPORT_SYMBOL_GPL(irq_radix_revmap_insert);
> -
> -/**
>   * irq_linear_revmap() - Find a linux irq from a hw irq number.
>   * @domain: domain owning this hardware interrupt
>   * @hwirq: hardware irq number in that domain space

Cheers,
Ben.


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