RFC: Boiler plate functions for ida / idr allocation?

Taking ida's first, how about the following patch? I'm not at all attached to the form it takes, merely to cutting out on the cut and paste.

Note I've done only a few random places. There aren't that many ida users and I've only played with that for now.

Remaining ida cases:

drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c: (cleans up in case of out of range) drivers/scsi/osd/osd_uld.c: (locks not taken)

On Jul 13 Jonathan Cameron wrote at LKML:
[...]

Quoted 9 lines

Taking ida's first, how about the following patch? I'm not at all attached to the form it takes, merely to cutting out on the cut and paste.

Note I've done only a few random places. There aren't that many ida users and I've only played with that for now.

Remaining ida cases:

Quoted 2 lines

drivers/gpu/drm/vmwgfx/vmwgfx_gmrid_manager.c: (cleans up in case of out of range) drivers/scsi/osd/osd_uld.c: (locks not taken)

[...]

Boaz, Benny,

osd_uld.c::osd_minor_ida is accessed unsafely.
Device probe() and remove() methods are not globally serialized.

Quoted 2 lines · Stefan Richter

osd_uld.c::osd_minor_ida is accessed unsafely.
Device probe() and remove() methods are not globally serialized.

Sorry for the delay. Vacation

Thanks Stefan I'll look into it. I remember I thought about it and tested it at the time, but I might be wrong. I'll look into it.

Taking ida's first, how about the following patch? I'm not at all attached to the form it takes, merely to cutting out on the cut and paste.

The other thing this highlights is that I suspect quite a few are protected by spin locks when a mutex would be fine. Hence that might be worth tidying up first.

drivers/misc/cb710/core.c +0 -8

