ice: Add a failure injection facility
Needs ReviewPublic
Actions

Authored by kbowling on Wed, Aug 19, 1:59 AM.

Details

Reviewers

kgalazka
gallatin
nick_spun.io
ziaee

Group Reviewers

Intel Networking
Restricted Owners Package	(Owns No Changed Paths)
manpages

Summary

Add compile-time optional, non-sleeping fail points around every VF
creation resource boundary, before VF VSI reconstruction, and in the
GET_STATS validation path.

Provide an ICE-wide wrapper and device selector so other driver
subsystems can add scoped points without duplicating the failpoint
plumbing.  Keep the current SR-IOV points and VF selector in an iov
child namespace.

Compile the facility only with options DRIVER_FAILPOINTS.  This shared
option avoids a separate kernel option for every driver that provides
test-only injection hooks.  Ordinary kernels contain no ICE failpoint
objects or sysctl nodes.  Require an exact PF device name and
optionally a VF index before any point can fire.  This prevents a stale
test setting from affecting another PF.

The hooks exposed two reset-lifetime defects while validating the
existing SR-IOV review series.  A PF reset could discard a firmware VSI
before teardown, and a rebuilt sibling could leave stale switch-filter
state after IOV destroy.

Tested on an E810-XXV with INVARIANTS and WITNESS.  All twelve creation
checkpoints rolled back and permitted immediate resource reuse.  Forced
reconstruction and malformed GET_STATS failures also preserved sibling
operation and reply cardinality.

MFC after:      2 weeks
Sponsored by:   BBOX.io

Test Plan

Can be triggered by hand or hooked into other test harnesses. The idea of keeping it behind a conf is to avoid polluting the sysctl tree for something only relevant to particular driver workers (as opposed to the iflib ones which are generic and could be hooked into CI on virtual devs etc). This is also to serve as a template for other driver workers.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kbowling created this revision.Wed, Aug 19, 1:59 AM

Owners added a reviewer: Restricted Owners Package.Wed, Aug 19, 1:59 AM

Herald added a subscriber: imp. · View Herald TranscriptWed, Aug 19, 1:59 AM

kbowling requested review of this revision.Wed, Aug 19, 1:59 AM

gallatin added inline comments.Wed, Aug 19, 1:47 PM

sys/conf/NOTES
442	I might make this more general. Eg, DRIVER_FAILPOINTS, so we don't have options explode for every driver.

Accept @gallatin suggestion of general driver conf, add small targeted doc (will not enumerate fail points) on fail testing to ice(4)

Herald added a subscriber: ziaee. · View Herald TranscriptThu, Aug 20, 12:08 AM

kbowling marked an inline comment as done.Thu, Aug 20, 12:08 AM

In D58940#1353461, @kbowling wrote:

Accept @gallatin suggestion of general driver conf, add small targeted doc (will not enumerate fail points) on fail testing to ice(4)

I'm probably going to follow suit and add some fail points to aq(4) so this helps :)

kbowling added a child revision: D59021: ice: Add malformed virtchnl injection points.Thu, Aug 20, 2:06 AM

kbowling added a child revision: D59025: ice: Add VF reset and policy failure injection.

kbowling added a child revision: D59026: ice: Make VF MAC filter requests idempotent.Thu, Aug 20, 2:08 AM

kbowling added a child revision: D59029: ice: Protect the PF mailbox from flooding VFs.

gallatin accepted this revision.Thu, Aug 20, 12:45 PM

nick_spun.io accepted this revision.Sun, Aug 23, 5:15 PM

ziaee added a reviewer: manpages.Mon, Aug 24, 6:04 PM

ziaee added inline comments.

share/man/man4/ice.4
1196–1200
1196–1202	Is this a sysctl? Whatever it is, I'd like to have that context as shown. Also, see the suggestion for correct markup of kernel configuration declarations inside prose.
sys/dev/ice/ice_fault.h
1–30	We have updated the preferred license in the licensing policy, please take a look: https://docs.freebsd.org/en/articles/license-guide/ Suggestion: Note that SPDX short form is preferred, 2 clause is preferred over 3 clause, and "all rights reserved" is implied by the berne convention since 1989.

Requesting changes for manual page markup.

This revision now requires changes to proceed.Mon, Aug 24, 6:05 PM

address license format and man page

kbowling marked an inline comment as done.Fri, Aug 28, 6:17 AM

kbowling added inline comments.

share/man/man4/ice.4
1196–1202	Sort of, but that's just the interface to a developer tool. See fail(9).

kbowling marked an inline comment as done.Fri, Aug 28, 6:18 AM

ziaee added inline comments.Fri, Aug 28, 3:48 PM

share/man/man4/ice.4
1197	Nit: .Cd takes the rest of the line as an argument, so it does not need to be quoted
1198	Oh, okay thanks. Well if this fail is actually referring to a specific interface, maybe xreffing at the top of the section could make that more clear for those unfamiliar?