(Andrew, I think this is probably best routed via -mm since it touches
several different places)
This is the second version of this patchset. The main changes are:
- removed an incorrect comment on the new allocator function that
stemmed from a misunderstanding of the IDR code. Also fixed up
the kerneldoc comment.
- the idr_init_cyclic function has been removed, since it doesn't add
any benefit over a "normal" idr_init.
- minor change to the caller in inotify code to make it not allocate
an ID of 0. The existing code starts at 1, so this seems like a
reasonable precaution.
Original cover letter follows:
As Tejun points out, there are several users of the IDR facility that
attempt to use it in a cyclic fashion. These users are likely to see
-ENOSPC errors after the counter wraps one or more times however.
This patchset adds a new idr_alloc_cyclic routine and converts several
of these users to it. Many of these users are in obscure parts of the
kernel, and I don't have a good way to test some of them. The change is
pretty straightforward though, so hopefully it won't be an issue.
There is one other cyclic user of idr_alloc that I didn't touch in
ipc/util.c. That one is doing some strange stuff that I didn't quite
understand, but it looks like it should probably be converted later
somehow.
Jeff Layton (6):
idr: introduce idr_alloc_cyclic
amso1100: convert to using idr_alloc_cyclic
mlx4: convert to using idr_alloc_cyclic
nfsd: convert nfs4_alloc_stid to use idr_alloc_cyclic
inotify: convert inotify_add_to_idr to use idr_alloc_cyclic
sctp: convert sctp_assoc_set_id to use idr_alloc_cyclic
drivers/infiniband/hw/amso1100/c2.h | 1 -
drivers/infiniband/hw/amso1100/c2_qp.c | 3 +--
drivers/infiniband/hw/mlx4/cm.c | 4 +---
fs/nfsd/nfs4state.c | 7 +------
fs/notify/inotify/inotify_user.c | 8 ++------
include/linux/fsnotify_backend.h | 1 -
include/linux/idr.h | 2 ++
lib/idr.c | 27 +++++++++++++++++++++++++++
net/sctp/associola.c | 16 ++--------------
9 files changed, 36 insertions(+), 33 deletions(-)
--
1.7.11.7
Signed-off-by: Jeff Layton <[email protected]>
Cc: John McCutchan <[email protected]>
Cc: Robert Love <[email protected]>
Cc: Eric Paris <[email protected]>
---
fs/notify/inotify/inotify_user.c | 8 ++------
include/linux/fsnotify_backend.h | 1 -
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index e0f7c12..8562bd3 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -359,7 +359,6 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns
}
static int inotify_add_to_idr(struct idr *idr, spinlock_t *idr_lock,
- int *last_wd,
struct inotify_inode_mark *i_mark)
{
int ret;
@@ -367,11 +366,10 @@ static int inotify_add_to_idr(struct idr *idr, spinlock_t *idr_lock,
idr_preload(GFP_KERNEL);
spin_lock(idr_lock);
- ret = idr_alloc(idr, i_mark, *last_wd + 1, 0, GFP_NOWAIT);
+ ret = idr_alloc_cyclic(idr, i_mark, 1, 0, GFP_NOWAIT);
if (ret >= 0) {
/* we added the mark to the idr, take a reference */
i_mark->wd = ret;
- *last_wd = i_mark->wd;
fsnotify_get_mark(&i_mark->fsn_mark);
}
@@ -638,8 +636,7 @@ static int inotify_new_watch(struct fsnotify_group *group,
if (atomic_read(&group->inotify_data.user->inotify_watches) >= inotify_max_user_watches)
goto out_err;
- ret = inotify_add_to_idr(idr, idr_lock, &group->inotify_data.last_wd,
- tmp_i_mark);
+ ret = inotify_add_to_idr(idr, idr_lock, tmp_i_mark);
if (ret)
goto out_err;
@@ -697,7 +694,6 @@ static struct fsnotify_group *inotify_new_group(unsigned int max_events)
spin_lock_init(&group->inotify_data.idr_lock);
idr_init(&group->inotify_data.idr);
- group->inotify_data.last_wd = 0;
group->inotify_data.user = get_current_user();
if (atomic_inc_return(&group->inotify_data.user->inotify_devs) >
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index d5b0910..4b2ee8d 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -157,7 +157,6 @@ struct fsnotify_group {
struct inotify_group_private_data {
spinlock_t idr_lock;
struct idr idr;
- u32 last_wd;
struct user_struct *user;
} inotify_data;
#endif
--
1.7.11.7
Signed-off-by: Jeff Layton <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: [email protected]
---
fs/nfsd/nfs4state.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e27430..417c848 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -234,7 +234,6 @@ static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct
kmem_cache *slab)
{
struct idr *stateids = &cl->cl_stateids;
- static int min_stateid = 0;
struct nfs4_stid *stid;
int new_id;
@@ -242,7 +241,7 @@ kmem_cache *slab)
if (!stid)
return NULL;
- new_id = idr_alloc(stateids, stid, min_stateid, 0, GFP_KERNEL);
+ new_id = idr_alloc_cyclic(stateids, stid, 0, 0, GFP_KERNEL);
if (new_id < 0)
goto out_free;
stid->sc_client = cl;
@@ -261,10 +260,6 @@ kmem_cache *slab)
* amount of time until an id is reused, by ensuring they always
* "increase" (mod INT_MAX):
*/
-
- min_stateid = new_id+1;
- if (min_stateid == INT_MAX)
- min_stateid = 0;
return stid;
out_free:
kfree(stid);
--
1.7.11.7
(Note: compile-tested only)
Signed-off-by: Jeff Layton <[email protected]>
Cc: Vlad Yasevich <[email protected]>
Cc: Sridhar Samudrala <[email protected]>
Cc: Neil Horman <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
net/sctp/associola.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index d2709e2..fa261a3 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -66,13 +66,6 @@ static void sctp_assoc_bh_rcv(struct work_struct *work);
static void sctp_assoc_free_asconf_acks(struct sctp_association *asoc);
static void sctp_assoc_free_asconf_queue(struct sctp_association *asoc);
-/* Keep track of the new idr low so that we don't re-use association id
- * numbers too fast. It is protected by they idr spin lock is in the
- * range of 1 - INT_MAX.
- */
-static u32 idr_low = 1;
-
-
/* 1st Level Abstractions. */
/* Initialize a new association from provided memory. */
@@ -1601,13 +1594,8 @@ int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
if (preload)
idr_preload(gfp);
spin_lock_bh(&sctp_assocs_id_lock);
- /* 0 is not a valid id, idr_low is always >= 1 */
- ret = idr_alloc(&sctp_assocs_id, asoc, idr_low, 0, GFP_NOWAIT);
- if (ret >= 0) {
- idr_low = ret + 1;
- if (idr_low == INT_MAX)
- idr_low = 1;
- }
+ /* 0 is not a valid assoc_id, must be >= 1 */
+ ret = idr_alloc_cyclic(&sctp_assocs_id, asoc, 1, 0, GFP_NOWAIT);
spin_unlock_bh(&sctp_assocs_id_lock);
if (preload)
idr_preload_end();
--
1.7.11.7
Thus spake Tejun Heo:
Ooh, BTW, the cyclic allocation is broken. It's prone to -ENOSPC
after the first wraparound. There are several cyclic users in the
kernel and I think it probably would be best to implement cyclic
support in idr.
This patch does that by adding new idr_alloc_cyclic function that such
users in the kernel can use. With this, there's no need for a caller to
keep track of the last value used as that's now tracked internally.
This should prevent the ENOSPC problems that can hit when the "last
allocated" counter exceeds INT_MAX.
Later patches will convert existing cyclic users to the new interface.
Reviewed-by: Tejun Heo <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
include/linux/idr.h | 2 ++
lib/idr.c | 27 +++++++++++++++++++++++++++
2 files changed, 29 insertions(+)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 2640c7e..a470ac3 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -42,6 +42,7 @@ struct idr {
struct idr_layer *id_free;
int layers; /* only valid w/o concurrent changes */
int id_free_cnt;
+ int cur; /* current pos for cyclic allocation */
spinlock_t lock;
};
@@ -75,6 +76,7 @@ struct idr {
void *idr_find_slowpath(struct idr *idp, int id);
void idr_preload(gfp_t gfp_mask);
int idr_alloc(struct idr *idp, void *ptr, int start, int end, gfp_t gfp_mask);
+int idr_alloc_cyclic(struct idr *idr, void *ptr, int start, int end, gfp_t gfp_mask);
int idr_for_each(struct idr *idp,
int (*fn)(int id, void *p, void *data), void *data);
void *idr_get_next(struct idr *idp, int *nextid);
diff --git a/lib/idr.c b/lib/idr.c
index 322e281..cca4b93 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -495,6 +495,33 @@ int idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp_mask)
}
EXPORT_SYMBOL_GPL(idr_alloc);
+/**
+ * idr_alloc_cyclic - allocate new idr entry in a cyclical fashion
+ * @idr: the (initialized) idr
+ * @ptr: pointer to be associated with the new id
+ * @start: the minimum id (inclusive)
+ * @end: the maximum id (exclusive, <= 0 for max)
+ * @gfp_mask: memory allocation flags
+ *
+ * Essentially the same as idr_alloc, but prefers to allocate progressively
+ * higher ids if it can. If the "cur" counter wraps, then it will start again
+ * at the "start" end of the range and allocate one that has already been used.
+ */
+int idr_alloc_cyclic(struct idr *idr, void *ptr, int start, int end,
+ gfp_t gfp_mask)
+{
+ int id;
+
+ id = idr_alloc(idr, ptr, max(start, idr->cur), end, gfp_mask);
+ if (id == -ENOSPC)
+ id = idr_alloc(idr, ptr, start, end, gfp_mask);
+
+ if (likely(id >= 0))
+ idr->cur = id + 1;
+ return id;
+}
+EXPORT_SYMBOL(idr_alloc_cyclic);
+
static void idr_remove_warning(int id)
{
printk(KERN_WARNING
--
1.7.11.7
(Note: compile tested only)
Signed-off-by: Jeff Layton <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Jack Morgenstein <[email protected]>
Cc: Or Gerlitz <[email protected]>
Cc: Roland Dreier <[email protected]>
Cc: [email protected]
---
drivers/infiniband/hw/mlx4/cm.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/mlx4/cm.c b/drivers/infiniband/hw/mlx4/cm.c
index add98d0..d1f5f1d 100644
--- a/drivers/infiniband/hw/mlx4/cm.c
+++ b/drivers/infiniband/hw/mlx4/cm.c
@@ -204,7 +204,6 @@ static struct id_map_entry *
id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id)
{
int ret;
- static int next_id;
struct id_map_entry *ent;
struct mlx4_ib_sriov *sriov = &to_mdev(ibdev)->sriov;
@@ -223,9 +222,8 @@ id_map_alloc(struct ib_device *ibdev, int slave_id, u32 sl_cm_id)
idr_preload(GFP_KERNEL);
spin_lock(&to_mdev(ibdev)->sriov.id_map_lock);
- ret = idr_alloc(&sriov->pv_id_table, ent, next_id, 0, GFP_NOWAIT);
+ ret = idr_alloc_cyclic(&sriov->pv_id_table, ent, 0, 0, GFP_NOWAIT);
if (ret >= 0) {
- next_id = max(ret + 1, 0);
ent->pv_cm_id = (u32)ret;
sl_id_map_add(ibdev, ent);
list_add_tail(&ent->list, &sriov->cm_list);
--
1.7.11.7
(Note: compile-tested only)
Signed-off-by: Jeff Layton <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: Steve Wise <[email protected]>
Cc: Tom Tucker <[email protected]>
Cc: [email protected]
---
drivers/infiniband/hw/amso1100/c2.h | 1 -
drivers/infiniband/hw/amso1100/c2_qp.c | 3 +--
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/amso1100/c2.h b/drivers/infiniband/hw/amso1100/c2.h
index ba7a1208..d619d73 100644
--- a/drivers/infiniband/hw/amso1100/c2.h
+++ b/drivers/infiniband/hw/amso1100/c2.h
@@ -265,7 +265,6 @@ struct c2_pd_table {
struct c2_qp_table {
struct idr idr;
spinlock_t lock;
- int last;
};
struct c2_element {
diff --git a/drivers/infiniband/hw/amso1100/c2_qp.c b/drivers/infiniband/hw/amso1100/c2_qp.c
index 0ab826b..86708de 100644
--- a/drivers/infiniband/hw/amso1100/c2_qp.c
+++ b/drivers/infiniband/hw/amso1100/c2_qp.c
@@ -385,8 +385,7 @@ static int c2_alloc_qpn(struct c2_dev *c2dev, struct c2_qp *qp)
idr_preload(GFP_KERNEL);
spin_lock_irq(&c2dev->qp_table.lock);
- ret = idr_alloc(&c2dev->qp_table.idr, qp, c2dev->qp_table.last++, 0,
- GFP_NOWAIT);
+ ret = idr_alloc_cyclic(&c2dev->qp_table.idr, qp, 0, 0, GFP_NOWAIT);
if (ret >= 0)
qp->qpn = ret;
--
1.7.11.7
On Wed, Mar 27, 2013 at 03:29:34PM -0400, Jeff Layton wrote:
> (Note: compile-tested only)
>
> Signed-off-by: Jeff Layton <[email protected]>
> Cc: Tejun Heo <[email protected]>
> Cc: Steve Wise <[email protected]>
> Cc: Tom Tucker <[email protected]>
> Cc: [email protected]
Reviewed-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
On Wed, Mar 27, 2013 at 03:29:32PM -0400, Jeff Layton wrote:
> (Andrew, I think this is probably best routed via -mm since it touches
> several different places)
Yeap, I think it'd be best to route these through -mm.
Please feel free to add Reviewed-by: Tejun Heo <[email protected]> for the
whole series.
Thanks!
--
tejun
On Wed, 27 Mar 2013 12:38:47 -0700 Tejun Heo <[email protected]> wrote:
> On Wed, Mar 27, 2013 at 03:29:32PM -0400, Jeff Layton wrote:
> > (Andrew, I think this is probably best routed via -mm since it touches
> > several different places)
>
> Yeap, I think it'd be best to route these through -mm.
>
> Please feel free to add Reviewed-by: Tejun Heo <[email protected]> for the
> whole series.
I understand that Jeff will be sending a v2 series?
btw, I'll be away-from-keyboard until the end of next week. Nobody be
alarmed ;)
On Wed, 27 Mar 2013 12:50:53 -0700
Andrew Morton <[email protected]> wrote:
> On Wed, 27 Mar 2013 12:38:47 -0700 Tejun Heo <[email protected]> wrote:
>
> > On Wed, Mar 27, 2013 at 03:29:32PM -0400, Jeff Layton wrote:
> > > (Andrew, I think this is probably best routed via -mm since it touches
> > > several different places)
> >
> > Yeap, I think it'd be best to route these through -mm.
> >
> > Please feel free to add Reviewed-by: Tejun Heo <[email protected]> for the
> > whole series.
>
> I understand that Jeff will be sending a v2 series?
>
Yes, just sent a few mins ago...
> btw, I'll be away-from-keyboard until the end of next week. Nobody be
> alarmed ;)
>
Thanks!
--
Jeff Layton <[email protected]>
On Wed, Mar 27, 2013 at 03:29:38PM -0400, Jeff Layton wrote:
> (Note: compile-tested only)
>
> Signed-off-by: Jeff Layton <[email protected]>
> Cc: Vlad Yasevich <[email protected]>
> Cc: Sridhar Samudrala <[email protected]>
> Cc: Neil Horman <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> net/sctp/associola.c | 16 ++--------------
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
I don't see anything wrong with this patch per-se, but the idr_alloc_cyclic call
isn't integrated with net/net-next or Linus' tree yet. If we don't gate this
patch on that integration, we'll break the build.
Neil
On Thu, Mar 28, 2013 at 09:53:08AM -0400, Neil Horman wrote:
> On Wed, Mar 27, 2013 at 03:29:38PM -0400, Jeff Layton wrote:
> > (Note: compile-tested only)
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > Cc: Vlad Yasevich <[email protected]>
> > Cc: Sridhar Samudrala <[email protected]>
> > Cc: Neil Horman <[email protected]>
> > Cc: "David S. Miller" <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > net/sctp/associola.c | 16 ++--------------
> > 1 file changed, 2 insertions(+), 14 deletions(-)
> >
> I don't see anything wrong with this patch per-se, but the idr_alloc_cyclic call
> isn't integrated with net/net-next or Linus' tree yet. If we don't gate this
> patch on that integration, we'll break the build.
> Neil
>
Actually, I just noticed that you only sent us 6/6 here, I'm assuming a prior
patch in the series adds the idr_alloc_cyclic code? if so, I've seen the prior
version
Acked-by: Neil Horman <[email protected]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, 28 Mar 2013 10:04:06 -0400
Neil Horman <[email protected]> wrote:
> On Thu, Mar 28, 2013 at 09:53:08AM -0400, Neil Horman wrote:
> > On Wed, Mar 27, 2013 at 03:29:38PM -0400, Jeff Layton wrote:
> > > (Note: compile-tested only)
> > >
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > Cc: Vlad Yasevich <[email protected]>
> > > Cc: Sridhar Samudrala <[email protected]>
> > > Cc: Neil Horman <[email protected]>
> > > Cc: "David S. Miller" <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > ---
> > > net/sctp/associola.c | 16 ++--------------
> > > 1 file changed, 2 insertions(+), 14 deletions(-)
> > >
> > I don't see anything wrong with this patch per-se, but the idr_alloc_cyclic call
> > isn't integrated with net/net-next or Linus' tree yet. If we don't gate this
> > patch on that integration, we'll break the build.
> > Neil
> >
> Actually, I just noticed that you only sent us 6/6 here, I'm assuming a prior
> patch in the series adds the idr_alloc_cyclic code? if so, I've seen the prior
> version
Yes, idr_alloc_cyclic is added in patch #1. v2 has some slight changes
from v1 but it's essentially the same concept.
>
> Acked-by: Neil Horman <[email protected]>
>
Thanks!
--
Jeff Layton <[email protected]>
Err, and ack v2.--b.
On Wed, Mar 27, 2013 at 03:29:36PM -0400, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: [email protected]
> ---
> fs/nfsd/nfs4state.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2e27430..417c848 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -234,7 +234,6 @@ static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct
> kmem_cache *slab)
> {
> struct idr *stateids = &cl->cl_stateids;
> - static int min_stateid = 0;
> struct nfs4_stid *stid;
> int new_id;
>
> @@ -242,7 +241,7 @@ kmem_cache *slab)
> if (!stid)
> return NULL;
>
> - new_id = idr_alloc(stateids, stid, min_stateid, 0, GFP_KERNEL);
> + new_id = idr_alloc_cyclic(stateids, stid, 0, 0, GFP_KERNEL);
> if (new_id < 0)
> goto out_free;
> stid->sc_client = cl;
> @@ -261,10 +260,6 @@ kmem_cache *slab)
> * amount of time until an id is reused, by ensuring they always
> * "increase" (mod INT_MAX):
> */
> -
> - min_stateid = new_id+1;
> - if (min_stateid == INT_MAX)
> - min_stateid = 0;
> return stid;
> out_free:
> kfree(stid);
> --
> 1.7.11.7
>