Re: idr_get_new_exact ?
On Mon, Sep 20, 2010 at 11:26:47PM +0200, Tejun Heo wrote: Hello, On 09/20/2010 10:35 PM, Roland Dreier wrote: Looks fine to me as an improvement over the status quo, but I wonder how many of these places could use the radix_tree stuff instead? If you're not using the ability of the idr code to assign an id for you, then it seems the radix_tree API is a better fit. I agree. Wouldn't those users better off simply using radix tree? It could go either way. I was about to write the same function when playing with it for IRQ mapping, the idea being to propagate the initial tree with sparse static vectors and then switch over to dynamic IDs for virtual IRQ creation. I ended up going with a radix tree for other reasons, though. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: idr_get_new_exact ?
Hello, On 09/23/2010 01:42 PM, Paul Mundt wrote: On Mon, Sep 20, 2010 at 11:26:47PM +0200, Tejun Heo wrote: Hello, On 09/20/2010 10:35 PM, Roland Dreier wrote: Looks fine to me as an improvement over the status quo, but I wonder how many of these places could use the radix_tree stuff instead? If you're not using the ability of the idr code to assign an id for you, then it seems the radix_tree API is a better fit. I agree. Wouldn't those users better off simply using radix tree? It could go either way. I was about to write the same function when playing with it for IRQ mapping, the idea being to propagate the initial tree with sparse static vectors and then switch over to dynamic IDs for virtual IRQ creation. I ended up going with a radix tree for other reasons, though. I see. If there are use cases where fixed and dynamic IDs need to be mixed, no objection from me. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
idr_get_new_exact ?
Occasionally, drivers care about the value that idr associates with
their pointers.
Today we have idr_get_new_above() which allocates a new idr entry
above or equal to a given starting id, but sometimes drivers need to
force an exact value.
To overcome this small API gap, drivers are wrapping idr_get_new_above
and then either BUG_ON() or just call idr_remove() and returns -EBUSY
when idr allocates them an id which is different than their requested
value.
There are only a handful of users who need this (see below. especially
note the i2c comment :), but it might be nice to have such an API (a
bit less of code, and a bit less error prone).
Would something like the below be desirable/acceptable ?
(untested. and i just picked the simplest and straight-forward way to
implement this; obviously it's not optimal since there's no reason to
even allocate an id if we know it's not the id we're looking for. but
it's enough to get the idea, it's not a hot path, and it's what
drivers are doing today)
---
drivers/i2c/i2c-core.c |9 +--
drivers/infiniband/core/cma.c |9 +--
drivers/infiniband/hw/cxgb3/iwch.h |3 +-
drivers/infiniband/hw/cxgb4/iw_cxgb4.h |3 +-
drivers/md/dm.c|8 +-
drivers/net/ppp_generic.c |7 +-
include/linux/idr.h|1 +
lib/idr.c | 39
8 files changed, 46 insertions(+), 33 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..6220900 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -964,14 +964,7 @@ retry:
return -ENOMEM;
mutex_lock(core_lock);
- /* above here means above or equal to, sigh;
-* we need the equal to result to force the result
-*/
- status = idr_get_new_above(i2c_adapter_idr, adap, adap-nr, id);
- if (status == 0 id != adap-nr) {
- status = -EBUSY;
- idr_remove(i2c_adapter_idr, id);
- }
+ status = idr_get_new_exact(i2c_adapter_idr, adap, adap-nr, id);
mutex_unlock(core_lock);
if (status == -EAGAIN)
goto retry;
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index b930b81..1756c99 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -1945,23 +1945,16 @@ static int cma_alloc_port(struct idr *ps,
struct rdma_id_private *id_priv,
return -ENOMEM;
do {
- ret = idr_get_new_above(ps, bind_list, snum, port);
+ ret = idr_get_new_exact(ps, bind_list, snum, port);
} while ((ret == -EAGAIN) idr_pre_get(ps, GFP_KERNEL));
if (ret)
goto err1;
- if (port != snum) {
- ret = -EADDRNOTAVAIL;
- goto err2;
- }
-
bind_list-ps = ps;
bind_list-port = (unsigned short) port;
cma_bind_port(bind_list, id_priv);
return 0;
-err2:
- idr_remove(ps, port);
err1:
kfree(bind_list);
return ret;
diff --git a/drivers/infiniband/hw/cxgb3/iwch.h
b/drivers/infiniband/hw/cxgb3/iwch.h
index a1c4457..71d9cae 100644
--- a/drivers/infiniband/hw/cxgb3/iwch.h
+++ b/drivers/infiniband/hw/cxgb3/iwch.h
@@ -160,8 +160,7 @@ static inline int insert_handle(struct iwch_dev
*rhp, struct idr *idr,
return -ENOMEM;
}
spin_lock_irq(rhp-lock);
- ret = idr_get_new_above(idr, handle, id, newid);
- BUG_ON(newid != id);
+ ret = idr_get_new_exact(idr, handle, id, newid);
spin_unlock_irq(rhp-lock);
} while (ret == -EAGAIN);
diff --git a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
index ed459b8..45567b1 100644
--- a/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
+++ b/drivers/infiniband/hw/cxgb4/iw_cxgb4.h
@@ -190,8 +190,7 @@ static inline int insert_handle(struct c4iw_dev
*rhp, struct idr *idr,
if (!idr_pre_get(idr, GFP_KERNEL))
return -ENOMEM;
spin_lock_irq(rhp-lock);
- ret = idr_get_new_above(idr, handle, id, newid);
- BUG_ON(newid != id);
+ ret = idr_get_new_exact(idr, handle, id, newid);
spin_unlock_irq(rhp-lock);
} while (ret == -EAGAIN);
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index ac384b2..dee497c 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1873,16 +1873,10 @@ static int specific_minor(int minor)
goto out;
}
- r = idr_get_new_above(_minor_idr, MINOR_ALLOCED, minor, m);
+ r = idr_get_new_exact(_minor_idr, MINOR_ALLOCED, minor, m);
if (r)
goto out;
- if (m != minor) {
- idr_remove(_minor_idr, m);
- r = -EBUSY;
- goto out;
- }
-
out
Re: idr_get_new_exact ?
On Mon, 20 Sep 2010 16:11:31 +0200 Ohad Ben-Cohen o...@wizery.com wrote: Occasionally, drivers care about the value that idr associates with their pointers. Today we have idr_get_new_above() which allocates a new idr entry above or equal to a given starting id, but sometimes drivers need to force an exact value. To overcome this small API gap, drivers are wrapping idr_get_new_above and then either BUG_ON() or just call idr_remove() and returns -EBUSY when idr allocates them an id which is different than their requested value. There are only a handful of users who need this (see below. especially note the i2c comment :), but it might be nice to have such an API (a bit less of code, and a bit less error prone). Would something like the below be desirable/acceptable ? It seems OK to me - it's an improvement over what we have now. (untested. and i just picked the simplest and straight-forward way to implement this; obviously it's not optimal since there's no reason to even allocate an id if we know it's not the id we're looking for. but it's enough to get the idea, it's not a hot path, and it's what drivers are doing today) Sure, we can speed it up later if that appears to be necessary. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: idr_get_new_exact ?
On 09/20/2010 02:31 PM, Andrew Morton wrote: On Mon, 20 Sep 2010 16:11:31 +0200 Ohad Ben-Coheno...@wizery.com wrote: Occasionally, drivers care about the value that idr associates with their pointers. Today we have idr_get_new_above() which allocates a new idr entry above or equal to a given starting id, but sometimes drivers need to force an exact value. To overcome this small API gap, drivers are wrapping idr_get_new_above and then either BUG_ON() or just call idr_remove() and returns -EBUSY when idr allocates them an id which is different than their requested value. There are only a handful of users who need this (see below. especially note the i2c comment :), but it might be nice to have such an API (a bit less of code, and a bit less error prone). Would something like the below be desirable/acceptable ? It seems OK to me - it's an improvement over what we have now. Looks ok to me also. This is exactly what cxgb* needs. IE the driver manages the ID space and never expects an idr insertion to fail because its already inserted. That constitutes a driver bug (which is why the BUG_ON() is there :)). Steve. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: idr_get_new_exact ?
Occasionally, drivers care about the value that idr associates with their pointers. Today we have idr_get_new_above() which allocates a new idr entry above or equal to a given starting id, but sometimes drivers need to force an exact value. To overcome this small API gap, drivers are wrapping idr_get_new_above and then either BUG_ON() or just call idr_remove() and returns -EBUSY when idr allocates them an id which is different than their requested value. Looks fine to me as an improvement over the status quo, but I wonder how many of these places could use the radix_tree stuff instead? If you're not using the ability of the idr code to assign an id for you, then it seems the radix_tree API is a better fit. - R. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: idr_get_new_exact ?
Hello, On 09/20/2010 10:35 PM, Roland Dreier wrote: Looks fine to me as an improvement over the status quo, but I wonder how many of these places could use the radix_tree stuff instead? If you're not using the ability of the idr code to assign an id for you, then it seems the radix_tree API is a better fit. I agree. Wouldn't those users better off simply using radix tree? Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [dm-devel] idr_get_new_exact ?
On Mon, Sep 20, 2010 at 11:26:47PM +0200, Tejun Heo wrote: I agree. Wouldn't those users better off simply using radix tree? Can't speak for the other users, but dm uses it both ways - normally happy with any id (minor number), but sometimes user requires a specific one. Alasdair -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
