No. The three errors in that AI review are not valid, and the warnings do not block this patch.
rte_flow_conv_item_spec() returns size_t, the number of bytes to copy. It does not return a negative errno. In rte_flow_conv_pattern() the local ret is also size_t, so if (ret < 0) is always false and cannot be the missing check the review describes. That copy pattern was already there for spec, last, and mask. This patch only ANDs the mask onto a buffer after that copy has produced a pointer. The other notes are process nits: The with_mask comment is already clear. A release note belongs in the current release file, not release_25_03.rst. This master has no current notes file, and release_26_07.rst is gone, which is why that hunk was dropped. RTE_FLOW_CONV_OP_PATTERN_MASKED is one new enumerator at the end of an existing enum. It cannot be tagged __rte_experimental, and existing callers of rte_flow_conv() are unchanged. https://mails.dpdk.org/archives/test-report/2026-October/1057123.html @[email protected], FYI > -----Original Message----- > From: Bing Zhao <[email protected]> > Sent: Thursday, October 8, 2026 12:58 AM > To: Slava Ovsiienko <[email protected]>; [email protected]; Raslan > Darawsheh <[email protected]>; [email protected] > Cc: Ori Kam <[email protected]>; Dariusz Sosnowski <[email protected]>; > Suanming Mou <[email protected]>; Matan Azrad <[email protected]>; NBU- > Contact-Thomas Monjalon (EXTERNAL) <[email protected]> > Subject: [PATCH v6] ethdev: support inline calculating masked item value > > External email: Use caution opening links or attachments > > > 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 <[email protected]> > Acked-by: Dariusz Sosnowski <[email protected]> > --- > v3: > - add test code > - fix the issue found by AI > v4: reabse on top of the main > v5: handle some items separately and add test for them > v6: resend the correct version instead of v5 with AI defects resolved > --- > app/test/test_ethdev_api.c | 123 +++++++++++++++++++++++++++++++++++++ > lib/ethdev/rte_flow.c | 55 +++++++++++++++-- > lib/ethdev/rte_flow.h | 13 ++++ > 3 files changed, 185 insertions(+), 6 deletions(-) > > diff --git a/app/test/test_ethdev_api.c b/app/test/test_ethdev_api.c index > 00d6a5c614..ba3e785425 100644 > --- a/app/test/test_ethdev_api.c > +++ b/app/test/test_ethdev_api.c > @@ -2,8 +2,11 @@ > * Copyright (C) 2023, Advanced Micro Devices, Inc. > */ > > +#include <string.h> > + > #include <rte_log.h> > #include <rte_ethdev.h> > +#include <rte_flow.h> > > #include <rte_test.h> > #include "test.h" > @@ -15,6 +18,125 @@ > #define NUM_MBUF 1024 > #define MBUF_CACHE_SIZE 256 > > +static int32_t > +ethdev_api_flow_conv_pattern_masked(void) > +{ > + const struct rte_flow_item_eth spec = { > + .hdr.dst_addr.addr_bytes = { 0x01, 0x02, 0x03, 0x04, 0x05, > 0x06 }, > + .hdr.src_addr.addr_bytes = { 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, > 0x0f }, > + .hdr.ether_type = RTE_BE16(0x1234), > + }; > + const struct rte_flow_item_eth last = { > + .hdr.dst_addr.addr_bytes = { 0x11, 0x12, 0x13, 0x14, 0x15, > 0x16 }, > + .hdr.src_addr.addr_bytes = { 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, > 0x1f }, > + .hdr.ether_type = RTE_BE16(0x5678), > + }; > + const struct rte_flow_item_eth mask = { > + .hdr.dst_addr.addr_bytes = { 0xff, 0xff, 0x00, 0x00, 0xff, > 0xff }, > + .hdr.src_addr.addr_bytes = { 0xff, 0x00, 0xff, 0x00, 0xff, > 0x00 }, > + .hdr.ether_type = RTE_BE16(0xffff), > + }; > + const uint8_t raw_pattern[] = { 0xaa, 0xbb, 0xcc, 0xdd }; > + const struct rte_flow_item_raw raw_spec = { > + .relative = 1, > + .search = 1, > + .offset = 8, > + .limit = 4, > + .length = sizeof(raw_pattern), > + .pattern = raw_pattern, > + }; > + const struct rte_flow_item_raw raw_mask = { > + .relative = 1, > + .search = 0, > + .reserved = 0x3fffffff, > + .offset = 0xffffffff, > + .limit = 0xffff, > + .length = 0xffff, > + .pattern = NULL, > + }; > + const struct rte_flow_item pattern[] = { > + { > + .type = RTE_FLOW_ITEM_TYPE_ETH, > + .spec = &spec, > + .last = &last, > + .mask = &mask, > + }, > + { > + .type = RTE_FLOW_ITEM_TYPE_RAW, > + .spec = &raw_spec, > + .mask = &raw_mask, > + }, > + { .type = RTE_FLOW_ITEM_TYPE_END }, > + }; > + union { > + struct rte_flow_item item; > + struct rte_flow_item_eth eth; > + struct rte_flow_item_raw raw; > + double align; > + uint8_t buf[256]; > + } dst; > + const struct rte_flow_item *item; > + const struct rte_flow_item_eth *conv_spec; > + const struct rte_flow_item_eth *conv_last; > + const struct rte_flow_item_raw *conv_raw_spec; > + int ret; > + > + ret = rte_flow_conv(RTE_FLOW_CONV_OP_PATTERN_MASKED, NULL, 0, > pattern, NULL); > + TEST_ASSERT(ret > 0, "Masked pattern conversion size query > failed"); > + TEST_ASSERT((size_t)ret <= sizeof(dst.buf), > + "Masked pattern conversion needs too much storage"); > + > + memset(&dst, 0, sizeof(dst)); > + ret = rte_flow_conv(RTE_FLOW_CONV_OP_PATTERN_MASKED, dst.buf, > + sizeof(dst.buf), pattern, NULL); > + TEST_ASSERT(ret > 0, "Masked pattern conversion failed"); > + > + item = (const struct rte_flow_item *)dst.buf; > + conv_spec = item[0].spec; > + conv_last = item[0].last; > + TEST_ASSERT_NOT_NULL(conv_spec, "Converted spec must be set"); > + TEST_ASSERT_NOT_NULL(conv_last, "Converted last must be set"); > + > + TEST_ASSERT_EQUAL(conv_spec->hdr.dst_addr.addr_bytes[0], 0x01, > + "Masked spec dst byte 0 mismatch"); > + TEST_ASSERT_EQUAL(conv_spec->hdr.dst_addr.addr_bytes[2], 0x00, > + "Masked spec dst byte 2 mismatch"); > + TEST_ASSERT_EQUAL(conv_spec->hdr.src_addr.addr_bytes[1], 0x00, > + "Masked spec src byte 1 mismatch"); > + TEST_ASSERT_EQUAL(conv_spec->hdr.ether_type, RTE_BE16(0x1234), > + "Masked spec ether type mismatch"); > + TEST_ASSERT_EQUAL(conv_last->hdr.dst_addr.addr_bytes[0], 0x11, > + "Masked last dst byte 0 mismatch"); > + TEST_ASSERT_EQUAL(conv_last->hdr.dst_addr.addr_bytes[2], 0x00, > + "Masked last dst byte 2 mismatch"); > + TEST_ASSERT_EQUAL(conv_last->hdr.src_addr.addr_bytes[1], 0x00, > + "Masked last src byte 1 mismatch"); > + TEST_ASSERT_EQUAL(conv_last->hdr.ether_type, RTE_BE16(0x5678), > + "Masked last ether type mismatch"); > + > + conv_raw_spec = item[1].spec; > + TEST_ASSERT_NOT_NULL(conv_raw_spec, "Converted RAW spec must be > set"); > + TEST_ASSERT_NOT_NULL(conv_raw_spec->pattern, > + "Converted RAW pattern pointer must be set"); > + TEST_ASSERT(conv_raw_spec->pattern != raw_pattern, > + "Converted RAW pattern must be deep-copied"); > + TEST_ASSERT_EQUAL(conv_raw_spec->relative, 1, > + "Masked RAW relative mismatch"); > + TEST_ASSERT_EQUAL(conv_raw_spec->search, 0, > + "Masked RAW search mismatch"); > + TEST_ASSERT_EQUAL(conv_raw_spec->offset, raw_spec.offset, > + "Masked RAW offset mismatch"); > + TEST_ASSERT_EQUAL(conv_raw_spec->limit, raw_spec.limit, > + "Masked RAW limit mismatch"); > + TEST_ASSERT_EQUAL(conv_raw_spec->length, raw_spec.length, > + "Masked RAW length mismatch"); > + TEST_ASSERT_BUFFERS_ARE_EQUAL(conv_raw_spec->pattern, raw_pattern, > + sizeof(raw_pattern), > + "Converted RAW pattern mismatch"); > + > + return TEST_SUCCESS; > +} > + > static int32_t > ethdev_api_queue_status(void) > { > @@ -167,6 +289,7 @@ static struct unit_test_suite ethdev_api_testsuite = { > .setup = NULL, > .teardown = NULL, > .unit_test_cases = { > + TEST_CASE(ethdev_api_flow_conv_pattern_masked), > TEST_CASE(ethdev_api_queue_status), > /* TODO: Add deferred_start queue status test */ > TEST_CASES_END() /**< NULL terminate unit test array */ > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index > ca2f85c3fa..e4efcbf10f 100644 > --- a/lib/ethdev/rte_flow.c > +++ b/lib/ethdev/rte_flow.c > @@ -175,6 +175,23 @@ static const struct rte_flow_desc_data > rte_flow_desc_item[] = { > MK_FLOW_ITEM(COMPARE, sizeof(struct rte_flow_item_compare)), }; > > +static inline size_t > +rte_flow_conv_item_mask_size(const struct rte_flow_item *item) { > + if ((int)item->type < 0) > + return sizeof(void *); > + switch (item->type) { > + case RTE_FLOW_ITEM_TYPE_RAW: > + return offsetof(struct rte_flow_item_raw, pattern); > + case RTE_FLOW_ITEM_TYPE_GENEVE_OPT: > + return offsetof(struct rte_flow_item_geneve_opt, data); > + default: > + if (rte_flow_desc_item[item->type].desc_fn != NULL) > + return 0; > + return rte_flow_desc_item[item->type].size; > + } > +} > + > /** Generate flow_action[] entry. */ > #define MK_FLOW_ACTION(t, s) \ > [RTE_FLOW_ACTION_TYPE_ ## t] = { \ @@ -797,6 +814,8 @@ > rte_flow_conv_action_conf(void *buf, const size_t size, > * RTE_FLOW_ITEM_TYPE_END is encountered. > * @param[out] error > * Perform verbose error reporting if not NULL. > + * @param[in] with_mask > + * If true, @p src mask will be applied to spec and last. > * > * @return > * A positive value representing the number of bytes needed to store > @@ -809,12 +828,13 @@ rte_flow_conv_pattern(struct rte_flow_item *dst, > const size_t size, > const struct rte_flow_item *src, > unsigned int num, > + bool with_mask, > struct rte_flow_error *error) { > uintptr_t data = (uintptr_t)dst; > size_t off; > size_t ret; > - unsigned int i; > + unsigned int i, j; > > for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) { > /** > @@ -838,15 +858,27 @@ rte_flow_conv_pattern(struct rte_flow_item *dst, > src -= num; > dst -= num; > do { > + uint8_t *c_spec = NULL, *c_last = NULL; > + const uint8_t *mask = src->mask; > + size_t item_mask_size = mask ? > + rte_flow_conv_item_mask_size(src) : 0; > + > if (src->spec) { > off = RTE_ALIGN_CEIL(off, sizeof(double)); > ret = rte_flow_conv_item_spec > ((void *)(data + off), > size > off ? size - off : 0, src, > RTE_FLOW_CONV_ITEM_SPEC); > - if (size && size >= off + ret) > + if (size && size >= off + ret) { > dst->spec = (void *)(data + off); > + c_spec = (uint8_t *)(data + off); > + } > off += ret; > + if (with_mask && c_spec && mask) { > + size_t mask_size = RTE_MIN(ret, > + item_mask_size); > + > + for (j = 0; j < mask_size; j++) > + c_spec[j] &= mask[j]; > + } > > } > if (src->last) { > @@ -855,9 +887,17 @@ rte_flow_conv_pattern(struct rte_flow_item *dst, > ((void *)(data + off), > size > off ? size - off : 0, src, > RTE_FLOW_CONV_ITEM_LAST); > - if (size && size >= off + ret) > + if (size && size >= off + ret) { > dst->last = (void *)(data + off); > + c_last = (uint8_t *)(data + off); > + } > off += ret; > + if (with_mask && c_last && mask) { > + size_t mask_size = RTE_MIN(ret, > + item_mask_size); > + > + for (j = 0; j < mask_size; j++) > + c_last[j] &= mask[j]; > + } > } > if (src->mask) { > off = RTE_ALIGN_CEIL(off, sizeof(double)); @@ - > 1004,7 +1044,7 @@ rte_flow_conv_rule(struct rte_flow_conv_rule *dst, > off = RTE_ALIGN_CEIL(off, sizeof(double)); > ret = rte_flow_conv_pattern((void *)((uintptr_t)dst + > off), > size > off ? size - off : 0, > - src->pattern_ro, 0, error); > + src->pattern_ro, 0, false, > + error); > if (ret < 0) > return ret; > if (size && size >= off + (size_t)ret) @@ -1104,7 +1144,7 > @@ rte_flow_conv(enum rte_flow_conv_op op, > ret = sizeof(*attr); > break; > case RTE_FLOW_CONV_OP_ITEM: > - ret = rte_flow_conv_pattern(dst, size, src, 1, error); > + ret = rte_flow_conv_pattern(dst, size, src, 1, false, > + error); > break; > case RTE_FLOW_CONV_OP_ITEM_MASK: > item = src; > @@ -1119,7 +1159,7 @@ rte_flow_conv(enum rte_flow_conv_op op, > ret = rte_flow_conv_actions(dst, size, src, 1, error); > break; > case RTE_FLOW_CONV_OP_PATTERN: > - ret = rte_flow_conv_pattern(dst, size, src, 0, error); > + ret = rte_flow_conv_pattern(dst, size, src, 0, false, > + error); > break; > case RTE_FLOW_CONV_OP_ACTIONS: > ret = rte_flow_conv_actions(dst, size, src, 0, error); @@ > -1139,6 +1179,9 @@ rte_flow_conv(enum rte_flow_conv_op op, > case RTE_FLOW_CONV_OP_ACTION_NAME_PTR: > ret = rte_flow_conv_name(1, 1, dst, size, src, error); > break; > + case RTE_FLOW_CONV_OP_PATTERN_MASKED: > + ret = rte_flow_conv_pattern(dst, size, src, 0, true, > error); > + break; > default: > ret = rte_flow_error_set > (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL, > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index > f864578f80..2261d1ea06 100644 > --- a/lib/ethdev/rte_flow.h > +++ b/lib/ethdev/rte_flow.h > @@ -4544,6 +4544,19 @@ enum rte_flow_conv_op { > * @code const char ** @endcode > */ > RTE_FLOW_CONV_OP_ACTION_NAME_PTR, > + > + /** > + * Convert an entire pattern. > + * > + * Duplicates all pattern items at once, applying each source item > mask > + * to its copied specification and range. > + * > + * - @p src type: > + * @code const struct rte_flow_item * @endcode > + * - @p dst type: > + * @code struct rte_flow_item * @endcode > + */ > + RTE_FLOW_CONV_OP_PATTERN_MASKED, > }; > > /** > -- > 2.43.0
