iflib: report output drops and handle ENOBUFS properly

[gallatin]

Summary

When working on enhancements to the simple transmit routine, I noticed that iflib is not reporting back output drops & errors due to full tx rings or other misc. errors. This change:

• Fixes an mbuf leak with iflib.simple_tx=1 by freeing the mbuf chain in iflib_encap(). Note that seems odd, but iflib_encap() can consume mbufs for other errors, so it seemed simplest to just have it always consume the mbuf than try to untangle the error handling and conditionally free mbufs in iflib_simple_transmit(). This fixes an mbuf leak that would happen with the iflib.simple_tx=1 and output drops/errors.

Note that I don't understand the normal tx routine fully, and I can't tell by inspection if it also leaks, so I have left it alone (except for reporting the drop).

• Increments counters for output drops when ENOBUFS is encountered, and output errors when other transmit errors are encountered for both the simple and normal tx routines

• Augments iflib_if_get_counter() by adding iflib's observed IFCOUNTER_OQDROPS & IFCOUNTER_OERRORS to whatever the underlying driver reports. The underlying driver can't see these drops, so it makes sense to get these counters from the iflib / ifnet level. This was done as a switch in case there are similar counters to add that I'm forgetting

[gallatin]

• Call iflib_remove_mbuf() to unmap and remove the mbuf from list of mbufs to be freed when we free an mbuf due to ENOBUFS

[markj]

FreeBSD/sys/net/iflib.c

3673 ↗

(On Diff #161475)

Is the idea here that in the normal tx path, the mp_ring will buffer the packet until we're ready to try again?

4609 ↗

(On Diff #161475)

I think this will double-count drops or errors in some cases. Look at em_if_get_counter() for instance, for OQDROPS it'll return if_get_counter_default(ifp, IFCOUNTER_OQDROPS);. ixgbe_if_get_counter() on the other hand returns 0 for OQDROPS. So, I think you'll have to go through the implementations and make the consistent.

4610 ↗

(On Diff #161475)

These four lines should be de-indented by one tab.

[gallatin]

Updated drivers that were getting generic counters using if_get_counter_default() as part of their ifdi_get_counter method to avoid getting generic counters for output drops and output errors, as suggested by @markj .

FreeBSD/sys/net/iflib.c

3673 ↗

(On Diff #161475)

Yes, this is what I was saying here: "I don't understand the normal tx routine fully, and I can't tell by inspection if it also leaks, so I have left it alone "

And looking at iflib_txq_drain(), it returns skipped + pkts_sent. Where skipped is incremented for non-ENOBUFS errors. Eg, an ENOBUFS error results in not advancing the return value. And its caller, drain_ring_lock[ed|less]() in mp_ring.c increments its index only by the return value from iflib_txq_drain(). So the mbuf is retained in the ring, and we don't want to free it for the mp_ring based transmit.

For the simple transmit, we want to consume it the same as with any error.

4609 ↗

(On Diff #161475)

Ick. I think you're right.

[gallatin]

FreeBSD/sys/net/iflib.c

4609 ↗

(On Diff #161475)

I've audited the drivers and updated the patch to make them avoid using the generic counters for odrops and output errs

[markj]

Instead of making each driver report 0 if it doesn't collect a drop counter from the MAC, and treating these counters as a special case in iflib_if_get_counter(), wouldn't it be simpler and cleaner to make them all drivers return the value from if_get_counter_default()?

[gallatin]

reversed logic and made the driver responsible to query the generic counters for odrop/oerr rather than iflib.

[kbowling]

FreeBSD/sys/net/iflib.c

4586 ↗

(On Diff #161739)

nit: it seems we already have a \n here, no need for two

[gallatin]

In D52369#1197238, @markj wrote:

Instead of making each driver report 0 if it doesn't collect a drop counter from the MAC, and treating these counters as a special case in iflib_if_get_counter(), wouldn't it be simpler and cleaner to make them all drivers return the value from if_get_counter_default()?

Its a 6-in-one/half-dozen the other kind of thing. Doing it this way makes some drivers that have local oerr/odrop sources to report a bit more complex, but makes others that don't simpler. It is nice because it makes drivers entirely responsible for what gets reported. On the flip-side, it makes them responsible to report counters collected by a different layer. So there are arguments either way. I don't care which way we go. I've tested the updated version on an out-of-tree driver that reports its own errors, and with ixl and confirmed odrops look reasonable. And this is how its sitting in my tree now, so I'd prefer to go with this version.

BTW, sorry if you're diffing between revisions.. I also rebased my working tree...

[gallatin]

• remove extra space in iflib_if_get_counter as pointed out by @kbowling