On Wed, 16 Sep 2026 13:18:06 +0100
Anatoly Burakov <[email protected]> wrote:
> Most rte_flow parsers in DPDK suffer from huge implementation complexity
> because
> even though 99% of what people use rte_flow parsers for is parsing protocol
> graphs, no parser is written explicitly as a graph. This patchset attempts to
> suggest a viable model to build rte_flow parsers as graphs, by offering a
> lightweight header only library to build rte_flow parsering graphs without too
> much boilerplate and complexity.
>
> Most of the patchset is about Intel drivers, but they are meant as
> reimplementations as well as examples for the rest of the community to assess
> how to build parsers using this new infrastructure. I expect the first two
> patches will be of most interest to non-Intel reviewers, as they deal with
> building two reusable parser architecture pieces.
>
> The first piece is a new flow graph helper in ethdev. Its purpose is
> deliberately narrow: it targets the protocol-graph part of rte_flow pattern
> parsing, where drivers walk packet headers and validate legal item sequences
> and
> parameters. That does not cover all possible rte_flow features, especially
> more
> exotic flow items, but it does cover a large and widely shared part of what
> existing drivers need to do. Or, to put it in other words, the only flow items
> this infrastructure *doesn't* cover is things that do not lend themselves well
> to be parsed as a graph of protocol headers (e.g. conntrack items). Everything
> else should be covered or cover-able. In practice, just about all drivers will
> benefit from graph parsing as all but one of them implement only the protocol
> stack parts, which are the ones targeted by the graph helper.
>
> The second piece is a reusable flow engine framework for Intel Ethernet
> drivers.
> This is kept Intel-local because I do not feel it is even appropriate to
> define
> such a framework for all drivers to use in the first place. Even so, the
> intent
> is to establish a cleaner parser architecture with a defined interaction
> model,
> explicit memory ownership rules, locking, initialization sequence,
> implementations of rte_flow API entry points, flow replay and memory cleanup,
> and engine definitions that do not block secondary-process-safe usage. It is
> my
> hope that this would serve as a model for other drivers to follow, expand on,
> rework, and improve, so that maybe down the line we *might* have a common
> rte_flow infrastructure for drivers to use.
>
> Most of the rest of the series is parser reimplementation, but that is mainly
> the vehicle for demonstrating and validating those two pieces. ixgbe and i40e
> are wired into the new common parsing path, and their existing parsers are
> migrated incrementally to the graph-based model. Besides reducing ad hoc
> parser
> code, this also makes validation more explicit and more consistent. In a few
> places that means invalid inputs that were previously ignored, deferred, or
> interpreted loosely are now rejected earlier and more strictly, without any
> increase in code complexity (in fact, with marked *decrease* of it!).
AI review has lots of feedback (as always treat with skepticism)
Patch 1/19 (ethdev: add flow graph API)
Error
flow_graph.h breaks the build with -Dcheck_includes=true, in both the
C and C++ driver header checks:
ret = rte_flow_conv(RTE_FLOW_CONV_OP_ITEM_NAME_PTR,
rte_flow_conv() is experimental, and chkincs-drv builds without
ALLOW_EXPERIMENTAL_API:
flow_graph.h:54:9: error: 'rte_flow_conv' is deprecated: Symbol is
not yet part of stable ABI [-Werror=deprecated-declarations]
Either promote rte_flow_conv() to stable (it has been experimental
since 18.11), or do not call it from an installed inline helper.
Warning
The "non-END nodes must have edges" check never fires for the start
node:
if (node->type != RTE_FLOW_ITEM_TYPE_END &&
graph->edges[node_idx].next == NULL) {
The start node has no .type (per the doc and every graph in the
series), so its type is RTE_FLOW_ITEM_TYPE_END (0) and the check is
skipped. A graph with no edge list for START then dereferences NULL
here:
next_nodes = graph->edges[cur_idx].next;
for (edge_idx = 0; next_nodes[edge_idx] != FLOW_GRAPH_NODE_EDGE_END;
Test node_idx == FLOW_GRAPH_NODE_FIRST as well as the END type.
Info
FLOW_GRAPH_LOG declares
extern int RTE_CONCAT(RTE_COMPONENT_NAME, _logtype_driver);
in an installed driver SDK header. Any driver that includes it but
registers its logtype under another name fails to link. Consider
taking the logtype from the caller or using RTE_LOG_DEFAULT_LOGTYPE.
Installed header exports unprefixed names (struct flow_graph,
flow_graph_parse, FLOW_GRAPH_*) and file-scope identifiers with a
leading underscore (_flow_graph_*), which are reserved.
struct flow_graph and struct flow_graph_edge use non-const pointers
(struct flow_graph_node *nodes, size_t *next), so driver node tables
holding function pointers cannot be placed in read-only data.
When _flow_graph_find_next_node() fails because the graph is
malformed, flow_graph_parse() overwrites that error with "Pattern item
not supported", which hides the real cause.
flow_graph.rst: "The the ``END`` node can also have callbacks".
Patch 2/19 (net/intel/common: add flow engines infrastructure)
Warning
The commit message and header claim secondary-process compatibility,
but the engine list pointer is stored in shared memory:
const struct ci_flow_engine_list *engines;
...
engine_conf->engines = engine_list;
ci_flow_engine_conf is embedded in dev_private (e.g.
struct ci_flow_engine_conf flow_engine_conf;
in struct ixgbe_adapter), and engine_list is the address of driver
static data in the primary process. ci_flow_engine_ref_from_idx()
dereferences engine_conf->engines->engines[engine_idx], which is not
valid in a secondary process mapped at a different address. Pass the
engine list from the driver on each call, or resolve it per process,
instead of storing it in the conf.
Info
ci_flow_replay(), ci_flow_cleanup() and ci_flow_engine_conf_reset()
log error.message after a callback failure:
CI_DRV_LOG(DEBUG, "engine '%s': failed to install flow: %s",
engine_ref.engine->name, error.message);
error is zero-initialised, so a callback that returns non-zero
without calling rte_flow_error_set() passes NULL to %s.
ci_flow_flush() and ci_flow_cleanup() leave a flow on the list, and
leak it, when ci_flow_is_valid() fails:
if (!ci_flow_is_valid(flow, engine_conf))
continue;
ci_flow_flush() returns 0 after an uninstall failure while leaving the
caller's rte_flow_error populated.
Patch 3/19 (net/intel/common: add utility functions)
Info
Include guard comment does not match the guard:
#endif /* _INTEL_COMMON_FLOW_UTIL_H_ */
guard is _COMMON_INTEL_FLOW_UTIL_H_.
Patch 5/19 (net/ixgbe: reimplement ethertype parser)
Warning
ixgbe_dev_stop() now turns off PTP timestamping:
/* disable timestamping; the application must re-enable it after restart */
if (adapter->filter.timesync_installed)
ixgbe_timesync_disable(dev);
This is a user-visible behaviour change (timesync no longer survives
stop/start) with no release note in the patch.
Series
Patches 4-11 and 18 do not apply to main; the index lines reference
blobs not in the upstream history. Please state the base tree and any
dependent series (cover letter or base-commit).
Review-Result: ERROR