@@ -254,18 +254,9 @@ static int __devinit cb710_probe(struct pci_dev *pdev,
 	if (err)
 		return err;
 
-	do {
-		if (!ida_pre_get(&cb710_ida, GFP_KERNEL))
-			return -ENOMEM;
-
-		spin_lock_irqsave(&cb710_ida_lock, flags);
-		err = ida_get_new(&cb710_ida, &chip->platform_id);
-		spin_unlock_irqrestore(&cb710_ida_lock, flags);
-

lib/idr.c +10 -0

@@ -939,3 +939,23 @@ void ida_init(struct ida *ida)
 
 }
 EXPORT_SYMBOL(ida_init);
+
+int ida_get_id(spinlock_t *lock, struct ida *ida, int *val)
+{
+	int ret = 0;
+ida_again:
+	if (unlikely(ida_pre_get(ida, GFP_KERNEL) == 0))
+		return -ENOMEM;
+
+	spin_lock(lock);
+	ret = ida_get_new(ida, val);

Quoted 3 lines · On Jul 13 Jonathan Cameron wrote:

Taking ida's first, how about the following patch? I'm not at all attached to the form it takes, merely to cutting out on the cut and paste.

Not a big-picture opinion here whether this is a good thing; only some small comments on side issues:

[...]

Quoted 2 lines

The other thing this highlights is that I suspect quite a few are protected by spin locks when a mutex would be fine. Hence that might be worth tidying up first.

It seems to be the other way around in this case: Why use a mutex if a spinlock is fine?

[...]

Quoted 22 lines

drivers/misc/cb710/core.c +3 -12

@@ -254,18 +254,9 @@ static int __devinit cb710_probe(struct pci_dev *pdev,
 	if (err)
 		return err;
 
-	do {
-		if (!ida_pre_get(&cb710_ida, GFP_KERNEL))
-			return -ENOMEM;
-
-		spin_lock_irqsave(&cb710_ida_lock, flags);
-		err = ida_get_new(&cb710_ida, &chip->platform_id);
-		spin_unlock_irqrestore(&cb710_ida_lock, flags);
-
-		if (err && err != -EAGAIN)
-			return err;
-	} while (err);
-
+	err = ida_get_id(&cb710_ida_lock, &cb710_ida, &chip->platform_id);
+	if (err)
+		return err;
 

drivers/misc/cb710/core.c static int __devinit cb710_probe(struct pci_dev *pdev, · line 262

Quoted 2 lines

drivers/misc/cb710/core.c +0 -0

 	dev_info(&pdev->dev, "id %d, IO 0x%p, IRQ %d\n",
 		chip->platform_id, chip->iobase, pdev->irq);

To balance this change to cb710_probe, also switch from spin_lock_irqsave/ spin_unlock_irqrestore to spin_lock/spin_unlock in cb710_remove_one for clarity.

[...]

Quoted 23 lines

lib/idr.c +17 -0

@@ -939,3 +939,23 @@ void ida_init(struct ida *ida)
 
 }
 EXPORT_SYMBOL(ida_init);
+
+int ida_get_id(spinlock_t *lock, struct ida *ida, int *val)
+{
+	int ret = 0;
+ida_again:
+	if (unlikely(ida_pre_get(ida, GFP_KERNEL) == 0))
+		return -ENOMEM;
+
+	spin_lock(lock);
+	ret = ida_get_new(ida, val);
+	spin_unlock(lock);
+
+	if (unlikely (ret == -EAGAIN))
+		goto ida_again;
+	else if (likely(!ret))
+		*val = *val & MAX_ID_MASK;
+

lib/idr.c void ida_init(struct ida *ida) · line 961

Quoted 3 lines

lib/idr.c +3 -0

+	return ret;
+}
+EXPORT_SYMBOL(ida_get_id);

A new exported function (in lib/ even) should come with a kerneldoc comment of course. Here it is among else noteworthy that the caller must provide GFP_KERNEL allocations capable context and that @lock cannot be shared with users in IRQ or softIRQ contexts.

The other thing this highlights is that I suspect quite a few are protected by spin locks when a mutex would be fine. Hence that might be worth tidying up first.

Anyhow, a cleanup worth making? (obviously the exact form needs some work, but I think the following is enough to start a discussion!)

Subject: [PATCH] ida utility function

(cc'ing Rusty Russell)
Hello,

Quoted 7 lines · Jonathan Cameron

The other thing this highlights is that I suspect quite a few are protected by spin locks when a mutex would be fine. Hence that might be worth tidying up first.

Anyhow, a cleanup worth making? (obviously the exact form needs some work, but I think the following is enough to start a discussion!)

Subject: [PATCH] ida utility function

Rusty suggested similar addition some weeks ago, so people are really getting annoyed by this.

http://thread.gmane.org/gmane.linux.kernel/1148513/focus=74236

Here's an interface that I think should work,

	int ida_get(struct ida *ida, int begin, int end, gfp_t gfp);

It uses an internal spinlock, returns the allocated ID and @end <= 0 indicates no limit.

Rusty, are you pursuing this? Approach looks sensible to me.

Quoted 6 lines

Here's an interface that I think should work,

	int ida_get(struct ida *ida, int begin, int end, gfp_t gfp);

It uses an internal spinlock, returns the allocated ID and @end <= 0 indicates no limit.

Cool.

Rusty, are you pursuing this? Approach looks sensible to me.

From: Rusty Russell <[email protected]>
Subject: ida: Simplified functions for id allocation.

The current hyper-optimized functions are overkill if you simply want to allocate an id for a device. Create versions which use an internal lock.

Thanks to Tejun for feedback. Feel free to delete the #ifdef TEST code.

Signed-off-by Rusty Russell <[email protected]>

static struct kmem_cache *idr_layer_cache;
+static DEFINE_SPINLOCK(simple_ida);

Quoted 11 lines · Rusty Russell

From: Rusty Russell <[email protected]>
Subject: ida: Simplified functions for id allocation.

The current hyper-optimized functions are overkill if you simply want to allocate an id for a device. Create versions which use an internal lock.

Thanks to Tejun for feedback. Feel free to delete the #ifdef TEST code.

Signed-off-by Rusty Russell <[email protected]>

...

Quoted 2 lines

static struct kmem_cache *idr_layer_cache;
+static DEFINE_SPINLOCK(simple_ida);

I think the name is a bit confusing. Maybe simple_ida_lock is better? Other than that,

Acked-by Tejun Heo <[email protected]>

I guess this is best routed through -mm?

Thanks.

Quoted 10 lines · Rusty Russell

From: Rusty Russell <[email protected]>
Subject: ida: Simplified functions for id allocation.

The current hyper-optimized functions are overkill if you simply want to allocate an id for a device. Create versions which use an internal lock.

Thanks to Tejun for feedback. Feel free to delete the #ifdef TEST code.

Quoted 1 line

Signed-off-by Rusty Russell <[email protected]>

...

Quoted 2 lines

static struct kmem_cache *idr_layer_cache;
+static DEFINE_SPINLOCK(simple_ida);

I think the name is a bit confusing. Maybe simple_ida_lock is better? Other than that,

Quoted 13 lines · Tejun Heo

Quoted 11 lines · Rusty Russell

From: Rusty Russell <[email protected]>
Subject: ida: Simplified functions for id allocation.

The current hyper-optimized functions are overkill if you simply want to allocate an id for a device. Create versions which use an internal lock.

Thanks to Tejun for feedback. Feel free to delete the #ifdef TEST code.

Signed-off-by Rusty Russell <[email protected]>

...

Quoted 5 lines

Quoted 2 lines

static struct kmem_cache *idr_layer_cache;
+static DEFINE_SPINLOCK(simple_ida);

I think the name is a bit confusing. Maybe simple_ida_lock is better? Other than that,

Ooh, one more thing, maybe it would be better to use spin_lock_irq() to allow calling free under other irq locks.

Quoted 14 lines · Tejun Heo

Quoted 13 lines · Tejun Heo

Quoted 11 lines · Rusty Russell

From: Rusty Russell <[email protected]>
Subject: ida: Simplified functions for id allocation.

The current hyper-optimized functions are overkill if you simply want to allocate an id for a device. Create versions which use an internal lock.

Thanks to Tejun for feedback. Feel free to delete the #ifdef TEST code.

Signed-off-by Rusty Russell <[email protected]>

...

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

static struct kmem_cache *idr_layer_cache;
+static DEFINE_SPINLOCK(simple_ida);

I think the name is a bit confusing. Maybe simple_ida_lock is better? Other than that,

Ooh, one more thing, maybe it would be better to use spin_lock_irq() to allow calling free under other irq locks.

Jonathan, please take original patch and mod it to taste, and produce a series on it you can push to Andrew.

As for irqsave etc, as soon as someone needs that we can add it... Jonathan will run into it soon enough.

Thanks,
Rusty.