| Message ID | 20190221002107.22625-9-willy@infradead.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Convert the Infiniband subsystem to XArray | expand |
On Wed, Feb 20, 2019 at 04:20:43PM -0800, Matthew Wilcox wrote: > Also introduce cm_local_id() to reduce the amount of boilerplate when > converting a local ID to an XArray index. > > Signed-off-by: Matthew Wilcox <willy@infradead.org> > drivers/infiniband/core/cm.c | 40 +++++++++++++++--------------------- > 1 file changed, 17 insertions(+), 23 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index 37980c7564c0..b97592b44ce2 100644 > +++ b/drivers/infiniband/core/cm.c > @@ -124,7 +124,8 @@ static struct ib_cm { > struct rb_root remote_qp_table; > struct rb_root remote_id_table; > struct rb_root remote_sidr_table; > - struct idr local_id_table; > + struct xarray local_id_table; > + u32 local_id_next; > __be32 random_id_operand; > struct list_head timewait_list; > struct workqueue_struct *wq; > @@ -598,35 +599,31 @@ static int cm_init_av_by_path(struct sa_path_rec *path, > > static int cm_alloc_id(struct cm_id_private *cm_id_priv) > { > - unsigned long flags; > - int id; > - > - idr_preload(GFP_KERNEL); > - spin_lock_irqsave(&cm.lock, flags); > + int err; > + u32 id; > > - id = idr_alloc_cyclic(&cm.local_id_table, cm_id_priv, 0, 0, GFP_NOWAIT); > - > - spin_unlock_irqrestore(&cm.lock, flags); > - idr_preload_end(); > + err = xa_alloc_cyclic_irq(&cm.local_id_table, &id, cm_id_priv, > + xa_limit_32b, &cm.local_id_next, GFP_KERNEL); > > cm_id_priv->id.local_id = (__force __be32)id ^ cm.random_id_operand; > - return id < 0 ? id : 0; > + return err; > +} > + > +static u32 cm_local_id(__be32 local_id) > +{ > + return (__force u32) (local_id ^ cm.random_id_operand); > } > > static void cm_free_id(__be32 local_id) > { > - spin_lock_irq(&cm.lock); > - idr_remove(&cm.local_id_table, > - (__force int) (local_id ^ cm.random_id_operand)); > - spin_unlock_irq(&cm.lock); > + xa_erase_irq(&cm.local_id_table, cm_local_id(local_id)); > } > > static struct cm_id_private * cm_get_id(__be32 local_id, __be32 remote_id) > { > struct cm_id_private *cm_id_priv; > > - cm_id_priv = idr_find(&cm.local_id_table, > - (__force int) (local_id ^ cm.random_id_operand)); > + cm_id_priv = xa_load(&cm.local_id_table, cm_local_id(local_id)); > if (cm_id_priv) { > if (cm_id_priv->id.remote_id == remote_id) > atomic_inc(&cm_id_priv->refcount); > @@ -2824,9 +2821,8 @@ static struct cm_id_private * cm_acquire_rejected_id(struct cm_rej_msg *rej_msg) > spin_unlock_irq(&cm.lock); > return NULL; > } > - cm_id_priv = idr_find(&cm.local_id_table, (__force int) > - (timewait_info->work.local_id ^ > - cm.random_id_operand)); > + cm_id_priv = xa_load(&cm.local_id_table, > + cm_local_id(timewait_info->work.local_id)); > if (cm_id_priv) { > if (cm_id_priv->id.remote_id == remote_id) > atomic_inc(&cm_id_priv->refcount); > @@ -4513,7 +4509,7 @@ static int __init ib_cm_init(void) > cm.remote_id_table = RB_ROOT; > cm.remote_qp_table = RB_ROOT; > cm.remote_sidr_table = RB_ROOT; > - idr_init(&cm.local_id_table); > + xa_init_flags(&cm.local_id_table, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ); > get_random_bytes(&cm.random_id_operand, sizeof cm.random_id_operand); > INIT_LIST_HEAD(&cm.timewait_list); > > @@ -4539,7 +4535,6 @@ static int __init ib_cm_init(void) > error2: > class_unregister(&cm_class); > error1: > - idr_destroy(&cm.local_id_table); > return ret; > } > > @@ -4561,7 +4556,6 @@ static void __exit ib_cm_cleanup(void) > } > > class_unregister(&cm_class); > - idr_destroy(&cm.local_id_table); Why not do xa_destroy()? BTW, I kind of feel like we should have a xa_empty_destroy(xa) which does a if (WARN_ON(!xa_empty())) xa_destroy(xa) Since cases like this are leaking memory bugs to unload the module with anything in the xarray. Jason
On Thu, Feb 21, 2019 at 11:23:04AM -0700, Jason Gunthorpe wrote: > On Wed, Feb 20, 2019 at 04:20:43PM -0800, Matthew Wilcox wrote: > > @@ -4561,7 +4556,6 @@ static void __exit ib_cm_cleanup(void) > > } > > > > class_unregister(&cm_class); > > - idr_destroy(&cm.local_id_table); > > Why not do xa_destroy()? > > BTW, I kind of feel like we should have a > > xa_empty_destroy(xa) > > which does a > > if (WARN_ON(!xa_empty())) > xa_destroy(xa) > > Since cases like this are leaking memory bugs to unload the module > with anything in the xarray. Back In The Day (2016), you had to call idr_destroy() when you were going to free the data structure holding an IDR, or when unloading a module with a static IDR in it. I did a quick estimate, and I'd say about a third of IDR users didn't. That was definitely a memory leak because it wouldn't free any of the preallocated idr_layers which were held in the tree. Once I converted the IDR to use the radix tree, preallocated radix_tree_nodes were kept per-cpu and not per-IDR, so the idr_destroy() became entirely optional if it was empty. We see a few WARN/BUG_ON(idr_is_empty()) calls dotted around the kernel as heritage from these days, but mostly they just stayed as unnecessary calls to idr_destroy(). For the vast majority of users, these are no-ops. Because if you're tearing down an IDR/XArray, if it's not empty then usually it contains a pointer to something, and idr_destroy()/xa_destroy() can't take care of it. There's no generic free() function we can call. Most IDRs/XArrays contain the only reference to the pointer. If it's not empty and we destroy it, we're losing the only pointer to that memory. So I chose to delete most of the calls to idr_destroy rather than convert them to either an xa_empty assertion or a call to xa_destroy. Yes, if there's a bug in the module, we're going to leak an xa_node. But we were already leaking a pointer to something else. We have CONFIG_DEBUG_KMEMLEAK to help us find these things; we don't need individual drivers to be doing it too.
On Thu, Feb 21, 2019 at 10:36:58AM -0800, Matthew Wilcox wrote: > On Thu, Feb 21, 2019 at 11:23:04AM -0700, Jason Gunthorpe wrote: > > On Wed, Feb 20, 2019 at 04:20:43PM -0800, Matthew Wilcox wrote: > > > @@ -4561,7 +4556,6 @@ static void __exit ib_cm_cleanup(void) > > > } > > > > > > class_unregister(&cm_class); > > > - idr_destroy(&cm.local_id_table); > > > > Why not do xa_destroy()? > > > > BTW, I kind of feel like we should have a > > > > xa_empty_destroy(xa) > > > > which does a > > > > if (WARN_ON(!xa_empty())) > > xa_destroy(xa) > > > > Since cases like this are leaking memory bugs to unload the module > > with anything in the xarray. > > Back In The Day (2016), you had to call idr_destroy() when you were going > to free the data structure holding an IDR, or when unloading a module with > a static IDR in it. I did a quick estimate, and I'd say about a third of > IDR users didn't. That was definitely a memory leak because it wouldn't > free any of the preallocated idr_layers which were held in the tree. > > Once I converted the IDR to use the radix tree, preallocated > radix_tree_nodes were kept per-cpu and not per-IDR, so the > idr_destroy() became entirely optional if it was empty. We see a few > WARN/BUG_ON(idr_is_empty()) calls dotted around the kernel as heritage > from these days, but mostly they just stayed as unnecessary calls to > idr_destroy(). > > For the vast majority of users, these are no-ops. Because if you're > tearing down an IDR/XArray, if it's not empty then usually it contains > a pointer to something, and idr_destroy()/xa_destroy() can't take care > of it. There's no generic free() function we can call. Most IDRs/XArrays > contain the only reference to the pointer. If it's not empty and we > destroy it, we're losing the only pointer to that memory. > > So I chose to delete most of the calls to idr_destroy rather than > convert them to either an xa_empty assertion or a call to xa_destroy. > Yes, if there's a bug in the module, we're going to leak an xa_node. > But we were already leaking a pointer to something else. We have > CONFIG_DEBUG_KMEMLEAK to help us find these things; we don't need > individual drivers to be doing it too. I disagree that CONFIG_DEBUG_KMEMLEAK is a replacement for a WARN_ON, it is very imprecise and difficult to use feature. IHMO all the idr_destroys should be WARN_ON checks because, as you say, it is an unconditional bug to pass that point with pointers still in the IDR. It is clearly a bug in the module, why shouldn't we detect it when it is so cheap to do so? Jason
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index 37980c7564c0..b97592b44ce2 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -124,7 +124,8 @@ static struct ib_cm { struct rb_root remote_qp_table; struct rb_root remote_id_table; struct rb_root remote_sidr_table; - struct idr local_id_table; + struct xarray local_id_table; + u32 local_id_next; __be32 random_id_operand; struct list_head timewait_list; struct workqueue_struct *wq; @@ -598,35 +599,31 @@ static int cm_init_av_by_path(struct sa_path_rec *path, static int cm_alloc_id(struct cm_id_private *cm_id_priv) { - unsigned long flags; - int id; - - idr_preload(GFP_KERNEL); - spin_lock_irqsave(&cm.lock, flags); + int err; + u32 id; - id = idr_alloc_cyclic(&cm.local_id_table, cm_id_priv, 0, 0, GFP_NOWAIT); - - spin_unlock_irqrestore(&cm.lock, flags); - idr_preload_end(); + err = xa_alloc_cyclic_irq(&cm.local_id_table, &id, cm_id_priv, + xa_limit_32b, &cm.local_id_next, GFP_KERNEL); cm_id_priv->id.local_id = (__force __be32)id ^ cm.random_id_operand; - return id < 0 ? id : 0; + return err; +} + +static u32 cm_local_id(__be32 local_id) +{ + return (__force u32) (local_id ^ cm.random_id_operand); } static void cm_free_id(__be32 local_id) { - spin_lock_irq(&cm.lock); - idr_remove(&cm.local_id_table, - (__force int) (local_id ^ cm.random_id_operand)); - spin_unlock_irq(&cm.lock); + xa_erase_irq(&cm.local_id_table, cm_local_id(local_id)); } static struct cm_id_private * cm_get_id(__be32 local_id, __be32 remote_id) { struct cm_id_private *cm_id_priv; - cm_id_priv = idr_find(&cm.local_id_table, - (__force int) (local_id ^ cm.random_id_operand)); + cm_id_priv = xa_load(&cm.local_id_table, cm_local_id(local_id)); if (cm_id_priv) { if (cm_id_priv->id.remote_id == remote_id) atomic_inc(&cm_id_priv->refcount); @@ -2824,9 +2821,8 @@ static struct cm_id_private * cm_acquire_rejected_id(struct cm_rej_msg *rej_msg) spin_unlock_irq(&cm.lock); return NULL; } - cm_id_priv = idr_find(&cm.local_id_table, (__force int) - (timewait_info->work.local_id ^ - cm.random_id_operand)); + cm_id_priv = xa_load(&cm.local_id_table, + cm_local_id(timewait_info->work.local_id)); if (cm_id_priv) { if (cm_id_priv->id.remote_id == remote_id) atomic_inc(&cm_id_priv->refcount); @@ -4513,7 +4509,7 @@ static int __init ib_cm_init(void) cm.remote_id_table = RB_ROOT; cm.remote_qp_table = RB_ROOT; cm.remote_sidr_table = RB_ROOT; - idr_init(&cm.local_id_table); + xa_init_flags(&cm.local_id_table, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ); get_random_bytes(&cm.random_id_operand, sizeof cm.random_id_operand); INIT_LIST_HEAD(&cm.timewait_list); @@ -4539,7 +4535,6 @@ static int __init ib_cm_init(void) error2: class_unregister(&cm_class); error1: - idr_destroy(&cm.local_id_table); return ret; } @@ -4561,7 +4556,6 @@ static void __exit ib_cm_cleanup(void) } class_unregister(&cm_class); - idr_destroy(&cm.local_id_table); } module_init(ib_cm_init);
Also introduce cm_local_id() to reduce the amount of boilerplate when converting a local ID to an XArray index. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- drivers/infiniband/core/cm.c | 40 +++++++++++++++--------------------- 1 file changed, 17 insertions(+), 23 deletions(-)