Page MenuHomeFreeBSD

iflib sysctl cleanup
ClosedPublic

Authored by shurd on Aug 15 2018, 8:24 PM.
Tags
None
Referenced Files
F103165845: D16733.id47124.diff
Thu, Nov 21, 7:25 PM
F103165829: D16733.id46737.diff
Thu, Nov 21, 7:25 PM
F103165824: D16733.id.diff
Thu, Nov 21, 7:25 PM
F103165811: D16733.id47767.diff
Thu, Nov 21, 7:25 PM
F103165806: D16733.id46736.diff
Thu, Nov 21, 7:25 PM
F103164532: D16733.diff
Thu, Nov 21, 7:04 PM
Unknown Object (File)
Sat, Nov 16, 12:23 PM
Unknown Object (File)
Fri, Nov 15, 3:48 AM
Subscribers

Details

Summary

While working on an iflib.4 manpage, various issues with
the sysctl OIDs exposed by iflib came up. This patch has fixes for
most of them, but there's still a couple open questions.

Don't increment txq_drain_encapfail twice on all errors other than ENOBUFS
Add comments regarding txq_drain_encapfail and encap_txd_encap_fail this needs further review
Increment rxd_flush whenever ctx->isc_rxd_flush() is called
Increment tx_encap whenever ctx->isc_txd_encap() is called
Increment rx_intr_enables whenever IFDI_RX_QUEUE_INTR_ENABLE() is called
Increment tx_frees in more places where tx mbufs are freed and not cloned
Increment txq->ift_pullups whenever m_pullup() is called. This requires adding a txq argument to collapse_pkthdr()
Increment txq->ift_mbuf_defrag whenever m_defrag() is called

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19064
Build 18695: arc lint + arc unit

Event Timeline

Remove bnxt.4 changes from review.

sys/net/iflib.c
846

There's an open question here of the netmap refill should increment the fl_refills* values.

3462

Not sure why this doesn't count against encap_txd_encap_fail.

sys/net/iflib.c
3046–3047

Totally not your fault, but looking at the review reminded me that this code existed. This collapse_pkthdr() thing is broken (derefs m_next->m_next after m_next has been freed via m, which is the same pointer). I don't recall any other driver checking for a pkthdr with no data, and I don't think the stack will ever do that to a driver, so maybe we should just remove the check, or replace it with an error return or even a panic.. It seems clear that this code is not tested, so if we ever did encounter such a case, we'd go off the rails.

3462

I think all the error returns should count against encap_txd_encap_fail.

Eg, maybe set an retval to replace the return values in the existing error returns, and then always jump to a new encap_failed: label after defrag_failed than increments encap_txd_ecnap_fail and returns?

This revision is now accepted and ready to land.Aug 23 2018, 12:46 PM
This revision was automatically updated to reflect the committed changes.