Re: [PATCH v3 00/19] Building a better rte_flow parser

[Stephen Hemminger <stephen@networkplumber.org>]

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