RFC: Boiler plate functions for ida / idr allocation?

RFC: Boiler plate functions for ida / idr allocation?

[Jonathan Cameron]

One bit of common functionality that a lot of subsystem cores
(and some drivers) implement is allocation of unique id's.

Now we have ida / idr for this. However, given I copied someone
else's code for those in IIO, I took a look around to see how
common these 20 ish lines of code were.

The answer is fairly, though in some cases in subtly different
forms.  Seems to be from a naive standpoint to be a classic case
for a convenient library function.

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)

However, a few users of idr's that look like they are using them only as idas
(perhaps predating ida availability?)

e.g. 
drivers/hwmon/hwmon.c

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

---
 drivers/misc/cb710/core.c                  |   15 +-----
 drivers/scsi/sd.c                          |   10 +----
 drivers/staging/iio/iio.h                  |    1 -
 drivers/staging/iio/industrialio-core.c    |   38 ++--------------
 drivers/staging/iio/industrialio-trigger.c |   65 ++++++----------------------
 include/linux/idr.h                        |    1 +
 lib/idr.c                                  |   20 +++++++++
 7 files changed, 42 insertions(+), 108 deletions(-)

diff --git a/drivers/misc/cb710/core.c b/drivers/misc/cb710/core.c
index efec413..914be30 100644
--- a/drivers/misc/cb710/core.c
+++ b/drivers/misc/cb710/core.c
@@ -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;
 
 	dev_info(&pdev->dev, "id %d, IO 0x%p, IRQ %d\n",
 		chip->platform_id, chip->iobase, pdev->irq);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 953773c..aadf8a3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2580,15 +2580,7 @@ static int sd_probe(struct device *dev)
 	if (!gd)
 		goto out_free;
 
-	do {
-		if (!ida_pre_get(&sd_index_ida, GFP_KERNEL))
-			goto out_put;
-
-		spin_lock(&sd_index_lock);
-		error = ida_get_new(&sd_index_ida, &index);
-		spin_unlock(&sd_index_lock);
-	} while (error == -EAGAIN);
-
+	error = ida_get_id(&sd_index_lock, &sd_index_ida, &index);
 	if (error)
 		goto out_put;
 
diff --git a/drivers/staging/iio/iio.h b/drivers/staging/iio/iio.h
index 2a8c8e2..7bdda68 100644
--- a/drivers/staging/iio/iio.h
+++ b/drivers/staging/iio/iio.h
@@ -426,6 +426,5 @@ static inline bool iio_ring_enabled(struct iio_dev *dev_info)
 
 struct ida;
 
-int iio_get_new_ida_val(struct ida *this_ida);
 void iio_free_ida_val(struct ida *this_ida, int id);
 #endif /* _INDUSTRIAL_IO_H_ */
diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c
index a9628ed..258d68e 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -250,15 +250,8 @@ int iio_device_get_chrdev_minor(void)
 {
 	int ret, val;
 
-ida_again:
-	if (unlikely(ida_pre_get(&iio_chrdev_ida, GFP_KERNEL) == 0))
-		return -ENOMEM;
-	spin_lock(&iio_ida_lock);
-	ret = ida_get_new(&iio_chrdev_ida, &val);
-	spin_unlock(&iio_ida_lock);
-	if (unlikely(ret == -EAGAIN))
-		goto ida_again;
-	else if (unlikely(ret))
+	ret = ida_get_id(&iio_ida_lock, &iio_chrdev_ida, &val);
+	if (ret < 0)
 		return ret;
 	if (val > IIO_DEV_MAX)
 		return -ENOMEM;
@@ -794,28 +787,6 @@ static void iio_device_unregister_sysfs(struct iio_dev *dev_info)
 		sysfs_remove_group(&dev_info->dev.kobj, dev_info->info->attrs);
 }
 
