On 2/11/22 13:56, Jason Gunthorpe wrote: > On Fri, Feb 11, 2022 at 01:36:06PM -0600, Bob Pearson wrote: >> On 2/11/22 12:43, Jason Gunthorpe wrote: >>> On Tue, Feb 08, 2022 at 03:16:42PM -0600, Bob Pearson wrote: >>>> Well behaved applications will free all memory allocated by multicast >>>> but programs which do not clean up properly can leave behind allocated >>>> memory when the rxe driver is unloaded. This patch walks the red-black >>>> tree holding multicast group elements and then walks the list of attached >>>> qp's freeing the mca's and finally the mcg's. >>> >>> How does this happen? the ib core ensures that all uobjects are >>> destroyed, so if something is still in the rb tree here it means that >>> an earlier uobject destruction leaked it >>> >>> Jason >> >> The mc_grp and mc_elem objects are not rdma-core uobjects. So their memory >> is allocated by the rxe driver. They get created by ib_attach_mcast and destroyed >> by ib_detach_mcast. If an application crashes without calling a matching >> ib_detach_mcast for each attachment the driver would have leaked the memory. >> This patch fixes that. > > The mcast attachment is affiliated with a QP, when all the QPs are > destroyed, which are rdma-core objects, then all attachments should be > free'd as well. That should have happened by the time things get here > > I would expect a simple WARN_ON that the rb tree is empty in destroy > to catch any bugs. > > Jason I wrote a simple user test case that calls ibv_attach_mcast() and then sleeps. If I type ^C the qp is detached as you predicted so someone is counting them somewhere. It isn't obvious in rdma-core. Not sure what you are suggesting above. The detach code already checks to see if the attachment is *not* present. The only time it is clear that there shouldn't be any more attachments present is when the device is closed which is what the cleanup code is doing. I can add a warning if there was anything there but it will likely never get called since someone above me is actively cleaning them up from failed apps. Or we can just drop this patch and assume it isn't needed. Bob
