From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751239AbdBWMAA (ORCPT ); Thu, 23 Feb 2017 07:00:00 -0500 Received: from foss.arm.com ([217.140.101.70]:54180 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751049AbdBWL77 (ORCPT ); Thu, 23 Feb 2017 06:59:59 -0500 Subject: Re: [PATCH] iommu: iova: Consolidate code for adding new node to iovad domain rbtree To: Marek Szyprowski , iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org References: <1487837853-3195-1-git-send-email-m.szyprowski@samsung.com> Cc: Bartlomiej Zolnierkiewicz From: Robin Murphy Message-ID: <54ade5ee-a7e7-e048-8eed-7ebace656ea0@arm.com> Date: Thu, 23 Feb 2017 11:59:11 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1487837853-3195-1-git-send-email-m.szyprowski@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23/02/17 08:17, Marek Szyprowski wrote: > This patch consolidates almost the same code used in iova_insert_rbtree() > and __alloc_and_insert_iova_range() functions. There is no functional change. > > Signed-off-by: Marek Szyprowski > --- > drivers/iommu/iova.c | 85 +++++++++++++++++++--------------------------------- > 1 file changed, 31 insertions(+), 54 deletions(-) > > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c > index b7268a14184f..32b9c2fb37b6 100644 > --- a/drivers/iommu/iova.c > +++ b/drivers/iommu/iova.c > @@ -100,6 +100,32 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad, > } > } > > +/* Insert the iova into domain rbtree by holding writer lock */ > +static void > +iova_insert_rbtree(struct rb_root *root, struct iova *iova, > + struct rb_node *start) > +{ > + struct rb_node **new, *parent = NULL; > + > + new = (start) ? &start : &(root->rb_node); > + /* Figure out where to put new node */ > + while (*new) { > + struct iova *this = rb_entry(*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 */ Ooh, if we're touching this, can we downgrade it to a WARN()? Granted, allocating an IOVA region of size 0 is not a reasonable thing to do intentionally, but the fact that it's guaranteed to take down the kernel is perhaps a bit much (I hit it soooo many times back when debugging the iommu_dma_map_sg() stuff). Nice tidyup otherwise, though. Robin. > + } > + /* Add new node and rebalance tree. */ > + rb_link_node(&iova->node, parent, new); > + rb_insert_color(&iova->node, root); > +} > + > /* > * Computes the padding size required, to make the start address > * naturally aligned on the power-of-two order of its size > @@ -157,35 +183,8 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad, > 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 = rb_entry(*entry, struct iova, node); > - 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); > - } > + /* If we have 'prev', it's a valid place to start the insertion. */ > + iova_insert_rbtree(&iovad->rbroot, new, prev); > __cached_rbnode_insert_update(iovad, saved_pfn, new); > > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); > @@ -194,28 +193,6 @@ static int __alloc_and_insert_iova_range(struct iova_domain *iovad, > 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 = rb_entry(*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); > -} > - > static struct kmem_cache *iova_cache; > static unsigned int iova_cache_users; > static DEFINE_MUTEX(iova_cache_mutex); > @@ -505,7 +482,7 @@ void put_iova_domain(struct iova_domain *iovad) > > iova = alloc_and_init_iova(pfn_lo, pfn_hi); > if (iova) > - iova_insert_rbtree(&iovad->rbroot, iova); > + iova_insert_rbtree(&iovad->rbroot, iova, NULL); > > return iova; > } > @@ -612,11 +589,11 @@ struct iova * > rb_erase(&iova->node, &iovad->rbroot); > > if (prev) { > - iova_insert_rbtree(&iovad->rbroot, prev); > + iova_insert_rbtree(&iovad->rbroot, prev, NULL); > iova->pfn_lo = pfn_lo; > } > if (next) { > - iova_insert_rbtree(&iovad->rbroot, next); > + iova_insert_rbtree(&iovad->rbroot, next, NULL); > iova->pfn_hi = pfn_hi; > } > spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags); >