-/* Return a negative errno on failure */
-int iio_get_new_ida_val(struct ida *this_ida)
-{
-	int ret;
-	int val;
-
-ida_again:
-	if (unlikely(ida_pre_get(this_ida, GFP_KERNEL) == 0))
-		return -ENOMEM;
-
-	spin_lock(&iio_ida_lock);
-	ret = ida_get_new(this_ida, &val);
-	spin_unlock(&iio_ida_lock);
-	if (unlikely(ret == -EAGAIN))
-		goto ida_again;
-	else if (unlikely(ret))
-		return ret;
-
-	return val;
-}
-EXPORT_SYMBOL(iio_get_new_ida_val);
-
 void iio_free_ida_val(struct ida *this_ida, int id)
 {
 	spin_lock(&iio_ida_lock);
@@ -1179,9 +1150,8 @@ int iio_device_register(struct iio_dev *dev_info)
 {
 	int ret;
 
-	dev_info->id = iio_get_new_ida_val(&iio_ida);
-	if (dev_info->id < 0) {
-		ret = dev_info->id;
+	ret = ida_get_id(&iio_ida_lock, &iio_ida, &dev_info->id);
+	if (ret < 0) {
 		dev_err(&dev_info->dev, "Failed to get id\n");
 		goto error_ret;
 	}
diff --git a/drivers/staging/iio/industrialio-trigger.c b/drivers/staging/iio/industrialio-trigger.c
index 6940f8c..75d19f6 100644
--- a/drivers/staging/iio/industrialio-trigger.c
+++ b/drivers/staging/iio/industrialio-trigger.c
@@ -31,8 +31,8 @@
  * Any other suggestions?
  */
 
-static DEFINE_IDR(iio_trigger_idr);
-static DEFINE_SPINLOCK(iio_trigger_idr_lock);
+static DEFINE_IDA(iio_trigger_ida);
+static DEFINE_SPINLOCK(iio_trigger_ida_lock);
 
 /* Single list of all available triggers */
 static LIST_HEAD(iio_trigger_list);
@@ -52,65 +52,22 @@ static ssize_t iio_trigger_read_name(struct device *dev,
 static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
 
 /**
- * iio_trigger_register_sysfs() - create a device for this trigger
- * @trig_info:	the trigger
- *
- * Also adds any control attribute registered by the trigger driver
- **/
-static int iio_trigger_register_sysfs(struct iio_trigger *trig_info)
-{
-	return sysfs_add_file_to_group(&trig_info->dev.kobj,
-				       &dev_attr_name.attr,
-				       NULL);
-}
-
-static void iio_trigger_unregister_sysfs(struct iio_trigger *trig_info)
-{
-	sysfs_remove_file_from_group(&trig_info->dev.kobj,
-					   &dev_attr_name.attr,
-					   NULL);
-}
-
-
-/**
- * iio_trigger_register_id() - get a unique id for this trigger
- * @trig_info:	the trigger
- **/
-static int iio_trigger_register_id(struct iio_trigger *trig_info)
-{
-	int ret = 0;
-
-idr_again:
-	if (unlikely(idr_pre_get(&iio_trigger_idr, GFP_KERNEL) == 0))
-		return -ENOMEM;
-
-	spin_lock(&iio_trigger_idr_lock);
-	ret = idr_get_new(&iio_trigger_idr, NULL, &trig_info->id);
-	spin_unlock(&iio_trigger_idr_lock);
-	if (unlikely(ret == -EAGAIN))
-		goto idr_again;
-	else if (likely(!ret))
-		trig_info->id = trig_info->id & MAX_ID_MASK;
-
-	return ret;
-}
-
-/**
  * iio_trigger_unregister_id() - free up unique id for use by another trigger
  * @trig_info: the trigger
  **/
 static void iio_trigger_unregister_id(struct iio_trigger *trig_info)
 {
-	spin_lock(&iio_trigger_idr_lock);
-	idr_remove(&iio_trigger_idr, trig_info->id);
-	spin_unlock(&iio_trigger_idr_lock);
+	spin_lock(&iio_trigger_ida_lock);
+	ida_remove(&iio_trigger_ida, trig_info->id);
+	spin_unlock(&iio_trigger_ida_lock);
 }
 
 int iio_trigger_register(struct iio_trigger *trig_info)
 {
 	int ret;
 
-	ret = iio_trigger_register_id(trig_info);
+	ret = ida_get_id(&iio_trigger_ida_lock,
+			 &iio_trigger_ida, &trig_info->id);
 	if (ret)
 		goto error_ret;
 	/* Set the name used for the sysfs directory etc */
@@ -121,7 +78,9 @@ int iio_trigger_register(struct iio_trigger *trig_info)
 	if (ret)
 		goto error_unregister_id;
 
-	ret = iio_trigger_register_sysfs(trig_info);
+	ret = sysfs_add_file_to_group(&trig_info->dev.kobj,
+				      &dev_attr_name.attr,
+				      NULL);
 	if (ret)
 		goto error_device_del;
 
@@ -147,7 +106,9 @@ void iio_trigger_unregister(struct iio_trigger *trig_info)
 	list_del(&trig_info->list);
 	mutex_unlock(&iio_trigger_list_lock);
 
-	iio_trigger_unregister_sysfs(trig_info);
+	sysfs_remove_file_from_group(&trig_info->dev.kobj,
+				     &dev_attr_name.attr,
+				     NULL);
 	iio_trigger_unregister_id(trig_info);
 	/* Possible issue in here */
 	device_unregister(&trig_info->dev);
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 13a801f..412b46c 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -147,5 +147,6 @@ void ida_destroy(struct ida *ida);
 void ida_init(struct ida *ida);
 
 void __init idr_init_cache(void);
+int ida_get_id(spinlock_t *lock, struct ida *ida, int *val);
 
 #endif /* __IDR_H__ */
diff --git a/lib/idr.c b/lib/idr.c
index e15502e..d68e222 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -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;
+
+	return ret;
+}
+EXPORT_SYMBOL(ida_get_id);
-- 
1.7.3.4

Re: RFC: Boiler plate functions for ida / idr allocation?

[Stefan Richter]

On Jul 13 Jonathan Cameron wrote at LKML:
[...]
> 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)
[...]

Boaz, Benny,

osd_uld.c::osd_minor_ida is accessed unsafely.
Device probe() and remove() methods are not globally serialized.
-- 
Stefan Richter
-=====-==-== -=== -==-=
http://arcgraph.de/sr/

Re: RFC: Boiler plate functions for ida / idr allocation?

[Stefan Richter]

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:

[...]
> 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?

[...]
> --- a/drivers/misc/cb710/core.c
> +++ b/drivers/misc/cb710/core.c
> @@ -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;
>  
>  	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.

[...]
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -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;
> +
> +	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.
-- 
Stefan Richter
-=====-==-== -=== -==-=
http://arcgraph.de/sr/

Re: RFC: Boiler plate functions for ida / idr allocation?

[Jonathan Cameron]

On 07/13/11 14:14, Stefan Richter wrote:
> 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:
> 
> [...]
>> 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?
> 
> [...]
>> --- a/drivers/misc/cb710/core.c
>> +++ b/drivers/misc/cb710/core.c
>> @@ -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;
>>  
>>  	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.
Good point. I'd actually suggest adding a paired release_id function with similar
semantics to the get function just to keep things consistent.
> 
> [...]
>> --- a/lib/idr.c
>> +++ b/lib/idr.c
>> @@ -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;
>> +
>> +	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.
Of course, I was just being lazy for an RFC ;)  Is suspect any formal submission
will need a few iterations before everyone is happy.

Re: RFC: Boiler plate functions for ida / idr allocation?

[Tejun Heo]

(cc'ing Rusty Russell)
Hello,

On Wed, Jul 13, 2011 at 10:44:32AM +0100, Jonathan Cameron wrote:
> 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.

Thanks.

-- 
tejun

Re: RFC: Boiler plate functions for ida / idr allocation?

[Jonathan Cameron]

On 07/13/11 14:31, Tejun Heo wrote:
> (cc'ing Rusty Russell)
> Hello,
> 
> On Wed, Jul 13, 2011 at 10:44:32AM +0100, Jonathan Cameron wrote:
>> 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.
Cool.

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

Re: RFC: Boiler plate functions for ida / idr allocation?

[Boaz Harrosh]

On 07/13/2011 05:41 AM, Stefan Richter wrote:
> On Jul 13 Jonathan Cameron wrote at LKML:
> [...]
>> 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)
> [...]
> 
> Boaz, Benny,
> 
> 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.

Thanks
Boaz

Re: RFC: Boiler plate functions for ida / idr allocation?

[Stefan Richter]

On Jul 15 Boaz Harrosh wrote:
> On 07/13/2011 05:41 AM, Stefan Richter wrote:
> > 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.

Could actually be that there was or even still is some degree of
serialization by the SCSI core in typical usage.  But I think the
general case is nowadays fully concurrent.  E.g.
    echo $name > /sys/bus/scsi/drivers/osd*/{,un}bind
if issued in parallel for different device names.  Though it is probably
borderline impossible to actually hit a concurrent osd_minor_ida access on
purpose.
-- 
Stefan Richter
-=====-==-== -=== -====
http://arcgraph.de/sr/

Re: RFC: Boiler plate functions for ida / idr allocation?

[Rusty Russell]

On Wed, 13 Jul 2011 14:48:34 +0100, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> Rusty, are you pursuing this?  Approach looks sensible to me.

OK, I'll polish up what I had based on Tejun's feedback and repost.

Thanks for the prod...
Rusty.

Re: RFC: Boiler plate functions for ida / idr allocation?

[Rusty Russell]

On Wed, 13 Jul 2011 14:48:34 +0100, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> > 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.

Yep, here's my latest version.  Feel free to run with it, or not.

Thanks!
Rusty.

From: Rusty Russell <rusty@rustcorp.com.au>
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 <rusty@rustcorp.com.au>
---
 include/linux/idr.h |    4 +
 lib/idr.c           |  116 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/include/linux/idr.h b/include/linux/idr.h
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -146,6 +146,10 @@ void ida_remove(struct ida *ida, int id)
 void ida_destroy(struct ida *ida);
 void ida_init(struct ida *ida);
 
+int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
+		   gfp_t gfp_mask);
+void ida_simple_remove(struct ida *ida, unsigned int id);
+
 void __init idr_init_cache(void);
 
 #endif /* __IDR_H__ */
diff --git a/lib/idr.c b/lib/idr.c
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -34,8 +34,10 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/idr.h>
+#include <linux/spinlock.h>
 
 static struct kmem_cache *idr_layer_cache;
+static DEFINE_SPINLOCK(simple_ida);
 
 static struct idr_layer *get_from_free_list(struct idr *idp)
 {
@@ -926,6 +928,120 @@ void ida_destroy(struct ida *ida)
 EXPORT_SYMBOL(ida_destroy);
 
 /**
+ * ida_simple_get - get a new id.
+ * @ida: the (initialized) ida.
+ * @start: the minimum id (inclusive, < 0x8000000)
+ * @end: the maximum id (exclusive, < 0x8000000 or 0)
+ * @gfp_mask: memory allocation flags
+ *
+ * Allocates an id in the range start <= id < end, or returns -ENOSPC.
+ * On memory allocation failure, returns -ENOMEM.
+ *
+ * Use ida_simple_remove() to get rid of an id.
+ */
+int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
+		   gfp_t gfp_mask)
+{
+	int ret, id;
+	unsigned int max;
+
+	BUG_ON((int)start < 0);
+	BUG_ON((int)end < 0);
+
+	if (end == 0)
+		max = 0x80000000;
+	else {
+		BUG_ON(end < start);
+		max = end - 1;
+	}
+
+again:
+	if (!ida_pre_get(ida, gfp_mask))
+		return -ENOMEM;
+
+	spin_lock(&simple_ida);
+	ret = ida_get_new_above(ida, start, &id);
+	if (!ret) {
+		if (id > max) {
+			ida_remove(ida, id);
+			ret = -ENOSPC;
+		} else {
+			ret = id;
+		}
+	}
+	spin_unlock(&simple_ida);
+
+	if (unlikely(ret == -EAGAIN))
+		goto again;
+
+	return ret;
+}
+EXPORT_SYMBOL(ida_simple_get);
+
+/**
+ * ida_simple_remove - remove an allocated id.
+ * @ida: the (initialized) ida.
+ * @id: the id returned by ida_simple_get.
+ */
+void ida_simple_remove(struct ida *ida, unsigned int id)
+{
+	BUG_ON((int)id < 0);
+	spin_lock(&simple_ida);
+	ida_remove(ida, id);
+	spin_unlock(&simple_ida);
+}
+EXPORT_SYMBOL(ida_simple_remove);
+
+#ifdef TEST
+static int test_ida_simple(void)
+{
+	int i;
+	DEFINE_IDA(ida);
+
+	/* Simple range 0..0. */
+	i = ida_simple_get(&ida, 0, 1, GFP_KERNEL);
+	BUG_ON(i != 0);
+	i = ida_simple_get(&ida, 0, 1, GFP_KERNEL);
+	BUG_ON(i != -ENOSPC);
+	ida_simple_remove(&ida, 0);
+	i = ida_simple_get(&ida, 0, 1, GFP_KERNEL);
+	BUG_ON(i != 0);
+
+	/* Empty range. */
+	i = ida_simple_get(&ida, 0x7FFFFFFE, 0x7FFFFFFE, GFP_KERNEL);
+	BUG_ON(i != -ENOSPC);
+
+	/* Top of range. */
+	i = ida_simple_get(&ida, 0x7FFFFFFE, 0x7FFFFFFF, GFP_KERNEL);
+	BUG_ON(i != 0x7FFFFFFE);
+	i = ida_simple_get(&ida, 0x7FFFFFFE, 0x7FFFFFFF, GFP_KERNEL);
+	BUG_ON(i != -ENOSPC);
+	ida_simple_remove(&ida, 0x7FFFFFFE);
+
+	/* End of range testing. */
+	i = ida_simple_get(&ida, 0x7FFFFFFE, 0, GFP_KERNEL);
+	BUG_ON(i != 0x7FFFFFFE);
+
+	i = ida_simple_get(&ida, 0x7FFFFFFE, 0, GFP_KERNEL);
+	BUG_ON(i != 0x7FFFFFFF);
+
+	i = ida_simple_get(&ida, 0x7FFFFFFE, 0, GFP_KERNEL);
+	BUG_ON(i != -ENOSPC);
+
+	/* Remove works. */
+	i = ida_simple_get(&ida, 0x7FFFFFFD, 0, GFP_KERNEL);
+	BUG_ON(i != 0x7FFFFFFD);
+
+	ida_simple_remove(&ida, 0x7FFFFFFE);
+	i = ida_simple_get(&ida, 0x7FFFFFFD, 0, GFP_KERNEL);
+	BUG_ON(i != 0x7FFFFFFE);
+
+	ida_destroy(&ida);
+	return 0;
+}
+#endif
+
+/**
  * ida_init - initialize ida handle
  * @ida:	ida handle
  *

Re: RFC: Boiler plate functions for ida / idr allocation?

[Tejun Heo]

On Thu, Jul 21, 2011 at 05:07:36PM +0930, Rusty Russell wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
> 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 <rusty@rustcorp.com.au>
...
>  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 <tj@kernel.org>

I guess this is best routed through -mm?

Thanks.

-- 
tejun

Re: RFC: Boiler plate functions for ida / idr allocation?

[Jonathan Cameron]

On 07/21/11 09:19, Tejun Heo wrote:
> On Thu, Jul 21, 2011 at 05:07:36PM +0930, Rusty Russell wrote:
>> From: Rusty Russell <rusty@rustcorp.com.au>
>> 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 <rusty@rustcorp.com.au>
> ...
>>  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 <tj@kernel.org>
> 
> I guess this is best routed through -mm?
> 
> Thanks.
> 
Thanks Rusty. Just what I was looking for. I'll push out some patches using
this once it's picked up.

Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

Re: RFC: Boiler plate functions for ida / idr allocation?

[Tejun Heo]

On Thu, Jul 21, 2011 at 10:19:46AM +0200, Tejun Heo wrote:
> On Thu, Jul 21, 2011 at 05:07:36PM +0930, Rusty Russell wrote:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 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 <rusty@rustcorp.com.au>
> ...
> >  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.

Thanks.

-- 
tejun

Re: RFC: Boiler plate functions for ida / idr allocation?

[Rusty Russell]

On Thu, 21 Jul 2011 10:35:01 +0200, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Jul 21, 2011 at 10:19:46AM +0200, Tejun Heo wrote:
> > On Thu, Jul 21, 2011 at 05:07:36PM +0930, Rusty Russell wrote:
> > > From: Rusty Russell <rusty@rustcorp.com.au>
> > > 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 <rusty@rustcorp.com.au>
> > ...
> > >  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.

Re: RFC: Boiler plate functions for ida / idr allocation?

[Jonathan Cameron]

On 07/22/11 12:13, Rusty Russell wrote:
> On Thu, 21 Jul 2011 10:35:01 +0200, Tejun Heo <tj@kernel.org> wrote:
>> On Thu, Jul 21, 2011 at 10:19:46AM +0200, Tejun Heo wrote:
>>> On Thu, Jul 21, 2011 at 05:07:36PM +0930, Rusty Russell wrote:
>>>> From: Rusty Russell <rusty@rustcorp.com.au>
>>>> 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 <rusty@rustcorp.com.au>
>>> ...
>>>>  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.
> 
Given the timing, it's clearly 3.2 merge window material anyway. Too late to
push any users this time round even if we hustled it through.

Quite a few 'interesting' users out there now I look into them.  Might take
a little while to get them past the relevant maintainers.

I've just sent out a few of the simpler cases.  I guess Andrew may take the core
patch and the others will route through relevant subsystems.

A few more to do. Some of them need a little more thought.

Jonathan