Patchwork [-v12,01/15] resources: Split out __allocate_resource()

login
register
mail settings
Submitter Yinghai Lu
Date June 26, 2012, 6:53 p.m.
Message ID <1340736849-14875-2-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/167452/
State Rejected
Headers show

Comments

Yinghai Lu - June 26, 2012, 6:53 p.m.
It will take bool lock, so we could use it in other functions that
hold the resource lock already.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/resource.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)
Linus Torvalds - June 26, 2012, 7:01 p.m.
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.

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - June 26, 2012, 8:33 p.m.
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
Linus Torvalds - June 26, 2012, 8:43 p.m.
On Tue, Jun 26, 2012 at 1:33 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
> You are right, please check updated one.

I think you can now also remove the "goto out" and in
__reallocate_resource() and replace it with a "return err;" instead.

But yeah, this looks fine.

             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/kernel/resource.c b/kernel/resource.c
index dc8b477..3f6e522 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -521,14 +521,14 @@  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 *,
 						const struct resource *,
 						resource_size_t,
 						resource_size_t),
-		      void *alignf_data)
+		      void *alignf_data, bool lock)
 {
 	int err;
 	struct resource_constraint constraint;
@@ -542,19 +542,35 @@  int allocate_resource(struct resource *root, struct resource *new,
 	constraint.alignf = alignf;
 	constraint.alignf_data = alignf_data;
 
-	if ( new->parent ) {
+	if (new->parent && lock) {
 		/* resource is already allocated, try reallocating with
 		   the new constraints */
 		return reallocate_resource(root, new, size, &constraint);
 	}
 
-	write_lock(&resource_lock);
+	if (lock)
+		write_lock(&resource_lock);
 	err = find_resource(root, new, size, &constraint);
 	if (err >= 0 && __request_resource(root, new))
 		err = -EBUSY;
-	write_unlock(&resource_lock);
+	if (lock)
+		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)
+{
+	bool lock = true;
+
+	return __allocate_resource(root, new, size, min, max, align,
+				   alignf, alignf_data, lock);
+}
 
 EXPORT_SYMBOL(allocate_resource);