netmap: Address errors on memory free in netmap_generic

[thj]

Summary

netmap_generic keeps a pool of mbufs for uses in handling transfers,
these mbufs have an external buffer attached to them.

If some cases other parts of the network stack can chain these mbufs,
when this happens the normal pool destructor function can end up
free'ing the pool mbufs twice:

• A first time if a pool mbuf has been chained with another mbuf when its chain is freed
• A second time when its entry in the pool is freed

Additionally if other parts of the stack demote a pool mbuf its
interface reference will be cleared. In this case we deference a NULL
pointer when trying to free the mbuf through the destructor. Store a
reference to the adapter in ext_arg1 with the destructor callback so we
can find the correct adapter when free'ing a pool mbuf.

This change enables using netmap with epair interfaces.

Test Plan

I've tested this using tools/tools/netmap/bridge and vpp with a netmap adapter.
I put bulk traffic through the interface to exercise the memory pooling and
requests per second traffic to try and catch any timing issues that might occur
with well paced TCP.

I've run 2TB with iperf3 and wrk doing 20,000/rps for 12 hours. I didn't
encounter mbuf exhaustion or obvious leaks running these tests.

[markj]

sys/dev/netmap/netmap_generic.c

439

There's still a narrow race here: during teardown of the netmap interface, a thread may have started calling the destructor before generic_netmap_unregister() sets it to NULL. Or is there something which prevents this?

[thj]

• - if the reference has gone after taking the lock don't free the mbuf

[thj]

sys/dev/netmap/netmap_generic.c

439

I agree there is a race. I've updated but I think I need to also rearrange the free'ing path a little too.

[thj]

• - Don't remove the pool lock until we have removed all mbufs

[vmaffione]

sys/dev/netmap/netmap_generic.c

260

Can you add this to the SET_MBUF_DESCTRUCTOR macro, to make the code compile also under linux?

658

Same as above, this should go into the SET_MBUF_DESTRUCTOR macro

[thj]

• - use SET_MBUF_DESTRUCTOR to set ext_arg1

[vmaffione]

Any additional tests with pkt-gen in TX or RX mode over iflib interfaces and virtio-net ones?
E.g. two VMs/machines connected back to back with one transmitting and the other receiving, to stress test it?
Setting the sys.dev.netmap.admode sysctl to 2 will force emulated netmap even if the interface has native support.

sys/dev/netmap/netmap_generic.c

439

Same here... We should define a GEN_TX_MBUF_NA() macro to hide the internals. Better names welcome

[thj]

• Add a macro for getting netmap adapter from ext_arg1

[thj]

In D44371#1013215, @vmaffione wrote:

Any additional tests with pkt-gen in TX or RX mode over iflib interfaces and virtio-net ones?
E.g. two VMs/machines connected back to back with one transmitting and the other receiving, to stress test it?
Setting the sys.dev.netmap.admode sysctl to 2 will force emulated netmap even if the interface has native support.

I think tests are a good idea, one barrier to freebsd test suite tests so far has been that epair didn't work. I think we can now write tests to exercise stability of netmap

[vmaffione]

sys/dev/netmap/netmap_generic.c

259

This call sets the two ext fields to NULL, so we should be explicit and pass NULL to both arguments.
(See the other comment below).

682

What about the m_freem call in generic_netmap_tx_clean? Not used by freebsd, but change that for consistency?

sys/dev/netmap/netmap_kern.h

2403

I think it is more readable if we are explicit here: set ext_arg1 unconditionally to na and pass NULL when removing the notification event.
Also, since the name of this macro mentions the destructor, it looks better to have the fn as a first argument and na as the second.

[thj]

• swap order of arguments for destructor
• replace an m_freem with m_free

[thj]

updated

[vmaffione]

Last modification.
Thanks

sys/dev/netmap/netmap_generic.c

471

Access ext_arg1 using the GEN_TX_MBUF_NA macro.

[thj]

• access ext_arg1 via the accessor

[vmaffione]

Thanks!