Re: [PATCH v3 02/19] net/intel/common: add flow engines infrastructure

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 16 Sep 2026 13:18:08 +0100
Anatoly Burakov <anatoly.burakov@intel.com> 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:

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.