iflib sysctl cleanup
ClosedPublic
Actions

Authored by shurd on Aug 15 2018, 8:24 PM.

Details

Reviewers

mmacy
sbruno
jeffrey.e.pieper_intel.com
gallatin
erj

Group Reviewers

manpages

Commits

rS338505: Clean up iflib sysctls

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 18829
Build 18492: arc lint + arc unit

Event Timeline

shurd created this revision.Aug 15 2018, 8:24 PM

Herald added a reviewer: manpages. · View Herald TranscriptAug 15 2018, 8:24 PM

Herald added subscribers: ae, imp. · View Herald Transcript

Harbormaster completed remote builds in B18828: Diff 46736.Aug 15 2018, 8:24 PM

Remove bnxt.4 changes from review.

Harbormaster completed remote builds in B18829: Diff 46737.Aug 15 2018, 8:25 PM

shurd added inline comments.Aug 15 2018, 8:28 PM

sys/net/iflib.c
846	There's an open question here of the netmap refill should increment the fl_refills* values.
3480	Not sure why this doesn't count against encap_txd_encap_fail.

gallatin added inline comments.Aug 16 2018, 1:03 PM

sys/net/iflib.c
3055	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.
3480	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?