|SUCCESS| [v2,4/4] net/ice: implement flow dump

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163489

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-30

git sha: fd0d1c9d3436942e842e5ce81d5ae26c5bd58bca, etc.
Author: Anatoly Burakov <anatoly.burakov at intel.com>
Subject: [PATCH v2 1/4] net/ixgbe: implement flow dump

This patch series adds flow dump API implementation across multiple Intel
network drivers: ixgbe, i40e, iavf, and ice. Each driver implements the
rte_flow dev_dump operation to output binary rule data in hex format.

> diff --git a/drivers/net/intel/ixgbe/ixgbe_ethdev.h 
b/drivers/net/intel/ixgbe/ixgbe_ethdev.h
> index 32d7b98ed1..3f00896943 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/intel/ixgbe/ixgbe_ethdev.h
> @@ -515,6 +515,9 @@ uint16_t ixgbe_vf_representor_tx_burst(void 
*tx_queue, struct rte_mbuf **tx_pkts
>  #define IXGBE_DEV_FDIR_CONF(dev) \
>  	(&((struct ixgbe_adapter *)(dev)->data->dev_private)->fdir_conf)
>  
> +#define IXGBE_DEV_PRIVATE_TO_ADAPTER(adapter) \
> +	((struct ixgbe_adapter *)adapter)

This macro is defined but never used in this patch. Can it be removed or
is it needed for a future change?

[ ... ]

> diff --git a/drivers/net/intel/ixgbe/ixgbe_flow.c 
b/drivers/net/intel/ixgbe/ixgbe_flow.c
> index 01cd4f9bde..e40ece9755 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_flow.c

[ ... ]

> @@ -3581,9 +3583,148 @@ ixgbe_flow_flush(struct rte_eth_dev *dev,
>  	return 0;
>  }
>  
> +#define IXGBE_FLOW_DUMP_CHUNK_BYTES 32
> +
> +static const char *
> +ixgbe_flow_rule_engine_name(const struct rte_flow *flow)
> +{
> +	if (flow->is_security)
> +		return "security";
> +
> +	switch (flow->filter_type) {
> +	case RTE_ETH_FILTER_NTUPLE:
> +		return "ntuple";
> +	case RTE_ETH_FILTER_ETHERTYPE:
> +		return "ethertype";
> +	case RTE_ETH_FILTER_SYN:
> +		return "syn";
> +	case RTE_ETH_FILTER_FDIR:
> +		return "fdir";
> +	case RTE_ETH_FILTER_L2_TUNNEL:
> +		return "l2_tunnel";
> +	case RTE_ETH_FILTER_HASH:
> +		return "hash";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +static size_t
> +ixgbe_flow_rule_size(const struct rte_flow *flow)
> +{
> +	if (flow->is_security)
> +		return 0;
> +
> +	switch (flow->filter_type) {
> +	case RTE_ETH_FILTER_NTUPLE:
> +		return sizeof(struct rte_eth_ntuple_filter);
> +	case RTE_ETH_FILTER_ETHERTYPE:
> +		return sizeof(struct rte_eth_ethertype_filter);
> +	case RTE_ETH_FILTER_SYN:
> +		return sizeof(struct rte_eth_syn_filter);
> +	case RTE_ETH_FILTER_FDIR:
> +		return sizeof(struct ixgbe_fdir_rule);
> +	case RTE_ETH_FILTER_L2_TUNNEL:
> +		return sizeof(struct ixgbe_l2_tunnel_conf);
> +	case RTE_ETH_FILTER_HASH:
> +		return sizeof(struct ixgbe_rte_flow_rss_conf);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static const void *
> +ixgbe_flow_rule_data(const struct rte_flow *flow)
> +{
> +	if (flow->is_security || flow->rule == NULL)
> +		return NULL;
> +
> +	switch (flow->filter_type) {
> +	case RTE_ETH_FILTER_NTUPLE:
> +		return RTE_PTR_ADD(flow->rule, 
sizeof(TAILQ_ENTRY(ixgbe_ntuple_filter_ele)));
> +	case RTE_ETH_FILTER_ETHERTYPE:
> +		return RTE_PTR_ADD(flow->rule, 
sizeof(TAILQ_ENTRY(ixgbe_ethertype_filter_ele)));
> +	case RTE_ETH_FILTER_SYN:
> +		return RTE_PTR_ADD(flow->rule, 
sizeof(TAILQ_ENTRY(ixgbe_eth_syn_filter_ele)));
> +	case RTE_ETH_FILTER_FDIR:
> +		return RTE_PTR_ADD(flow->rule, 
sizeof(TAILQ_ENTRY(ixgbe_fdir_rule_ele)));
> +	case RTE_ETH_FILTER_L2_TUNNEL:
> +		return RTE_PTR_ADD(flow->rule, 
sizeof(TAILQ_ENTRY(ixgbe_eth_l2_tunnel_conf_ele)));
> +	case RTE_ETH_FILTER_HASH:
> +		return RTE_PTR_ADD(flow->rule, sizeof(TAILQ_ENTRY(ixgbe_rss_conf_ele)));
> +	default:
> +		return NULL;
> +	}
> +}

Does this code assume a specific structure layout for each filter element
type? The pointer arithmetic with RTE_PTR_ADD and sizeof(TAILQ_ENTRY(...))
suggests these structures have a TAILQ_ENTRY as their first member followed
by the actual filter data. Is this layout guaranteed to be stable? If the
struct layout changes in the element definitions, would this pointer
arithmetic still be valid?

> +
> +static void
> +ixgbe_flow_dump_blob(FILE *file, const char *engine,
> +		     const void *data, size_t data_len)
> +{
> +	const uint8_t *raw = (const uint8_t *)data;
> +	const size_t nchunks =
> +		(data_len + IXGBE_FLOW_DUMP_CHUNK_BYTES - 1) /
> +		IXGBE_FLOW_DUMP_CHUNK_BYTES;
> +	char title[64];
> +	size_t ci;
> +
> +	fprintf(file, "FLOW DUMP: driver=ixgbe engine=%s\n", engine);
> +	fprintf(file, "FLOW DUMP: DATA size=%zu chunks=%zu chunk_bytes=%d\n",
> +		data_len, nchunks, IXGBE_FLOW_DUMP_CHUNK_BYTES);
> +
> +	for (ci = 0; ci < nchunks; ci++) {
> +		const size_t off = ci * IXGBE_FLOW_DUMP_CHUNK_BYTES;
> +		const size_t clen =
> +			RTE_MIN((size_t)IXGBE_FLOW_DUMP_CHUNK_BYTES, data_len - off);
> +
> +		snprintf(title, sizeof(title), "FLOW DUMP: chunk %03zu/%03zu",
> +			 ci + 1, nchunks);
> +		rte_memdump(file, title, raw + off, clen);
> +	}
> +}

When data is NULL and data_len is 0, does this function handle that case
safely? The loop would not iterate when nchunks is 0, but would the initial
fprintf statements be misleading in that scenario?

> +
> +static int
> +ixgbe_flow_dev_dump(struct rte_eth_dev *dev __rte_unused,
> +		    struct rte_flow *flow,
> +		    FILE *file,
> +		    struct rte_flow_error *error)
> +{
> +	struct ixgbe_flow_mem *flow_mem_base;
> +	bool found = false;
> +
> +	TAILQ_FOREACH(flow_mem_base, &ixgbe_flow_list, entries) {
> +		struct ixgbe_flow_mem *ixgbe_flow_mem_ptr =
> +			(struct ixgbe_flow_mem *)flow_mem_base;
> +		struct rte_flow *p_flow = ixgbe_flow_mem_ptr->flow;
> +		const void *rule_data = NULL;
> +		const char *engine_name;
> +		size_t rule_size = 0;
> +
> +		if (flow != NULL && p_flow != flow)
> +			continue;
> +
> +		found = true;
> +		if (p_flow->rule != NULL) {
> +			rule_data = ixgbe_flow_rule_data(p_flow);
> +			rule_size = ixgbe_flow_rule_size(p_flow);
> +		}
> +		engine_name = ixgbe_flow_rule_engine_name(p_flow);
> +		ixgbe_flow_dump_blob(file, engine_name,
> +			rule_data, rule_size);
> +	}

Does this code hold any lock while iterating over ixgbe_flow_list? Can
flows be added or removed concurrently during the iteration?

[ ... ]

> diff --git a/drivers/net/intel/i40e/i40e_flow.c 
b/drivers/net/intel/i40e/i40e_flow.c
> index 967af052ff..cb246da780 100644
> --- a/drivers/net/intel/i40e/i40e_flow.c
> +++ b/drivers/net/intel/i40e/i40e_flow.c

[ ... ]

> @@ -1256,6 +1262,103 @@ i40e_flow_parse_attr(const struct rte_flow_attr 
*attr,
>  	return 0;
>  }
>  
> +#define I40E_FLOW_DUMP_CHUNK_BYTES 32
> +
> +static const char *
> +i40e_flow_rule_name(enum rte_filter_type filter_type)
> +{
> +	switch (filter_type) {
> +	case RTE_ETH_FILTER_ETHERTYPE:
> +		return "ethertype";
> +	case RTE_ETH_FILTER_FDIR:
> +		return "fdir";
> +	case RTE_ETH_FILTER_TUNNEL:
> +		return "tunnel";
> +	case RTE_ETH_FILTER_HASH:
> +		return "hash";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +static size_t
> +i40e_flow_rule_size(enum rte_filter_type filter_type)
> +{
> +	switch (filter_type) {
> +	case RTE_ETH_FILTER_ETHERTYPE:
> +		return sizeof(struct i40e_ethertype_filter);
> +	case RTE_ETH_FILTER_FDIR:
> +		return sizeof(struct i40e_fdir_filter);
> +	case RTE_ETH_FILTER_TUNNEL:
> +		return sizeof(struct i40e_tunnel_filter);
> +	case RTE_ETH_FILTER_HASH:
> +		return sizeof(struct i40e_rss_filter);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static void
> +i40e_flow_dump_blob(FILE *file, const char *engine,
> +		    const void *data, size_t data_len)
> +{
> +	const uint8_t *raw = (const uint8_t *)data;
> +	const size_t nchunks =
> +		(data_len + I40E_FLOW_DUMP_CHUNK_BYTES - 1) /
> +		I40E_FLOW_DUMP_CHUNK_BYTES;
> +	char title[64];
> +	size_t ci;
> +
> +	fprintf(file, "FLOW DUMP: driver=i40e engine=%s\n", engine);
> +	fprintf(file, "FLOW DUMP: DATA size=%zu chunks=%zu chunk_bytes=%d\n",
> +		data_len, nchunks, I40E_FLOW_DUMP_CHUNK_BYTES);
> +
> +	for (ci = 0; ci < nchunks; ci++) {
> +		const size_t off = ci * I40E_FLOW_DUMP_CHUNK_BYTES;
> +		const size_t clen =
> +			RTE_MIN((size_t)I40E_FLOW_DUMP_CHUNK_BYTES, data_len - off);
> +
> +		snprintf(title, sizeof(title), "FLOW DUMP: chunk %03zu/%03zu",
> +			 ci + 1, nchunks);
> +		rte_memdump(file, title, raw + off, clen);
> +	}
> +}
> +
> +static int
> +i40e_flow_dev_dump(struct rte_eth_dev *dev,
> +		   struct rte_flow *flow,
> +		   FILE *file,
> +		   struct rte_flow_error *error)
> +{
> +	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
> +	struct rte_flow *p_flow;
> +	bool found = false;
> +
> +	TAILQ_FOREACH(p_flow, &pf->flow_list, node) {
> +		size_t rule_size = 0;
> +		const void *rule_data = NULL;
> +
> +		if (flow != NULL && p_flow != flow)
> +			continue;
> +
> +		found = true;
> +		if (p_flow->rule != NULL) {
> +			rule_size = i40e_flow_rule_size(p_flow->filter_type);
> +			rule_data = p_flow->rule;
> +		}
> +		i40e_flow_dump_blob(file,
> +			i40e_flow_rule_name(p_flow->filter_type),
> +			rule_data, rule_size);
> +	}
> +
> +	if (flow != NULL && !found)
> +		return rte_flow_error_set(error, ENOENT,
> +			RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +			"Flow not found");
> +
> +	return 0;
>