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
