Page MenuHomeFreeBSD

iflib: properly release memory allocated for DMA
ClosedPublic

Authored by jacob.e.keller_intel.com on Thu, Oct 31, 12:27 AM.

Details

Summary

DMA memory allocations using the bus_dma.h interface are not properly
released in all cases for both Tx and Rx. This causes ~448 bytes of
M_DEVBUF allocations to be leaked.

First, the DMA maps for Rx are not properly destroyed. A slight attempt
is made in iflib_fl_bufs_free to destroy the maps if we're detaching.
However, this function may not be reliably called during detach. Indeed,
there is a comment "asking" if this should be moved out.

Fix this by moving the bus_dmamap_destroy call into iflib_rx_sds_free,
where we already sync and unload the DMA.

Second, the DMA tag associated with the ifr_ifdi descriptor DMA is not
released properly anywhere. Add a call to iflib_dma_free in
iflib_rx_structures_free.

Third, use of NULL as a canary value on the map pointer returned by
bus_dmamap_create is not valid. On some platforms, notably x86, this
value may be NULL. In this case, we fail to properly release the related
resources.

Remove the NULL checks on map values in both iflib_fl_bufs_free and
iflib_txsd_destroy.

With all of these fixes applied, the leaks to M_DEVBUF are squelched,
and iflib drivers now seem to properly cleanup when detaching.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

The fact that the map pointer can be NULL is weird, but we found this to be true in several places, and without the removal of the NULL checks we end up leaking ~200 bytes of M_DEVBUF, and associated other DMA memory allocated outside of malloc.

Even with this patch (and the previous M_IFLIB fixes) I still see a small amount of memory leaked that doesn't appear to come from malloc-based allocations. I'm still root causing that.

gallatin accepted this revision.Thu, Oct 31, 1:05 PM

Thanks for fixing this. The NULL canaries were left over from when iflib had a separate code path which avoided busdma. When I removed this codepath, I neglected to remove those canaries.

Thanks for fixing this. The NULL canaries were left over from when iflib had a separate code path which avoided busdma. When I removed this codepath, I neglected to remove those canaries.

Yep. Still got a few more things to work out. two M_TEMP allocations and one M_IFADDR allocation is being leaked as well, still.

erj added a comment.Mon, Nov 4, 9:45 PM

@jacob.e.keller_intel.com, is this ready to be committed?

In D22203#486049, @erj wrote:

@jacob.e.keller_intel.com, is this ready to be committed?

Yep!

erj accepted this revision.Mon, Nov 4, 11:04 PM
This revision is now accepted and ready to land.Mon, Nov 4, 11:04 PM
This revision was automatically updated to reflect the committed changes.