Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753202AbbG1UkW (ORCPT ); Tue, 28 Jul 2015 16:40:22 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:53533 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752300AbbG1UkU (ORCPT ); Tue, 28 Jul 2015 16:40:20 -0400 Date: Tue, 28 Jul 2015 13:40:19 -0700 From: Andrew Morton To: Ashutosh Dixit Cc: Greg Kroah-Hartman , Joerg Roedel , iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, "David S. Miller" , Robin Murphy , Sudeep Dutt , Nikhil Rao , Anil S Keshavamurthy , Harish Chegondi Subject: Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library Message-Id: <20150728134019.7f00118543a04a53855d7ba5@linux-foundation.org> In-Reply-To: <8131ebc8ecb5ef13ef0aa4c49dabe9694f0e410f.1438040669.git.ashutosh.dixit@intel.com> References: <8131ebc8ecb5ef13ef0aa4c49dabe9694f0e410f.1438040669.git.ashutosh.dixit@intel.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 18251 Lines: 665 On Mon, 27 Jul 2015 16:57:32 -0700 Ashutosh Dixit wrote: > From: Harish Chegondi > > This patch converts iova.c into a library, moving it from > drivers/iommu/ to lib/, and exports its virtual address allocation and > management functions so that other modules can reuse them. >From the following emails it is unclear to me whether we'll actually be going ahead with this, but whatever. It's a chance to do some code reading. > {drivers/iommu => lib}/iova.c | 9 +++++++++ Except the code isn't here. Stupid git. Here 'tis: > /* > * Copyright __ 2006-2009, Intel Corporation. Non-ascii thing which I can't get to display correctly in anything. Give it up, it'll never work, use "(c)". > * This program is free software; you can redistribute it and/or modify it > * under the terms and conditions of the GNU General Public License, > * version 2, as published by the Free Software Foundation. > * > * This program is distributed in the hope it will be useful, but WITHOUT > * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > * more details. > * > * You should have received a copy of the GNU General Public License along with > * this program; if not, write to the Free Software Foundation, Inc., 59 Temple > * Place - Suite 330, Boston, MA 02111-1307 USA. > * > * Author: Anil S Keshavamurthy > */ > > #include > #include Wow. This file only compiles by sheer dumb luck. kernel.h? spinlock.h? bitops.h? Some of these things come in thanks to iova.h, others don't. > static struct kmem_cache *iommu_iova_cache; > > int iommu_iova_cache_init(void) Should actually be called iommu_iova_cache_create(). Because it's a wrapper around kmem_cache_create(), and "create" is what it does. > { > int ret = 0; > > iommu_iova_cache = kmem_cache_create("iommu_iova", > sizeof(struct iova), > 0, > SLAB_HWCACHE_ALIGN, > NULL); Could use KMEM_CACHE(). > if (!iommu_iova_cache) { > pr_err("Couldn't create iova cache\n"); > ret = -ENOMEM; > } > > return ret; > } > > void iommu_iova_cache_destroy(void) The naming throughout this driver is a mess. Sometimes it's iommu_iova_. Other times it's _iova. Other times it's _iova_. There's no thought and no consistency. The usual standard will work well here. Stick with iova_ and use it consistently. So iova_cache_create iova_cache_destroy iova_mem_free iova_init_domain iova_alloc iova_find iova_etcetera > { > kmem_cache_destroy(iommu_iova_cache); > } > > struct iova *alloc_iova_mem(void) > { > return kmem_cache_alloc(iommu_iova_cache, GFP_ATOMIC); > } GFP_ATOMIC is unreliable and undesirably depletes page allocator reserves. If it is really really the case that no caller can ever allocate an iova in any other context then OK, but that's an extraordinary thing and I suggest it should be well documented in code comments. However I suspect the thing to do here is to convert this to take a gfp_t argument to permit callers to use the stronger memory allocation strategies. > void free_iova_mem(struct iova *iova) > { > kmem_cache_free(iommu_iova_cache, iova); > } > > void > init_iova_domain(struct iova_domain *iovad, unsigned long granule, > unsigned long start_pfn, unsigned long pfn_32bit) > { > /* > * IOVA granularity will normally be equal to the smallest > * supported IOMMU page size; both *must* be capable of > * representing individual CPU pages exactly. > */ > BUG_ON((granule > PAGE_SIZE) || !is_power_of_2(granule)); > > spin_lock_init(&iovad->iova_rbtree_lock); > iovad->rbroot = RB_ROOT; > iovad->cached32_node = NULL; > iovad->granule = granule; > iovad->start_pfn = start_pfn; > iovad->dma_32bit_pfn = pfn_32bit; > } > > static struct rb_node * > __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn) > { > if ((*limit_pfn != iovad->dma_32bit_pfn) || > (iovad->cached32_node == NULL)) > return rb_last(&iovad->rbroot); > else { > struct rb_node *prev_node = rb_prev(iovad->cached32_node); > struct iova *curr_iova = > container_of(iovad->cached32_node, struct iova, node); > *limit_pfn = curr_iova->pfn_lo - 1; > return prev_node; > } > } > > static void > __cached_rbnode_insert_update(struct iova_domain *iovad, > unsigned long limit_pfn, struct iova *new) > { > if (limit_pfn != iovad->dma_32bit_pfn) > return; > iovad->cached32_node = &new->node; > } > > static void > __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free) > { > struct iova *cached_iova; > struct rb_node *curr; > > if (!iovad->cached32_node) > return; > curr = iovad->cached32_node; > cached_iova = container_of(curr, struct iova, node); > > if (free->pfn_lo >= cached_iova->pfn_lo) { > struct rb_node *node = rb_next(&free->node); > struct iova *iova = container_of(node, struct iova, node); > > /* only cache if it's below 32bit pfn */ > if (node && iova->pfn_lo < iovad->dma_32bit_pfn) > iovad->cached32_node = node; > else > iovad->cached32_node = NULL; > } > } > > /* Computes the padding size required, to make the > * the start address naturally aligned on its size > */ Comment has unusual layout, doesn't use all the display and has "the the". Like this: /* * Computes the padding size required, to make the start address naturally * aligned on its size. */ > static int > iova_get_pad_size(int size, unsigned int limit_pfn) iova_. Yay. > { > unsigned int pad_size = 0; > unsigned int order = ilog2(size); > > if (order) > pad_size = (limit_pfn + 1) % (1 << order); > > return pad_size; > } > > static int __alloc_and_insert_iova_range(struct iova_domain *iovad, > unsigned long size, unsigned long limit_pfn, > struct iova *new, bool size_aligned) > { > struct rb_node *prev, *curr = NULL; > unsigned long flags; > unsigned long saved_pfn; > unsigned int pad_size = 0; > > /* Walk the tree backwards */ > spin_lock_irqsave(&iovad->iova_rbtree_lock, flags); > saved_pfn = limit_pfn; > curr = __get_cached_rbnode(iovad, &limit_pfn); > prev = curr; > while (curr) { > struct iova *curr_iova = container_of(curr, struct iova, node); > > if (limit_pfn < curr_iova->pfn_lo) > goto move_left; > else if (limit_pfn < curr_iova->pfn_hi) > goto adjust_limit_pfn; > else { > if (size_aligned) > pad_size = iova_get_pad_size(size, limit_pfn); > if ((curr_iova->pfn_hi + size + pad_size) <= limit_pfn) > break; /* found a free slot */ > } > adjust_limit_pfn: > limit_pfn = curr_iova->pfn_lo - 1; > move_left: > prev = curr; > curr = rb_prev(curr); > } > > if (!curr) { > if (size_aligned) > pad_size = iova_get_pad_size(size, limit_pfn); > if ((iovad->start_pfn + size + pad_size) > limit_pfn) { > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > return -ENOMEM; > } > } > > /* pfn_lo will point to size aligned address if size_aligned is set */ > new->pfn_lo = limit_pfn - (size + pad_size) + 1; > new->pfn_hi = new->pfn_lo + size - 1; > > /* Insert the new_iova into domain rbtree by holding writer lock */ > /* Add new node and rebalance tree. */ > { > struct rb_node **entry, *parent = NULL; > > /* If we have 'prev', it's a valid place to start the > insertion. Otherwise, start from the root. */ > if (prev) > entry = &prev; > else > entry = &iovad->rbroot.rb_node; > > /* Figure out where to put new node */ > while (*entry) { > struct iova *this = container_of(*entry, > struct iova, node); Could do struct iova *this; this = container_of(*entry, struct iova, node); and avoid 80-col weirdnesses. This happens a lot. > parent = *entry; > > if (new->pfn_lo < this->pfn_lo) > entry = &((*entry)->rb_left); > else if (new->pfn_lo > this->pfn_lo) > entry = &((*entry)->rb_right); > else > BUG(); /* this should not happen */ > } > > /* Add new node and rebalance tree. */ > rb_link_node(&new->node, parent, entry); > rb_insert_color(&new->node, &iovad->rbroot); > } > __cached_rbnode_insert_update(iovad, saved_pfn, new); > > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > > > return 0; > } > > static void > iova_insert_rbtree(struct rb_root *root, struct iova *iova) > { > struct rb_node **new = &(root->rb_node), *parent = NULL; > /* Figure out where to put new node */ > while (*new) { > struct iova *this = container_of(*new, struct iova, node); > > parent = *new; > > if (iova->pfn_lo < this->pfn_lo) > new = &((*new)->rb_left); > else if (iova->pfn_lo > this->pfn_lo) > new = &((*new)->rb_right); > else > BUG(); /* this should not happen */ > } > /* Add new node and rebalance tree. */ > rb_link_node(&iova->node, parent, new); > rb_insert_color(&iova->node, root); > } > > /** > * alloc_iova - allocates an iova > * @iovad: - iova domain in question > * @size: - size of page frames to allocate > * @limit_pfn: - max limit address > * @size_aligned: - set if size_aligned address range is required The "-" on the function arguments isn't correct kerneldoc, I think. > * This function allocates an iova in the range iovad->start_pfn to limit_pfn, > * searching top-down from limit_pfn to iovad->start_pfn. If the size_aligned > * flag is set then the allocated address iova->pfn_lo will be naturally > * aligned on roundup_power_of_two(size). > */ > struct iova * > alloc_iova(struct iova_domain *iovad, unsigned long size, > unsigned long limit_pfn, > bool size_aligned) Layout of the arguments is strange and straggly. > { > struct iova *new_iova; > int ret; > > new_iova = alloc_iova_mem(); > if (!new_iova) > return NULL; > > /* If size aligned is set then round the size to > * to next power of two. > */ That's a pretty useless comment! > if (size_aligned) > size = __roundup_pow_of_two(size); > > ret = __alloc_and_insert_iova_range(iovad, size, limit_pfn, > new_iova, size_aligned); > > if (ret) { > free_iova_mem(new_iova); > return NULL; > } > > return new_iova; > } > > /** > * find_iova - find's an iova for a given pfn > * @iovad: - iova domain in question. > * @pfn: - page frame number > * This function finds and returns an iova belonging to the > * given doamin which matches the given pfn. > */ > struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn) > { > unsigned long flags; > struct rb_node *node; > > /* Take the lock so that no other thread is manipulating the rbtree */ > spin_lock_irqsave(&iovad->iova_rbtree_lock, flags); > node = iovad->rbroot.rb_node; > while (node) { > struct iova *iova = container_of(node, struct iova, node); > > /* If pfn falls within iova's range, return iova */ > if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) { > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > /* We are not holding the lock while this iova > * is referenced by the caller as the same thread > * which called this function also calls __free_iova() > * and it is by design that only one thread can possibly > * reference a particular iova and hence no conflict. > */ Well that sounds fishy. I'm generally assuming that this code isn't going to be worked on by the lay masses - much of the design resides within a few people's heads and that is where it will stay. Otherwise a whole lotta documetation will need to be added. eg, what is this "design" of which you speak. > return iova; > } > > if (pfn < iova->pfn_lo) > node = node->rb_left; > else if (pfn > iova->pfn_lo) > node = node->rb_right; > } > > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > return NULL; > } > > /** > * __free_iova - frees the given iova > * @iovad: iova domain in question. > * @iova: iova in question. > * Frees the given iova belonging to the giving domain > */ > void > __free_iova(struct iova_domain *iovad, struct iova *iova) > { > unsigned long flags; > > spin_lock_irqsave(&iovad->iova_rbtree_lock, flags); > __cached_rbnode_delete_update(iovad, iova); > rb_erase(&iova->node, &iovad->rbroot); > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > free_iova_mem(iova); > } > > /** > * free_iova - finds and frees the iova for a given pfn > * @iovad: - iova domain in question. > * @pfn: - pfn that is allocated previously > * This functions finds an iova for a given pfn and then > * frees the iova from that domain. > */ > void > free_iova(struct iova_domain *iovad, unsigned long pfn) > { > struct iova *iova = find_iova(iovad, pfn); > > if (iova) > __free_iova(iovad, iova); > > } > > /** > * put_iova_domain - destroys the iova doamin > * @iovad: - iova domain in question. > * All the iova's in that domain are destroyed. > */ > void put_iova_domain(struct iova_domain *iovad) This is not a "put" function. In Linux, "put" means "decrement the refcount and free if it fell to zero". A better name would be iova_domain_free(). > { > struct rb_node *node; > unsigned long flags; > > spin_lock_irqsave(&iovad->iova_rbtree_lock, flags); > node = rb_first(&iovad->rbroot); > while (node) { > struct iova *iova = container_of(node, struct iova, node); > > rb_erase(node, &iovad->rbroot); > free_iova_mem(iova); > node = rb_first(&iovad->rbroot); > } > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > } > > static int > __is_range_overlap(struct rb_node *node, > unsigned long pfn_lo, unsigned long pfn_hi) > { > struct iova *iova = container_of(node, struct iova, node); > > if ((pfn_lo <= iova->pfn_hi) && (pfn_hi >= iova->pfn_lo)) > return 1; > return 0; return (pfn_lo <= iova->pfn_hi) && (pfn_hi >= iova->pfn_lo); > } > > static inline struct iova * > alloc_and_init_iova(unsigned long pfn_lo, unsigned long pfn_hi) > { > struct iova *iova; > > iova = alloc_iova_mem(); > if (iova) { > iova->pfn_lo = pfn_lo; > iova->pfn_hi = pfn_hi; > } > > return iova; > } > > static struct iova * > __insert_new_range(struct iova_domain *iovad, > unsigned long pfn_lo, unsigned long pfn_hi) > { > struct iova *iova; > > iova = alloc_and_init_iova(pfn_lo, pfn_hi); > if (iova) > iova_insert_rbtree(&iovad->rbroot, iova); > > return iova; > } > > static void > __adjust_overlap_range(struct iova *iova, > unsigned long *pfn_lo, unsigned long *pfn_hi) > { > if (*pfn_lo < iova->pfn_lo) > iova->pfn_lo = *pfn_lo; > if (*pfn_hi > iova->pfn_hi) > *pfn_lo = iova->pfn_hi + 1; > } > > /** > * reserve_iova - reserves an iova in the given range > * @iovad: - iova domain pointer > * @pfn_lo: - lower page frame address > * @pfn_hi:- higher pfn adderss More kerneldoc funnies. s/adderss/address/ > * This function allocates reserves the address range from pfn_lo to pfn_hi so Sentence needs an edit > * that this address is not dished out as part of alloc_iova. > */ > struct iova * > reserve_iova(struct iova_domain *iovad, > unsigned long pfn_lo, unsigned long pfn_hi) > { > struct rb_node *node; > unsigned long flags; > struct iova *iova; > unsigned int overlap = 0; > > spin_lock_irqsave(&iovad->iova_rbtree_lock, flags); > for (node = rb_first(&iovad->rbroot); node; node = rb_next(node)) { > if (__is_range_overlap(node, pfn_lo, pfn_hi)) { > iova = container_of(node, struct iova, node); > __adjust_overlap_range(iova, &pfn_lo, &pfn_hi); > if ((pfn_lo >= iova->pfn_lo) && > (pfn_hi <= iova->pfn_hi)) > goto finish; > overlap = 1; > > } else if (overlap) > break; indenting > } > > /* We are here either because this is the first reserver node > * or need to insert remaining non overlap addr range > */ > iova = __insert_new_range(iovad, pfn_lo, pfn_hi); > finish: > > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > return iova; > } > > /** > * copy_reserved_iova - copies the reserved between domains > * @from: - source doamin from where to copy > * @to: - destination domin where to copy > * This function copies reserved iova's from one doamin to > * other. > */ > void > copy_reserved_iova(struct iova_domain *from, struct iova_domain *to) > { > unsigned long flags; > struct rb_node *node; > > spin_lock_irqsave(&from->iova_rbtree_lock, flags); > for (node = rb_first(&from->rbroot); node; node = rb_next(node)) { > struct iova *iova = container_of(node, struct iova, node); > struct iova *new_iova; > > new_iova = reserve_iova(to, iova->pfn_lo, iova->pfn_hi); > if (!new_iova) > printk(KERN_ERR "Reserve iova range %lx@%lx failed\n", > iova->pfn_lo, iova->pfn_lo); > } > spin_unlock_irqrestore(&from->iova_rbtree_lock, flags); > } > > struct iova * > split_and_remove_iova(struct iova_domain *iovad, struct iova *iova, > unsigned long pfn_lo, unsigned long pfn_hi) It would be nice to complete the kerneldoc, at least on the exported interface. > { > unsigned long flags; > struct iova *prev = NULL, *next = NULL; > > spin_lock_irqsave(&iovad->iova_rbtree_lock, flags); > if (iova->pfn_lo < pfn_lo) { > prev = alloc_and_init_iova(iova->pfn_lo, pfn_lo - 1); > if (prev == NULL) > goto error; > } > if (iova->pfn_hi > pfn_hi) { > next = alloc_and_init_iova(pfn_hi + 1, iova->pfn_hi); > if (next == NULL) > goto error; > } > > __cached_rbnode_delete_update(iovad, iova); > rb_erase(&iova->node, &iovad->rbroot); > > if (prev) { > iova_insert_rbtree(&iovad->rbroot, prev); > iova->pfn_lo = pfn_lo; > } > if (next) { > iova_insert_rbtree(&iovad->rbroot, next); > iova->pfn_hi = pfn_hi; > } > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > > return iova; > > error: > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > if (prev) > free_iova_mem(prev); > return NULL; > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/