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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 9/17/2026 2:26 AM, Stephen Hemminger wrote:
> 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:

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