On 9/17/2026 2:26 AM, Stephen Hemminger wrote:
On Wed, 16 Sep 2026 13:18:08 +0100
Anatoly Burakov <[email protected]> wrote:
+
+/*
+ * This is a common header for Intel Ethernet drivers' flow engine
+ * implementations. It defines the interfaces and data structures required to
+ * implement flow rule engines that can be plugged into the drivers' flow
+ * handling logic.
+ *
+ * Design considerations:
+ *
+ * 1. Ease of implementation
+ *
+ * The flow engine interface is designed to be as simple as possible with
+ * obvious defaults (i.e. not specifying something leads to behavior that
+ * would've been the most expected in context). The point is not to produce a
+ * monstrous driver-within-a-driver framework, but rather to make engine
+ * definitions follow semantic expectations of what the engine actually does.
+ *
+ * All the boilerplate (flow management, engine enablement tracking, etc.) is
+ * handled by the common flow infrastructure, so the engine implementation only
+ * needs to focus on the actual logic of parsing and installing/uninstalling
+ * flow rules, and defining each step of the process as it pertains to each
flow
+ * engine.
+ *
+ * It is expected that drivers will use other utility functions from the common
+ * flow-related code where applicable (e.g. flow_util.h, flow_check.h, etc.).
+ *
+ * 2. Full secondary process compatibility
+ *
+ * In order to support rte_flow operations in secondary processes, we need to
+ * store which engines are enabled for particular driver instance, and resolve
+ * them at runtime. The engine index (its position in the engine list) is used
as
+ * a bit position in a driver-specific 64-bit field of enabled engines. This
+ * way, the engine definitions can be stored in read-only memory, and
referenced
+ * by both primary and secondary processes without issues.
+ *
+ * For this to remain safe, flow engine lists and engine definitions must be
+ * immutable for process lifetime (declare them as const).
+ *
+ * Note that this does not imply that all drivers are therefore able to support
+ * rte_flow-related operations in secondary processes - that is still up to
each
+ * driver to implement. This just ensures that the flow engine framework does
+ * not prevent it.
+ *
+ * Engine callbacks must not access or retain an `struct rte_eth_dev *`
pointer,
+ * as that object is process-local; use the process-independent
+ * `struct rte_eth_dev_data *` provided by the framework instead.
+ *
+ * The per-instance engine configuration is set up and torn down exclusively by
+ * `ci_flow_engine_conf_init()` and `ci_flow_engine_conf_reset()`. These
functions
+ * should only be called at device setup/teardown by primary process.
+ *
+ * 3. Flow object lifecycle is framework-owned
+ *
+ * Engines are expected to treat framework-provided context and flow objects as
+ * storage they fill in, not storage they own. In other words, engine logic
+ * should focus on contents of flow data, while object lifetime is managed by
+ * the framework. Engines may still allocate auxiliary data, but only in places
+ * where the framework guarantees a matching teardown call, which will give the
+ * engine the opportunity to release said auxiliary data.
+ *
+ * 4. Pattern parsing: flow_graph and pattern_parse callback
+ *
+ * The flow engine framework is designed to work hand-in-hand with the
+ * `flow_graph` parsing infrastructure. Each engine may provide a pattern
+ * graph that is used to match the flow pattern, and extract relevant data
+ * into the engine context provided by the framework.
+ *
+ * Engines may also provide a `pattern_parse` callback that is invoked before
+ * the graph parser runs. This allows engines to handle pattern items that
+ * don't fit neatly into the graph model (e.g. FUZZY items that can appear at
+ * any position), as well as ignoring the graph parser entirely and
implementing
+ * custom pattern parsing.
+ *
+ * There is no way to completely ignore pattern contents for the engine except
+ * for defining a noop `pattern_parse` callback. This is by design, as such
case
+ * is considered rte_flow API misuse. By default, even for empty fallback case,
+ * a meaningful pattern (one that is not empty or ANY) will be treated as
error.
+ *
+ * 5. Setup, teardown, and flow list lifecycle ordering
+ *
+ * `ci_flow_engine_conf_init()` and `ci_flow_engine_conf_reset()` are
+ * primary-process-only (see point 2), and the framework does not serialize
+ * them against concurrent flow operations or against each other - the driver
+ * must do so.
+ *
+ * The expected sequence of calls for a driver instance is:
+ *
+ * - `ci_flow_engine_conf_init()` should run from the driver's `dev_init` path.
+ *
+ * - At `dev_close`, `ci_flow_cleanup()` should be run first to drop all flows
+ * and their internal tracking. Then, `ci_flow_engine_conf_reset()` can be
run.
+ *
+ * - Devices may or may not advertise `RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP`,
+ * i.e. support for keeping flow rules across a `dev_stop`/`dev_start`
+ * cycle.
+ *
+ * - If the device does not advertise this capability, flows must be
+ * flushed via `ci_flow_flush()` as the first step of `dev_stop()` (doing so
+ * later may interfere with flow uninstall).
+ *
+ * - If rule replay is needed (i.e. flows were kept rather than flushed at
+ * `dev_stop`), the driver should call `ci_flow_replay()` from `dev_start`
+ * to re-install the kept flows to hardware as last step.
+ *
This is excess commenting, which is the kind of thing AI likes to generate
unless you tell to get to the point. Even AI evaluating itself said:
Such "excessive" commenting is intentional and it was not in fact
written by AI, it was written by me :)
This is an Intel-internal header so this can't be in programmer's guide,
but I do need to spell out the design intention and constraints, both to
prevent future misuse, and to enable far easier and far more precise AI
review (because now AI can read all of these comments and notice if the
reviewed code does not follow the design intent). AI calls this a
"design essay", and it's right: it *is* a design essay! That was the
entire point!
Warning
Comments are far longer than the code needs. Roughly a third of
flow_engine.h (587 of 1620 lines) and flow_graph.h (175 of 507) is
comment text, and many comments narrate the obvious or restate the
design document. Examples:
flow_graph.h, _flow_graph_node_is_expected():
/*
* In the interest of everyone debugging flow parsing code, we should
* provide the user with meaningful messages about exactly what failed,
* as no one likes non-descript "node constraints not met" errors with
* no clear indication of where this is even coming from. What follows
* is us building said meaningful error messages. It's a bit ugly, but
* it is for the greater good.
*/
flow_engine.h: the file header and the struct ci_flow_engine_ops
comment together run about 290 lines of design essay. ci_flow_parse()
repeats the same match-mode table that already appears above
ci_flow_engine_ops.
ixgbe_flow_dev_dump() and i40e_flow_dev_dump() (patches 4 and 13)
carry a 13-line comment explaining one if statement.
i40e_fdir_flow_install() and i40e_fdir_flow_register() (patch 16)
open with paragraph-length comments before a single condition.
Short one-line comments are enough for straightforward code. Put the
design description in flow_graph.rst (or a short block at the top of
flow_engine.h) once, and drop the per-function restatements. Trivial
comments such as "/* success */", "/* is the pointer valid? */" and
"/* engine looks valid */" can go.
This can't go into flow_graph documentation because this is not ethdev
header, this is common Intel code. There's no place in documentation for
this, at least not that I can see.
I'll see if I can reduce the number of "obvious" comments and any
possible duplication, but I would not look to reduce the overall comment
style. This is intended to present a complex system, I *want* it to be
well documented. The fact that our driver code is *not* this well
documented is why it's often impossible to reason about it.
--
Thanks,
Anatoly
