From patchwork Wed Jul 6 08:53:16 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ram Pai X-Patchwork-Id: 949002 X-Patchwork-Delegate: bhelgaas@google.com Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p668rUaX021829 for ; Wed, 6 Jul 2011 08:53:30 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753954Ab1GFIx3 (ORCPT ); Wed, 6 Jul 2011 04:53:29 -0400 Received: from e3.ny.us.ibm.com ([32.97.182.143]:49135 "EHLO e3.ny.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752990Ab1GFIx1 (ORCPT ); Wed, 6 Jul 2011 04:53:27 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e3.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p668UF7U029851; Wed, 6 Jul 2011 04:30:15 -0400 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p668rMNI153210; Wed, 6 Jul 2011 04:53:22 -0400 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p662rKCu003117; Tue, 5 Jul 2011 20:53:21 -0600 Received: from us.ibm.com (sig-9-48-53-246.mts.ibm.com [9.48.53.246]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with SMTP id p662rHKm003066; Tue, 5 Jul 2011 20:53:17 -0600 Received: by us.ibm.com (sSMTP sendmail emulation); Wed, 06 Jul 2011 01:53:16 -0700 Date: Wed, 6 Jul 2011 01:53:16 -0700 From: Ram Pai To: Linus Torvalds Cc: Ram Pai , jbarnes@virtuousgeek.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, yinghai@kernel.org, bhutchings@solarflare.com, socketcan@hartkopp.net, bhelgaas@google.com, linux@dominikbrodowski.net Subject: Re: [PATCH 0/5 v2] PCI: fix cardbus and sriov regressions Message-ID: <20110706085316.GA3543@ram-ThinkPad-T61> Reply-To: Ram Pai References: <1309477662-18680-1-git-send-email-linuxram@us.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Wed, 06 Jul 2011 08:53:30 +0000 (UTC) On Sun, Jul 03, 2011 at 02:30:00PM -0700, Linus Torvalds wrote: > Gaah. I'm still rather uncomfortable about this, and I wonder about > patch 2 in particular. It seems that that patch could/should be split > up: the whole change to "find_resource()" etc looks like prime > material for a separate patch that splits up that function and > explains why that change is done. > > Also, quite frankly, by the time you pass in eight different arguments > (and pretty complex ones at that, with one being the alignment > function pointer), I start thinking that you should have passed in a > pointer to a descriptor structure instead. I get the feeling that the > "resource requirements" really should be a structure instead of lots > of individual arguments: > > IOW, this part: > > + resource_size_t newsize, resource_size_t min, > + resource_size_t max, resource_size_t align, > + resource_size_t (*alignf)(void *, > + const struct resource *, > + resource_size_t, > + resource_size_t), > + void *alignf_data) > > really makes me go > > struct resource_requirement { > resource_size_t min, max, align; > resource_size_t (*alignf)(const struct resource *, struct > resource_requirement *); > void *alignf_data); > }; > > and I'd really change the function argument to take that kind of > simplified thing instead. > > And that cleanup/re-organization would be prime material for a totally > independent patch that changes no semantics at all, just prepares for > the other changes. > > That way the final "patch 2" would be smaller and do the semantic > changes, instead of being a mix of semantic changes and infrastructure > changes. > > And some of the cleanup stuff I could merge for 3.0 just to make things easier. > > Hmm? Here is a cleaned up patch that just adds functionality to kernel/resource.c It does make a small semantic addition to allocate_resource(), where it reallocates the resource with a newer size if that resource was already allocated. Will this be acceptable for 3.0.0? From: Ram Pai Date: Tue, 5 Jul 2011 23:44:30 -0700 Subject: [PATCH 1/1] resource: ability to resize an allocated resource Provides the ability to resize a resource that is already allocated. This functionality is put in place to support reallocation needs of pci resources. Signed-off-by: Ram Pai --- kernel/resource.c | 118 ++++++++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 99 insertions(+), 19 deletions(-) diff --git a/kernel/resource.c b/kernel/resource.c index 798e2fa..ba727b6 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -38,6 +38,14 @@ struct resource iomem_resource = { }; EXPORT_SYMBOL(iomem_resource); +/* constraints to be met while allocating resources */ +struct resource_constraint { + resource_size_t min, max, align; + resource_size_t (*alignf)(void *, const struct resource *, + resource_size_t, resource_size_t); + void *alignf_data; +}; + static DEFINE_RWLOCK(resource_lock); static void *r_next(struct seq_file *m, void *v, loff_t *pos) @@ -384,16 +392,13 @@ static bool resource_contains(struct resource *res1, struct resource *res2) } /* - * Find empty slot in the resource tree given range and alignment. + * Find empty slot in the resource tree with the given range and + * alignment constraints */ -static int find_resource(struct resource *root, struct resource *new, - resource_size_t size, resource_size_t min, - resource_size_t max, resource_size_t align, - resource_size_t (*alignf)(void *, - const struct resource *, - resource_size_t, - resource_size_t), - void *alignf_data) +static int __find_resource(struct resource *root, struct resource *old, + struct resource *new, + resource_size_t size, + struct resource_constraint *constraint) { struct resource *this = root->child; struct resource tmp = *new, avail, alloc; @@ -404,25 +409,26 @@ static int find_resource(struct resource *root, struct resource *new, * Skip past an allocated resource that starts at 0, since the assignment * of this->start - 1 to tmp->end below would cause an underflow. */ - if (this && this->start == 0) { - tmp.start = this->end + 1; - this = this->sibling; + if (this && this->start == root->start) { + tmp.start = (this == old) ? old->start : this->end + 1; + this = this->sibling; } for(;;) { if (this) - tmp.end = this->start - 1; + tmp.end = (this == old) ? this->end : this->start - 1; else tmp.end = root->end; - resource_clip(&tmp, min, max); + resource_clip(&tmp, constraint->min, constraint->max); arch_remove_reservations(&tmp); /* Check for overflow after ALIGN() */ avail = *new; - avail.start = ALIGN(tmp.start, align); + avail.start = ALIGN(tmp.start, constraint->align); avail.end = tmp.end; if (avail.start >= tmp.start) { - alloc.start = alignf(alignf_data, &avail, size, align); + alloc.start = constraint->alignf(constraint->alignf_data, &avail, + size, constraint->align); alloc.end = alloc.start + size - 1; if (resource_contains(&avail, &alloc)) { new->start = alloc.start; @@ -432,14 +438,75 @@ static int find_resource(struct resource *root, struct resource *new, } if (!this) break; - tmp.start = this->end + 1; + if (this != old) + tmp.start = this->end + 1; this = this->sibling; } return -EBUSY; } +/* + * Find empty slot in the resource tree given range and alignment. + */ +static int find_resource(struct resource *root, struct resource *new, + resource_size_t size, + struct resource_constraint *constraint) +{ + return __find_resource(root, NULL, new, size, constraint); +} + +/** + * reallocate_resource - allocate a slot in the resource tree given range & alignment. + * The resource will be relocated if the new size cannot be reallocated in the + * current location. + * + * @root: root resource descriptor + * @old: resource descriptor desired by caller + * @newsize: new size of the resource descriptor + * @constraint: the size and alignment constraints to be met. + */ +int reallocate_resource(struct resource *root, struct resource *old, + resource_size_t newsize, + struct resource_constraint *constraint) +{ + int err=0; + struct resource new = *old; + struct resource *conflict; + + write_lock(&resource_lock); + + if ((err = __find_resource(root, old, &new, newsize, constraint))) + goto out; + + if (resource_contains(&new, old)) { + old->start = new.start; + old->end = new.end; + goto out; + } + + if (old->child) { + err = -EBUSY; + goto out; + } + + if (resource_contains(old, &new)) { + old->start = new.start; + old->end = new.end; + } else { + __release_resource(old); + *old = new; + conflict = __request_resource(root, old); + BUG_ON(conflict); + } +out: + write_unlock(&resource_lock); + return err; +} + + /** - * allocate_resource - allocate empty slot in the resource tree given range & alignment + * allocate_resource - allocate empty slot in the resource tree given range & alignment. + * The resource will be reallocated with a new size if it was already allocated * @root: root resource descriptor * @new: resource descriptor desired by caller * @size: requested resource region size @@ -459,12 +526,25 @@ int allocate_resource(struct resource *root, struct resource *new, void *alignf_data) { int err; + struct resource_constraint constraint; if (!alignf) alignf = simple_align_resource; + constraint.min = min; + constraint.max = max; + constraint.align = align; + constraint.alignf = alignf; + constraint.alignf_data = alignf_data; + + if ( new->parent ) { + /* resource is already allocated, try reallocating with + the new constraints */ + return reallocate_resource(root, new, size, &constraint); + } + write_lock(&resource_lock); - err = find_resource(root, new, size, min, max, align, alignf, alignf_data); + err = find_resource(root, new, size, &constraint); if (err >= 0 && __request_resource(root, new)) err = -EBUSY; write_unlock(&resource_lock);