Re: [PATCH v6] ethdev: support inline calculating masked item value

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 7 Oct 2026 19:57:44 +0300
Bing Zhao <bingz@nvidia.com> wrote:

> In the asynchronous API definition and some drivers, the
> rte_flow_item spec value may not be calculated by the driver due to the
> reason of speed of light rule insertion rate and sometimes the input
> parameters will be copied and changed internally.
> 
> After copying, the spec and last will be protected by the keyword
> const and cannot be changed in the code itself. And also the driver
> needs some extra memory to do the calculation and extra conditions
> to understand the length of each item spec. This is not efficient.
> 
> To solve the issue and support usage of the following fix, a new OP
> was introduced to calculate the spec and last values after applying
> the mask inline.
> 
> Signed-off-by: Bing Zhao <bingz@nvidia.com>
> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>
> ---

Existing AI review done by CI is useless at this point...

Running a more detailed review does find some things.

Review: [PATCH v6] ethdev: support inline calculating masked item value

Error
-----

1. NULL mask leaves spec/last unmasked.

   rte_flow semantics: a NULL item mask means the default mask
   (rte_flow_item_<name>_mask) applies, not "match all bits". Here
   item_mask_size is 0 when src->mask is NULL, so spec and last are
   copied raw. For ETH the default mask is all ones so nothing shows,
   but for IPV4 (default mask covers only src/dst addr) tos, ttl,
   etc. come back unmasked. A driver relying on the new op to get
   "masked values" gets wrong ones.

   rte_flow.c has no type -> default mask table, so either add one
   to rte_flow_desc_data, or document that a NULL mask is not
   applied and the caller must handle it (see doc text in Warning 3).

Warning
-------

1. PMD private items get their opaque data ANDed.

   rte_flow_conv_item_mask_size() returns sizeof(void *) for
   (int)type < 0, so the first pointer-size bytes of a private spec
   are ANDed with the first bytes of its mask. ethdev has no idea
   what those bytes mean. Leave them alone:

	if ((int)item->type < 0)
		return 0;

2. Variable part of RAW and GENEVE_OPT is not masked.

   item_mask_size stops at offsetof(pattern) / offsetof(data), so
   the copied pattern bytes and option data are never ANDed with
   mask->pattern / mask->data. The same holds for FLEX (desc_fn
   returns 0). Either mask them or document it; the current comment
   says the mask is applied to the spec, which is not true for these.

3. Doxygen for RTE_FLOW_CONV_OP_PATTERN_MASKED is a copy of
   OP_PATTERN and does not describe the limits above. Suggest:

	/**
	 * Convert an entire pattern, applying item masks.
	 *
	 * Same as RTE_FLOW_CONV_OP_PATTERN, except each copied spec
	 * and last is ANDed with the item mask. Not masked:
	 * - items with a NULL mask (default mask is not applied);
	 * - RAW pattern and GENEVE_OPT data contents;
	 * - FLEX and PMD private items.
	 *
	 * - @p src type:
	 *   @code const struct rte_flow_item * @endcode
	 * - @p dst type:
	 *   @code struct rte_flow_item * @endcode
	 */

4. Missing release notes. New rte_flow_conv() op is a new API and
   needs an entry in doc/guides/rel_notes/release_26_11.rst.

5. Commit message is hard to parse ("due to the reason of speed of
   light rule insertion rate", "support usage of the following fix"
   refers to a patch not in this series). Suggested body:

	Add RTE_FLOW_CONV_OP_PATTERN_MASKED to rte_flow_conv(). It
	copies a pattern like RTE_FLOW_CONV_OP_PATTERN and ANDs each
	item spec and last with the item mask.

	Drivers using the async flow API copy patterns on the rule
	insertion path and need masked values. The copied spec and
	last are const, so masking them in the driver needs extra
	memory and per-item length handling. Doing it during the
	conversion avoids both.

Info
----

1. No in-tree user. Please send the mlx5 consumer in the same series
   so the API is exercised and reviewed in context.

2. Spec and last masking loops are duplicated, and item_mask_size is
   computed even when with_mask is false. Factor out:

	static void
	rte_flow_conv_apply_mask(uint8_t *buf, const uint8_t *mask,
				 size_t len)
	{
		size_t i;

		for (i = 0; i < len; i++)
			buf[i] &= mask[i];
	}

   and compute the size only under with_mask && mask.

3. Test does not cover a NULL-mask item or an item with a non-trivial
   default mask (e.g. IPV4). Add one once Error 1 is settled so the
   chosen behaviour is locked in.