Page MenuHomeFreeBSD

factor out remove logic from if_delgroup and if_delgroups
AbandonedPublic

Authored by jacob.e.keller_intel.com on Nov 1 2019, 5:23 PM.

Details

Reviewers
jhb
gallatin
erj
Group Reviewers
Intel Networking
Summary

The if_delgroups function failed to free the ifg_list structure pointer
memory. This results in a small leak on M_TEMP for each ifp group every
time an interface is detached.

This was caused because if_delgroup and if_delgroups duplicate the logic
for releasing the group memory.

Fix this by factoring out an if_freegroup function which can be called
by both if_delgroup and if_delgroups.

This results in slightly funky code, because we must drop the
IFNET_WLOCK before calling the event handler. However, we want the
IFNET_WLOCK around the group list search in if_delgroup and in the group
list iteration in if_delgroups. Further, if_delgroups must re-take the
lock after calling the new if_freegroups.

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

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 27309
Build 25567: arc lint + arc unit

Event Timeline

This is likely the better approach to solving the leak described in https://reviews.freebsd.org/D22215

I don't know 100% though, because the fact that the function releases the lock smells weird and like it could be confusing to look at in the future. That's why I posted both potential fixes.

I think the refactor is fine.

sys/net/if.c
1503

Better to add an IFNET_WLOCK_ASSERT() or some such than to just document it in a comment.

This revision is now accepted and ready to land.Nov 1 2019, 6:08 PM

add an IFNET_WLOCK_ASSERT in if_freegroup

This revision now requires review to proceed.Nov 1 2019, 6:15 PM
erj requested changes to this revision.Jan 3 2020, 5:53 PM

Can this get regenerated? It no longer applies cleanly.

This revision now requires changes to proceed.Jan 3 2020, 5:53 PM

It looks like similar work was done via r355942, with slightly different (better) function names, and various style changes. This can be closed as essentially resolved/committed now.