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.
