Re: [PATCH -v12 01/15] resources: Split out __allocate_resource()

[Yinghai Lu <yinghai@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

On Tue, Jun 26, 2012 at 12:01 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Jun 26, 2012 at 11:53 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> It will take bool lock, so we could use it in other functions that
>> hold the resource lock already.
>
> This is too damn ugly.
>
> These kinds of "conditionally take lock" things are always just bugs
> waiting to happen. Don't do it.
>
> Just make the rule be that the caller of the __allocate_resource
> helper has to hold the lock. Sure, that means that you need to then
> use split reallocate_resource() into a helper function (ie a static
> __reallocate_resource() that needs to have the lock taken by the
> caller too), but dammit, that's definitely the right thing to do
> anyway.
>
> These kinds of "bool lock" crap things have to die. They are *wrong*.
> They are a sign of bad locking rules.

You are right, please check updated one.

Thanks

Yinghai

[-- Attachment #2: probe_resource_1.patch --]
[-- Type: application/octet-stream, Size: 3516 bytes --]

Subject: [PATCH] resources: Split out __allocate_resource()

It will out hold lock, so we could use it in other functions that
hold the resource lock already.

-v2: according to Linus, using "bool lock" as parameter
     aka "conditionally take lock" is *wrong*.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>

---
 kernel/resource.c |   41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

Index: linux-2.6/kernel/resource.c
===================================================================
--- linux-2.6.orig/kernel/resource.c
+++ linux-2.6/kernel/resource.c
@@ -470,7 +470,7 @@ static int find_resource(struct resource
  * @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,
+static int __reallocate_resource(struct resource *root, struct resource *old,
 			resource_size_t newsize,
 			struct resource_constraint  *constraint)
 {
@@ -478,7 +478,6 @@ int reallocate_resource(struct resource
 	struct resource new = *old;
 	struct resource *conflict;
 
-	write_lock(&resource_lock);
 
 	if ((err = __find_resource(root, old, &new, newsize, constraint)))
 		goto out;
@@ -504,11 +503,20 @@ int reallocate_resource(struct resource
 		BUG_ON(conflict);
 	}
 out:
-	write_unlock(&resource_lock);
 	return err;
 }
+int reallocate_resource(struct resource *root, struct resource *old,
+			resource_size_t newsize,
+			struct resource_constraint  *constraint)
+{
+	int ret;
 
+	write_lock(&resource_lock);
+	ret = __reallocate_resource(root, old, newsize, constraint);
+	write_unlock(&resource_lock);
 
+	return ret;
+}
 /**
  * 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
@@ -521,7 +529,7 @@ out:
  * @alignf: alignment function, optional, called if not NULL
  * @alignf_data: arbitrary data to pass to the @alignf function
  */
-int allocate_resource(struct resource *root, struct resource *new,
+static int __allocate_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 *,
@@ -542,19 +550,36 @@ int allocate_resource(struct resource *r
 	constraint.alignf = alignf;
 	constraint.alignf_data = alignf_data;
 
-	if ( new->parent ) {
+	if (new->parent) {
 		/* resource is already allocated, try reallocating with
 		   the new constraints */
-		return reallocate_resource(root, new, size, &constraint);
+		return __reallocate_resource(root, new, size, &constraint);
 	}
 
-	write_lock(&resource_lock);
 	err = find_resource(root, new, size, &constraint);
 	if (err >= 0 && __request_resource(root, new))
 		err = -EBUSY;
-	write_unlock(&resource_lock);
+
 	return err;
 }
+int allocate_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)
+{
+	int ret;
+
+	write_lock(&resource_lock);
+	ret = __allocate_resource(root, new, size, min, max, align,
+				   alignf, alignf_data);
+	write_unlock(&resource_lock);
+
+	return ret;
+}
 
 EXPORT_SYMBOL(allocate_resource);