* [PATCH v5 03/21] net/ngbe: add missing CDR config for YT PHY
From: Zaiyu Wang @ 2026-05-27 13:02 UTC (permalink / raw)
To: dev; +Cc: Zaiyu Wang, stable, Jiawen Wu
In-Reply-To: <20260527130222.24348-1-zaiyuwang@trustnetic.com>
According to the PHY vendor, when YT8531S operates in UTP-to-Fiber or
RGMII-to-Fiber mode with auto-negotiation disabled (Force mode),
additional CDR (Clock Data Recovery) configuration is required to
improve link connectivity. Without this config, link may be unstable
or fail to establish.
Fixes: f1268369403d ("net/ngbe: support autoneg on/off for external PHY SFI mode")
Cc: stable@dpdk.org
Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
---
drivers/net/ngbe/base/ngbe_phy_yt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ngbe/base/ngbe_phy_yt.c b/drivers/net/ngbe/base/ngbe_phy_yt.c
index d110fbc8b2..ab0778d246 100644
--- a/drivers/net/ngbe/base/ngbe_phy_yt.c
+++ b/drivers/net/ngbe/base/ngbe_phy_yt.c
@@ -264,6 +264,9 @@ s32 ngbe_setup_phy_link_yt(struct ngbe_hw *hw, u32 speed,
value = YT_BCR_RESET | YT_BCR_ANE | YT_BCR_RESTART_AN |
YT_BCR_DUPLEX | YT_BCR_SPEED_SELECT1;
} else {
+ /* force mode need to config cdr */
+ ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, 0x1434);
+ ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, 0x163);
value = YT_BCR_RESET | YT_BCR_DUPLEX;
if (speed & NGBE_LINK_SPEED_1GB_FULL)
value |= YT_BCR_SPEED_SELECT1;
--
2.21.0.windows.1
^ permalink raw reply related
* [PATCH v5 01/21] net/txgbe: remove duplicate xstats counters
From: Zaiyu Wang @ 2026-05-27 13:02 UTC (permalink / raw)
To: dev; +Cc: Zaiyu Wang, stable, Jiawen Wu, Ferruh Yigit
In-Reply-To: <20260527130222.24348-1-zaiyuwang@trustnetic.com>
Remove four redundant counters (tx_xon_packets, rx_xon_packets,
tx_xoff_packets and rx_xoff_packets) from xstats, as they were duplicates
of tx_flow_control_xon_packets and others. Both sets were reading the same
registers but being output twice under different names. After removing
these entries, the flow control counters in DPDK now align with those in
our Linux kernel driver.
Fixes: 91fe49c87d76 ("net/txgbe: support device xstats")
Cc: stable@dpdk.org
Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
---
drivers/net/txgbe/txgbe_ethdev.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 5d360f8305..779874aac9 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -261,11 +261,6 @@ static const struct rte_txgbe_xstats_name_off rte_txgbe_stats_strings[] = {
HW_XSTAT(tx_size_1024_to_max_packets),
/* Flow Control */
- HW_XSTAT(tx_xon_packets),
- HW_XSTAT(rx_xon_packets),
- HW_XSTAT(tx_xoff_packets),
- HW_XSTAT(rx_xoff_packets),
-
HW_XSTAT_NAME(tx_xon_packets, "tx_flow_control_xon_packets"),
HW_XSTAT_NAME(rx_xon_packets, "rx_flow_control_xon_packets"),
HW_XSTAT_NAME(tx_xoff_packets, "tx_flow_control_xoff_packets"),
--
2.21.0.windows.1
^ permalink raw reply related
* [PATCH v5 02/21] net/ngbe: remove duplicate xstats counters
From: Zaiyu Wang @ 2026-05-27 13:02 UTC (permalink / raw)
To: dev; +Cc: Zaiyu Wang, stable, Jiawen Wu
In-Reply-To: <20260527130222.24348-1-zaiyuwang@trustnetic.com>
Remove four redundant counters (tx_xon_packets, rx_xon_packets,
tx_xoff_packets and rx_xoff_packets) from xstats, as they were duplicates
of tx_flow_control_xon_packets and others. Both sets were reading the same
registers but being output twice under different names. After removing
these entries, the flow control counters in DPDK now align with those in
our Linux kernel driver.
Fixes: 8b433d04adc9 ("net/ngbe: support device xstats")
Cc: stable@dpdk.org
Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
---
drivers/net/ngbe/ngbe_ethdev.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
index 8b9d6371fb..6df53f3266 100644
--- a/drivers/net/ngbe/ngbe_ethdev.c
+++ b/drivers/net/ngbe/ngbe_ethdev.c
@@ -227,11 +227,6 @@ static const struct rte_ngbe_xstats_name_off rte_ngbe_stats_strings[] = {
HW_XSTAT(tx_size_1024_to_max_packets),
/* Flow Control */
- HW_XSTAT(tx_xon_packets),
- HW_XSTAT(rx_xon_packets),
- HW_XSTAT(tx_xoff_packets),
- HW_XSTAT(rx_xoff_packets),
-
HW_XSTAT_NAME(tx_xon_packets, "tx_flow_control_xon_packets"),
HW_XSTAT_NAME(rx_xon_packets, "rx_flow_control_xon_packets"),
HW_XSTAT_NAME(tx_xoff_packets, "tx_flow_control_xoff_packets"),
--
2.21.0.windows.1
^ permalink raw reply related
* [PATCH v5 00/21] Wangxun Fixes
From: Zaiyu Wang @ 2026-05-27 13:02 UTC (permalink / raw)
To: dev; +Cc: Zaiyu Wang
In-Reply-To: <20260423034024.14404-1-zaiyuwang@trustnetic.com>
This series fixes several issues found on Wangxun Emerald, Sapphire and
Amber-lite NICs, with a focus on link-related problems.
---
v5:
- Fixed issues identified by AI review
---
v4:
- Fixed issues identified by devtools scripts
---
v3:
- Addressed Stephen's comments
---
v2:
- Fixed compilation error and code style issues
---
Zaiyu Wang (21):
net/txgbe: remove duplicate xstats counters
net/ngbe: remove duplicate xstats counters
net/ngbe: add missing CDR config for YT PHY
net/ngbe: fix VF promiscuous and allmulticast
net/txgbe: fix inaccuracy in Tx rate limiting
net/txgbe: fix link status check condition
net/txgbe: fix Tx desc free logic
net/txgbe: fix link flow control registers for Amber-Lite
net/txgbe: fix link flow control config for Sapphire
net/txgbe: fix a mass of unknown interrupts
net/txgbe: fix traffic class priority configuration
net/txgbe: fix link stability for 25G NIC
net/txgbe: fix link stability for 40G NIC
net/txgbe: fix link stability for Amber-Lite backplane mode
net/txgbe: fix FEC mode configuration on 25G NIC
net/txgbe: fix SFP module identification
net/txgbe: fix get module info operation
net/txgbe: fix get EEPROM operation
net/txgbe: fix to reset Tx write-back pointer
net/txgbe: fix to enable Tx desc check
net/txgbe: fix temperature track for AML NIC
drivers/net/ngbe/base/ngbe_phy_yt.c | 3 +
drivers/net/ngbe/ngbe_ethdev.c | 5 -
drivers/net/ngbe/ngbe_ethdev_vf.c | 11 +-
drivers/net/txgbe/base/meson.build | 2 +
drivers/net/txgbe/base/txgbe.h | 2 +
drivers/net/txgbe/base/txgbe_aml.c | 185 +-
drivers/net/txgbe/base/txgbe_aml.h | 6 +-
drivers/net/txgbe/base/txgbe_aml40.c | 114 +-
drivers/net/txgbe/base/txgbe_aml40.h | 6 +-
drivers/net/txgbe/base/txgbe_dcb_hw.c | 2 +-
drivers/net/txgbe/base/txgbe_e56.c | 3773 +++++++++++++++++++++
drivers/net/txgbe/base/txgbe_e56.h | 1753 ++++++++++
drivers/net/txgbe/base/txgbe_e56_bp.c | 2597 ++++++++++++++
drivers/net/txgbe/base/txgbe_e56_bp.h | 282 ++
drivers/net/txgbe/base/txgbe_hw.c | 54 +-
drivers/net/txgbe/base/txgbe_hw.h | 4 +-
drivers/net/txgbe/base/txgbe_osdep.h | 4 +
drivers/net/txgbe/base/txgbe_phy.c | 362 +-
drivers/net/txgbe/base/txgbe_phy.h | 45 +-
drivers/net/txgbe/base/txgbe_regs.h | 13 +-
drivers/net/txgbe/base/txgbe_type.h | 43 +-
drivers/net/txgbe/txgbe_ethdev.c | 458 ++-
drivers/net/txgbe/txgbe_ethdev.h | 7 +-
drivers/net/txgbe/txgbe_rxtx.c | 107 +-
drivers/net/txgbe/txgbe_rxtx.h | 36 +
drivers/net/txgbe/txgbe_rxtx_vec_common.h | 16 +-
26 files changed, 9445 insertions(+), 445 deletions(-)
create mode 100644 drivers/net/txgbe/base/txgbe_e56.c
create mode 100644 drivers/net/txgbe/base/txgbe_e56.h
create mode 100644 drivers/net/txgbe/base/txgbe_e56_bp.c
create mode 100644 drivers/net/txgbe/base/txgbe_e56_bp.h
--
2.21.0.windows.1
^ permalink raw reply
* Re: [PATCH v5 02/27] eal/common: add token concatenation macro
From: Bruce Richardson @ 2026-05-27 12:38 UTC (permalink / raw)
To: Anatoly Burakov; +Cc: dev
In-Reply-To: <57ed806b6c47aa9316bd5af3154535439d39cdf6.1779717943.git.anatoly.burakov@intel.com>
On Mon, May 25, 2026 at 03:06:21PM +0100, Anatoly Burakov wrote:
> Add `RTE_CONCAT()` macro to enable token concatenation with proper macro
> expansion.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> lib/eal/include/rte_common.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 0a356abae2..05e72e500e 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -918,6 +918,10 @@ __extension__ typedef uint64_t RTE_MARKER64[0];
> /** Take a macro value and get a string version of it */
> #define RTE_STR(x) _RTE_STR(x)
>
> +/** Concatenate two tokens after expanding macros in both. */
> +#define _RTE_CONCAT2(a, b) a ## b
> +#define RTE_CONCAT(a, b) _RTE_CONCAT2(a, b)
> +
> /**
> * ISO C helpers to modify format strings using variadic macros.
> * This is a replacement for the ", ## __VA_ARGS__" GNU extension.
> --
> 2.47.3
>
^ permalink raw reply
* [PATCH v5] net/mlx5: prepend implicit items in sync flow creation path
From: Maxime Peim @ 2026-05-27 10:35 UTC (permalink / raw)
To: dev; +Cc: dsosnowski, viacheslavo, bingz, orika, suanmingm, matan
In-Reply-To: <20260511150854.1398044-1-maxime.peim@gmail.com>
In eSwitch mode, the async (template) flow creation path automatically
prepends implicit match items to scope flow rules to the correct
representor port:
- Ingress: REPRESENTED_PORT item matching dev->data->port_id
- Egress: REG_C_0 TAG item matching the port's tx tag value
The sync path (flow_hw_list_create) was missing this logic, causing all
flow rules created via the non-template API to match traffic from all
ports rather than being scoped to the specific representor.
Add the same implicit item prepending to flow_hw_list_create, right
after pattern validation and before any branching (sample/RSS/single/
prefix), mirroring the behavior of flow_hw_pattern_template_create
and flow_hw_get_rule_items. The ingress case prepends
REPRESENTED_PORT with the current port_id; the egress case prepends
MLX5_RTE_FLOW_ITEM_TYPE_TAG with REG_C_0 value/mask (skipped when
user provides an explicit SQ item).
Also fix a pre-existing bug where 'return split' on metadata split
failure returned a negative int cast to uintptr_t, which callers
would treat as a valid flow handle instead of an error.
Fixes: e38776c36c8a ("net/mlx5: introduce HWS for non-template flow API")
Fixes: 821a6a5cc495 ("net/mlx5: add metadata split for compatibility")
Signed-off-by: Maxime Peim <maxime.peim@gmail.com>
---
v3:
- Factor the implicit-item prepend logic out of
flow_hw_pattern_template_create() into a new helper
flow_hw_adjust_pattern() and reuse it from flow_hw_list_create(),
instead of duplicating the prepend logic inline in the sync path.
- Zero-initialize item_flags in both callers. The validator is
read-modify-write on item_flags (reads MLX5_FLOW_LAYER_TUNNEL on
the first iteration), so leaving it uninitialized was UB.
- Call __flow_hw_pattern_validate() with nt_flow=true from the sync
path (was effectively nt_flow=false via the wrapper), restoring the
previous behavior that skips GENEVE_OPT TLV parser validation on
the non-template path.
- Document flow_hw_adjust_pattern(): the dual role of the nt_flow
parameter (template spec-left-zero vs. sync spec-filled + validator
flag), the three-way return, and the caller's ownership of
*copied_items across every exit path.
- Clarify the "omitting implicit REG_C_0 match" debug log now that
the helper runs on both the template and sync paths.
- Add Fixes: tags for the two original commits.
v4:
- Fix items in case splitted metadata are not needed.
v5:
- Make flow_hw_prepend_item() return a self-contained array. The
helper used to shallow-copy the prepended item, leaving its
.spec/.mask pointing at flow_hw_adjust_pattern()'s stack locals
(port_spec, tag_v, tag_m); once that frame returned, the
consumers in flow_hw_list_create() (sample / RSS / single create)
and the post-extraction template path dereferenced dangling
pointers. The prepended item is now deep-copied via
rte_flow_conv(RTE_FLOW_CONV_OP_ITEM, ...) into the tail of the
same mlx5_malloc() block, so the lifetime of every byte the
consumer can reach equals the lifetime of the returned array.
items[] continue to be shallow-copied (their spec/mask blobs are
application-owned and outlive the call). One alloc, one free; no
call-site or signature changes.
- Fix the &item_flags / &orig_item_nb argument order at both
flow_hw_adjust_pattern() call sites (introduced in v3 by the
helper extraction): the prior order silently stored the item
count into it->item_flags / the layer-flag arguments forwarded
into mlx5_nta_sample_flow_list_create / mlx5_flow_nta_handle_rss /
mlx5_flow_hw_create_flow, and stored the OR-accumulated layer
flags into it->orig_item_nb / per-rule item-count uses.
drivers/net/mlx5/mlx5_flow_hw.c | 262 +++++++++++++++++++++++---------
1 file changed, 194 insertions(+), 68 deletions(-)
diff --git a/drivers/net/mlx5/mlx5_flow_hw.c b/drivers/net/mlx5/mlx5_flow_hw.c
index bca5b2769e..b8974577a3 100644
--- a/drivers/net/mlx5/mlx5_flow_hw.c
+++ b/drivers/net/mlx5/mlx5_flow_hw.c
@@ -8386,6 +8386,34 @@ flow_hw_actions_template_destroy(struct rte_eth_dev *dev,
return 0;
}
+/*
+ * Build a new item array prefixed with @p new_item.
+ *
+ * Callers typically build @p new_item on their stack with .spec / .mask
+ * pointing at other stack locals, so a shallow copy of @p new_item would
+ * leave dangling pointers as soon as the caller's frame goes away.
+ *
+ * To make the returned array self-contained, deep-copy @p new_item (item
+ * header + spec/mask/last payloads) into the tail of the same allocation
+ * via rte_flow_conv(RTE_FLOW_CONV_OP_ITEM, ...). The remaining items[]
+ * are still shallow-copied: they reference the application-owned spec/
+ * mask blobs handed to rte_flow_create() / rte_flow_pattern_template_
+ * create(), whose lifetime already covers the returned array's use.
+ *
+ * Layout of the single mlx5_malloc() block:
+ *
+ * +--------------------------------------+ <- returned pointer
+ * | rte_flow_item[nb_items + 1] | slot 0: copy of the deep-
+ * | | copied header below
+ * | | slots 1..nb_items: shallow
+ * | | copies of items[]
+ * +--------------------------------------+ <- new_item_buf
+ * | rte_flow_item header (deep copy) | written by rte_flow_conv;
+ * | + spec / mask / last payloads, | header.spec/.mask/.last point
+ * | contiguous, self-referential | into the payloads right below
+ * +--------------------------------------+
+ *
+ */
static struct rte_flow_item *
flow_hw_prepend_item(const struct rte_flow_item *items,
const uint32_t nb_items,
@@ -8393,11 +8421,24 @@ flow_hw_prepend_item(const struct rte_flow_item *items,
struct rte_flow_error *error)
{
struct rte_flow_item *copied_items;
- size_t size;
+ size_t header_size;
+ size_t total_size;
+ void *new_item_buf;
+ int new_item_size;
+ int rc;
- /* Allocate new array of items. */
- size = sizeof(*copied_items) * (nb_items + 1);
- copied_items = mlx5_malloc(MLX5_MEM_ZERO, size, 0, SOCKET_ID_ANY);
+ /*
+ * Size the deep copy of the prepended item (header + payloads).
+ * On error, rte_flow_conv() already populates @p error.
+ */
+ new_item_size = rte_flow_conv(RTE_FLOW_CONV_OP_ITEM, NULL, 0, new_item, error);
+ if (new_item_size < 0)
+ return NULL;
+
+ header_size = sizeof(*copied_items) * (nb_items + 1);
+ total_size = header_size + (size_t)new_item_size;
+
+ copied_items = mlx5_malloc(MLX5_MEM_ZERO, total_size, 0, SOCKET_ID_ANY);
if (!copied_items) {
rte_flow_error_set(error, ENOMEM,
RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
@@ -8405,8 +8446,23 @@ flow_hw_prepend_item(const struct rte_flow_item *items,
"cannot allocate item template");
return NULL;
}
- /* Put new item at the beginning and copy the rest. */
- copied_items[0] = *new_item;
+
+ /* Deep-copy the prepended item into the tail region. */
+ new_item_buf = (char *)copied_items + header_size;
+ rc = rte_flow_conv(RTE_FLOW_CONV_OP_ITEM, new_item_buf, (size_t)new_item_size, new_item,
+ error);
+ if (rc < 0) {
+ mlx5_free(copied_items);
+ return NULL;
+ }
+
+ /*
+ * Slot 0 is the prepended item header just written by rte_flow_conv,
+ * with .spec/.mask/.last already pointing inside the same buffer.
+ * The remaining items[] retain their (caller-owned, longer-lived)
+ * spec/mask pointers via shallow copy.
+ */
+ copied_items[0] = *(const struct rte_flow_item *)new_item_buf;
rte_memcpy(&copied_items[1], items, sizeof(*items) * nb_items);
return copied_items;
}
@@ -9255,33 +9311,40 @@ pattern_template_validate(struct rte_eth_dev *dev,
return -ret;
}
-/**
- * Create flow item template.
+/*
+ * Validate the user-supplied items and, in eSwitch mode, prepend the implicit
+ * scoping item so the rule/template is bound to the current representor port:
+ * - ingress -> RTE_FLOW_ITEM_TYPE_REPRESENTED_PORT (dev->data->port_id)
+ * - egress -> MLX5_RTE_FLOW_ITEM_TYPE_TAG on REG_C_0 (tx vport tag),
+ * skipped when the user already supplied an SQ item.
*
- * @param[in] dev
- * Pointer to the rte_eth_dev structure.
- * @param[in] attr
- * Pointer to the item template attributes.
- * @param[in] items
- * The template item pattern.
- * @param[out] error
- * Pointer to error structure.
+ * @param nt_flow
+ * Selects between the two call paths that share this helper:
+ * false -> pattern template creation (async API). The prepended item's
+ * spec is left zeroed so mlx5dr matches any value; the live
+ * port_id / tx-tag value is substituted later by
+ * flow_hw_get_rule_items() at rule-create time.
+ * true -> sync (non-template) flow creation. The prepended item's spec
+ * is filled immediately with the live values, and the flag is
+ * forwarded to __flow_hw_pattern_validate() so that validation
+ * paths gated on nt_flow (e.g. GENEVE_OPT TLV parser creation)
+ * take the non-template branch.
*
- * @return
- * Item template pointer on success, NULL otherwise and rte_errno is set.
+ * Return / ownership:
+ * - NULL on validation or allocation failure (error populated).
+ * - `items` unchanged when no prepending is required; *copied_items == NULL.
+ * - A newly-allocated array otherwise; also stored in *copied_items. The
+ * caller must mlx5_free(*copied_items) on every path (it is safe to call
+ * with NULL). Do not free the returned pointer directly.
*/
-static struct rte_flow_pattern_template *
-flow_hw_pattern_template_create(struct rte_eth_dev *dev,
- const struct rte_flow_pattern_template_attr *attr,
- const struct rte_flow_item items[],
- bool external,
- struct rte_flow_error *error)
+static const struct rte_flow_item *
+flow_hw_adjust_pattern(struct rte_eth_dev *dev, const struct rte_flow_pattern_template_attr *attr,
+ bool nt_flow, const struct rte_flow_item *items, uint64_t *item_flags,
+ uint64_t *nb_items, struct rte_flow_item **copied_items,
+ struct rte_flow_error *error)
{
struct mlx5_priv *priv = dev->data->dev_private;
- struct rte_flow_pattern_template *it;
- struct rte_flow_item *copied_items = NULL;
- const struct rte_flow_item *tmpl_items;
- uint64_t orig_item_nb, item_flags = 0;
+ struct rte_flow_item_ethdev port_spec = {.port_id = dev->data->port_id};
struct rte_flow_item port = {
.type = RTE_FLOW_ITEM_TYPE_REPRESENTED_PORT,
.mask = &rte_flow_item_ethdev_mask,
@@ -9298,39 +9361,89 @@ flow_hw_pattern_template_create(struct rte_eth_dev *dev,
.type = (enum rte_flow_item_type)MLX5_RTE_FLOW_ITEM_TYPE_TAG,
.spec = &tag_v,
.mask = &tag_m,
- .last = NULL
+ .last = NULL,
};
- int it_items_size;
- unsigned int i = 0;
int rc;
+ if (!copied_items || !item_flags || !nb_items)
+ return NULL;
+
+ if (nt_flow) {
+ port.spec = &port_spec;
+ tag_v.data = flow_hw_tx_tag_regc_value(dev);
+ }
+
+ /*
+ * item_flags must be zero-initialized: __flow_hw_pattern_validate()
+ * OR-accumulates bits into it and reads it (MLX5_FLOW_LAYER_TUNNEL)
+ * on the first iteration.
+ */
+ *item_flags = 0;
+
/* Validate application items only */
- rc = flow_hw_pattern_validate(dev, attr, items, &item_flags, error);
+ rc = __flow_hw_pattern_validate(dev, attr, items, item_flags, nt_flow, error);
if (rc < 0)
return NULL;
- orig_item_nb = rc;
- if (priv->sh->config.dv_esw_en &&
- attr->ingress && !attr->egress && !attr->transfer) {
- copied_items = flow_hw_prepend_item(items, orig_item_nb, &port, error);
- if (!copied_items)
+ *nb_items = rc;
+
+ if (priv->sh->config.dv_esw_en && attr->ingress && !attr->egress && !attr->transfer) {
+ *copied_items = flow_hw_prepend_item(items, *nb_items, &port, error);
+ if (!*copied_items)
return NULL;
- tmpl_items = copied_items;
- } else if (priv->sh->config.dv_esw_en &&
- !attr->ingress && attr->egress && !attr->transfer) {
- if (item_flags & MLX5_FLOW_ITEM_SQ) {
- DRV_LOG(DEBUG, "Port %u omitting implicit REG_C_0 match for egress "
- "pattern template", dev->data->port_id);
- tmpl_items = items;
- goto setup_pattern_template;
+ return *copied_items;
+ } else if (priv->sh->config.dv_esw_en && !attr->ingress && attr->egress &&
+ !attr->transfer) {
+ if (*item_flags & MLX5_FLOW_ITEM_SQ) {
+ DRV_LOG(DEBUG,
+ "Port %u: explicit SQ item present, omitting implicit "
+ "REG_C_0 match for egress pattern",
+ dev->data->port_id);
+ return items;
}
- copied_items = flow_hw_prepend_item(items, orig_item_nb, &tag, error);
- if (!copied_items)
+ *copied_items = flow_hw_prepend_item(items, *nb_items, &tag, error);
+ if (!*copied_items)
return NULL;
- tmpl_items = copied_items;
- } else {
- tmpl_items = items;
+ return *copied_items;
}
-setup_pattern_template:
+ return items;
+}
+
+/**
+ * Create flow item template.
+ *
+ * @param[in] dev
+ * Pointer to the rte_eth_dev structure.
+ * @param[in] attr
+ * Pointer to the item template attributes.
+ * @param[in] items
+ * The template item pattern.
+ * @param[out] error
+ * Pointer to error structure.
+ *
+ * @return
+ * Item template pointer on success, NULL otherwise and rte_errno is set.
+ */
+static struct rte_flow_pattern_template *
+flow_hw_pattern_template_create(struct rte_eth_dev *dev,
+ const struct rte_flow_pattern_template_attr *attr,
+ const struct rte_flow_item items[],
+ bool external,
+ struct rte_flow_error *error)
+{
+ struct mlx5_priv *priv = dev->data->dev_private;
+ struct rte_flow_pattern_template *it;
+ struct rte_flow_item *copied_items = NULL;
+ const struct rte_flow_item *tmpl_items;
+ int it_items_size;
+ uint64_t orig_item_nb, item_flags;
+ unsigned int i = 0;
+ int rc;
+
+ tmpl_items = flow_hw_adjust_pattern(dev, attr, false, items, &item_flags, &orig_item_nb,
+ &copied_items, error);
+ if (!tmpl_items)
+ return NULL;
+
it = mlx5_malloc(MLX5_MEM_ZERO, sizeof(*it), 0, SOCKET_ID_ANY);
if (!it) {
rte_flow_error_set(error, ENOMEM,
@@ -14272,7 +14385,6 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
struct rte_flow_hw *prfx_flow = NULL;
const struct rte_flow_action *qrss = NULL;
const struct rte_flow_action *mark = NULL;
- uint64_t item_flags = 0;
uint64_t action_flags = mlx5_flow_hw_action_flags_get(actions, &qrss, &mark,
&encap_idx, &actions_n, error);
struct mlx5_flow_hw_split_resource resource = {
@@ -14289,20 +14401,27 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
.egress = attr->egress,
.transfer = attr->transfer,
};
-
- /* Validate application items only */
- ret = __flow_hw_pattern_validate(dev, &pattern_template_attr, items,
- &item_flags, true, error);
- if (ret < 0)
- return 0;
+ struct rte_flow_item *copied_items = NULL;
+ const struct rte_flow_item *prepend_items;
+ uint64_t orig_item_nb, item_flags;
RTE_SET_USED(encap_idx);
if (!error)
error = &shadow_error;
+
+ prepend_items = flow_hw_adjust_pattern(dev, &pattern_template_attr, true, items,
+ &item_flags, &orig_item_nb, &copied_items, error);
+ if (!prepend_items)
+ return 0;
+
split = mlx5_flow_nta_split_metadata(dev, attr, actions, qrss, action_flags,
actions_n, external, &resource, error);
- if (split < 0)
- return split;
+ if (split < 0) {
+ mlx5_free(copied_items);
+ return 0;
+ } else if (!split) {
+ resource.suffix.items = prepend_items;
+ }
/* Update the metadata copy table - MLX5_FLOW_MREG_CP_TABLE_GROUP */
if (((attr->ingress && attr->group != MLX5_FLOW_MREG_CP_TABLE_GROUP) ||
@@ -14313,23 +14432,26 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
goto free;
}
if (action_flags & MLX5_FLOW_ACTION_SAMPLE) {
- flow = mlx5_nta_sample_flow_list_create(dev, type, attr, items, actions,
+ flow = mlx5_nta_sample_flow_list_create(dev, type, attr, prepend_items, actions,
item_flags, action_flags, error);
- if (flow != NULL)
+ if (flow != NULL) {
+ mlx5_free(copied_items);
return (uintptr_t)flow;
+ }
goto free;
}
if (action_flags & MLX5_FLOW_ACTION_RSS) {
const struct rte_flow_action_rss
*rss_conf = mlx5_flow_nta_locate_rss(dev, actions, error);
- flow = mlx5_flow_nta_handle_rss(dev, attr, items, actions, rss_conf,
- item_flags, action_flags, external,
- type, error);
+ flow = mlx5_flow_nta_handle_rss(dev, attr, prepend_items, actions, rss_conf,
+ item_flags, action_flags, external, type, error);
if (flow) {
flow->nt2hws->rix_mreg_copy = cpy_idx;
cpy_idx = 0;
- if (!split)
+ if (!split) {
+ mlx5_free(copied_items);
return (uintptr_t)flow;
+ }
goto prefix_flow;
}
goto free;
@@ -14343,12 +14465,14 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
if (flow) {
flow->nt2hws->rix_mreg_copy = cpy_idx;
cpy_idx = 0;
- if (!split)
+ if (!split) {
+ mlx5_free(copied_items);
return (uintptr_t)flow;
+ }
/* Fall Through to prefix flow creation. */
}
prefix_flow:
- ret = mlx5_flow_hw_create_flow(dev, type, attr, items, resource.prefix.actions,
+ ret = mlx5_flow_hw_create_flow(dev, type, attr, prepend_items, resource.prefix.actions,
item_flags, action_flags, external, &prfx_flow, error);
if (ret)
goto free;
@@ -14357,6 +14481,7 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
flow->nt2hws->chaned_flow = 1;
SLIST_INSERT_AFTER(prfx_flow, flow, nt2hws->next);
mlx5_flow_nta_split_resource_free(dev, &resource);
+ mlx5_free(copied_items);
return (uintptr_t)prfx_flow;
}
free:
@@ -14368,6 +14493,7 @@ static uintptr_t flow_hw_list_create(struct rte_eth_dev *dev,
mlx5_flow_nta_del_copy_action(dev, cpy_idx);
if (split > 0)
mlx5_flow_nta_split_resource_free(dev, &resource);
+ mlx5_free(copied_items);
return 0;
}
--
2.43.0
^ permalink raw reply related
* RE: [RFC v2 2/3] lib: add fastmem library
From: Morten Brørup @ 2026-05-27 12:03 UTC (permalink / raw)
To: Mattias Rönnblom, Bruce Richardson
Cc: Stephen Hemminger, dev, Konstantin Ananyev, Mattias Rönnblom,
Yogaraj Baskaravel
In-Reply-To: <4b9f4f80-3bbf-490d-b39e-c40930d653fd@lysator.liu.se>
> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Wednesday, 27 May 2026 13.30
>
> On 5/27/26 13:17, Morten Brørup wrote:
> >> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> >> Sent: Wednesday, 27 May 2026 12.18
> >>
> >> On Wed, May 27, 2026 at 12:12:19PM +0200, Mattias Rönnblom wrote:
> >>> On 5/26/26 15:23, Stephen Hemminger wrote:
> >>>> On Tue, 26 May 2026 10:57:42 +0200
> >>>> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> >>>>
> >>>>> +__rte_experimental
> >>>>> +void *
> >>>>> +rte_fastmem_alloc(size_t size, size_t align, unsigned int flags)
> >>>>> + __rte_alloc_size(1) __rte_alloc_align(2);
> >>>>
> >>>> Should also add attribute __rte_malloc which tells compiler
> >>>> that pointer returned cannot alias other memory
> >>>>
> >>>> And add __rte_dealloc(rte_fastmem_free, 1)
> >>>> which tells compiler that the returned pointer should only go
> >>>> back to fastmem (not free, rte_free, etc).
> >>>
> >>> Done. Only works for the single-object ops (not bulk) though.
> >>>
> >>> I've had a look at how to extend fastmem to support larger
> >> allocations
> >>> (without suggesting this is the way to go).
> >>>
> >>> Seems to me that implementation should be something like
> >>> a) slab allocator for small objects.
> >>> b) a cache-less per-socket page run allocator for mid-sized
> objects.
> >>> c) per-object memzones for large objects.
> >
> > All allocations through the library should somehow build on memzones.
> > If an application wants "normal" memory, it can use libc
> free()/malloc().
> >
>
> All allocations come out of memzones. In the c) case, each object is a
> memzone. There is a very limited number of memzones, so they can't be
> many.
>
> >>>
> >>> If one would implement that, you would essentially have a plug-in
> >>> replacement for rte_malloc.h (maybe minus some debug and some more
> >> esoteric
> >>> DPDK heap features).
> >>>
> >>> Should fastmem be an outright replacement, or something that at
> least
> >>> initially lives alongside the regular heap, maybe with a run- or
> >>> compile-time option to make rte_malloc.h functions delegate to
> >> fastmem? This
> >>> is unclear to me at this point. I fear the more ambitious, cleaner
> >> and more
> >>> risky DPDK heap replacement path will go they way my attempts to
> >> replace
> >>> rte_memcpy or rte_timer went.
> >>>
> >>> I would agree with anyone saying that we should have only one heap-
> >> like API
> >>> for memory allocations. rte_malloc.h obviously needs to stay, for
> >> backward
> >>> compatibility reason, if nothing else. I would like to add bulk
> >> alloc/free,
> >>> and allow for smaller alignments than 64, since slabs can do that
> >>> efficiently (DPDK heap per-object header is 128 bytes!). One could
> >> either go
> >>> about that by extending rte_malloc.h or deprecating that API and
> >> starting
> >>> anew. In the latter case, one could do many more minor tweaks, like
> >> removing
> >>> the type pointers (only a nuance), remove the validate function,
> >> change and
> >>> extend the stats interface, etc.
> >>>
> >> +1 for replacing rte_malloc. For a replacement, I'd tend towards
> aiming
> >> for
> >> compatibilty over trying to fix too many little things at once.
> While
> >> changing a couple of things is ok, I'd rather not force applications
> to
> >> make too many updates to their code when moving from one DPDK
> version
> >> to
> >> another.
> >>
> >> /Bruce
> >
> > +1 for taking the path that begins with starting anew.
> > The new heap library can be designed specifically for the fast path,
> based on years of DPDK experience with what the fast path really needs.
> > It will allow much broader experimentation along the way.
> > We can easily get rid of all slow path legacy stuff in rte_malloc,
> like the "type" parameter.
> >
> > I think we all agree that replacing rte_malloc should be the end
> game.
> > But IMO, it very important that the properties of rte_malloc do not
> impose any limits on the new heap library.
> >
> > Someday, when the new library has sufficiently matured, we can
> discuss how it can replace the rte_malloc library.
> > Maybe some parts of rte_malloc are not replaceable, and need to be
> deprecated and removed.
> > Maybe we all have switched to using the new heap, and nobody is using
> rte_malloc anymore, so it can simply be removed. ;-)
> >
>
> It would be useful to have a run- or compile-time switch to use fastmem
> instead of the regular DPDK heap. In such a case, having it as an
> external library will complicate things during initialization, if you
> want all allocations to end up in fastmem.
IMHO, there is no need for a runtime (or startup time) switch; a build time configuration option suffices.
And it doesn't need to be there until we reach the point where we start looking into replacing rte_malloc with fastmem.
Putting it in too early might lead us to converging too early with rte_malloc, reducing the ability to experiment and further improve the library.
>
> Unfortunately, if a rte_malloc replacement, I think it belongs in the
> EAL, just like today's heap. That doesn't mean it have to start in the
> EAL.
The EAL is bloated, so it would be nice if rte_malloc could somehow be moved out of the EAL, perhaps only partially.
If that is impossible, then yes, fastmem would probably have to live in the EAL too.
Good point that fastmem doesn't have to start in the EAL.
But if further investigation shows that rte_malloc is too deeply integrated with the EAL to be moved out, fastmem might as well start its life in the EAL too.
It might save us from some headache at the point in time where we move it from a separate library into the EAL.
>
> > Suggestion regarding naming:
> > The prefix could be "rte_mem_" instead of "rte_fastmem_" - it is
> shorter, and most libraries are "fast".
> > And then it could live in /lib/memory instead of /lib/fastmem.
> >
>
> mem is very generic, but point taken. Name should change, unclear to
> what. Depends on the role it will serve.
^ permalink raw reply
* RE: [PATCH v2] app/test-pmd: add generic PROG action parser support
From: Konstantin Ananyev @ 2026-05-27 11:51 UTC (permalink / raw)
To: Ajmera, Megha, Stephen Hemminger
Cc: Richardson, Bruce, Dumitrescu, Cristian, Shetty, Praveen,
Singh, Aman Deep, dev@dpdk.org
In-Reply-To: <MW4PR11MB5892D90B2AF860214FB57F4D97082@MW4PR11MB5892.namprd11.prod.outlook.com>
> >
> > This looks like a third attempt to parse text into rte_flow.
> > Not sure how this fits in and why it would be useful?
>
> Thanks for the feedback.
> I wanted to clarify the intent of this patch. The backend support for the PROG
> action already exists in the PMD. This patch is focused on adding the
> corresponding frontend (testpmd) parser support so that users can configure
> and exercise this functionality via CLI.
> The motivation here is to enable configuration of programmable actions that are
> already supported in the backend but cannot currently be invoked through
> testpmd. With this addition, new actions that are exposed through PROG can be
> configured using the flow command interface.
For my own curiosity: how user can define his own PROG action?
Is it supposed to be programmed and uplodaded to the NIC by some external tool (P4 compiler)?
Or does it refer to the set of some predefined functions that given firmware supports?
Or ... ?
Sorry for probably naive questions, but I found is nearly zero information inside DPDK docs about
how PROG action supposed to work.
Thanks
Konstantin
^ permalink raw reply
* RE: [PATCH v4 12/20] net/txgbe: fix link stability for 25G NIC
From: Zaiyu Wang @ 2026-05-27 11:45 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517164936.1c83977e@phoenix.local>
> ## Errors
>
> ### 1. Use-after-free potential in txgbe_e56_rxs_calib_adapt_seq (CORRECTNESS)
No code change.
The return value of txgbe_e56_rxs_osc_init_for_temp_track_range is only used for debugging; the hardware can still continue the subsequent configuration flow even after a timeout. Therefore, we use status |= to record the error for logging but do not abort the sequence.
> ### 2. Resource leak: lock not released on error path (CORRECTNESS)
No code change.
The spinlock is correctly released on all paths, including the TXGBE_ERR_PHY_INIT_NOT_DONE branch. No change is needed.
> ### 3. Missing error propagation to caller (CORRECTNESS)
Fixed. The function now returns TXGBE_ERR_TIMEOUT when the link fails to come up.
> ### 4. Double error return value overwrite (CORRECTNESS)
Fixed by removing the break statements and letting the timeout errors return immediately.
> ### 5. Error code dropped without propagation (CORRECTNESS)
No code change.
A failed temperature read does not affect the subsequent configuration flow; stopping the process would be too aggressive. The function intentionally continues, and the error is only for debug purposes.
> ### 6. Integer overflow in bit shift (CORRECTNESS)
Fixed
> ### 7. Missing bounds check on array size calculation (CORRECTNESS)
Fixed by introducing the RXS_READ_COUNT macro to keep the array size, loop bound, and median calculation always consistent.
> ## Warnings
>
> ### 1. Variables declared but may be read before initialization (STYLE)
Adjusted the declaration of need_reset to the function scope for improved readability.
> ### 2. Missing documentation for complex algorithm (STYLE)
Acknowledged. Adding a high‑level comment for the complex calibration procedure is planned
> ### 3. Magic numbers without named constants (STYLE)
No code change.
These values come directly from the hardware vendor and are located in the base/ layer, which we prefer not to modify without an official update. We will keep them as‑is.
> ## Info
>
> ### 1. Deep nesting in calibration sequence
Considering that this is the hardware configuration code located in base/, for now we will not make change to it.
> ### 2. Possible candidate for helper function
Considering that this is the hardware configuration code located in base/, for now we will not make change to it.
^ permalink raw reply
* RE: [PATCH v4 14/20] net/txgbe: fix link stability for Amber-Lite backplane mode
From: Zaiyu Wang @ 2026-05-27 11:45 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517165032.059933d3@phoenix.local>
> Lots of AI feedback here:
>
>
> # Review of DPDK Patch: net/txgbe: fix link stability for 40G NIC
>
> ## Summary
> This patch re-adds direct PHY configuration flow to the txgbe driver to fix unstable link behavior on
> 40G NICs. The previous firmware-based configuration approach was causing link stability issues.
>
> ## ERRORS
>
> ### 1. Use-after-free risk in `txgbe_setup_phy_link_aml40`
Fixed.
> ### 2. Missing error propagation in `txgbe_e56_rx_rd_second_code_40g`
Fixed. This function has been changed to void type, as its configuration flow does not produce any error return value under normal operation.
> ### 3. Missing bounds check before array access
Fixed by introducing the RXS_READ_COUNT macro to keep the array size, loop bound, and median calculation always consistent.
> ### 4. Timeout return without cleanup in `txgbe_e56_rxs_calib_adapt_seq_40G`
Not a error.
> ## WARNINGS
>
> ### 1. Hardcoded timeout in multiple locations
Added a comment at the definition of PHYINIT_TIMEOUT clarifying that it is a loop iteration limit, not a fixed time unit. The actual timeout duration depends on the usec_delay() or msleep() value used inside the polling loop.
> ### 2. Potentially unreachable code after loop
Moved the comment before '}'.
> ### 4. Variable `bypass_ctle` hardcoded but declared as variable
Fixed.
> ### 5. Missing validation of speed parameter in initialization functions
Fixed.
> ## INFORMATIONAL
>
> ### 1. Large function complexity
> ### 2. Magic numbers without symbolic constants
> ### 3. Duplicated initialization sequences The 40G initialization in `txgbe_e56_cfg_40g`
I prefer not to split the hardware configuration flow at this stage. The current structure closely follows the hardware vendor's recommended initialization sequence.
Most magic numbers are directly from the hardware specification, we do not change them at this stage.
The hardware register requirements for different speeds are not fully identical. Merging them would risk subtle bugs.
> ### 4. Temperature check frequency
This was indeed an omission after moving PHY configuration from firmware to the driver. Previously, temperature tracking was handled by firmware together with hardware setup. We have now added a new patch that implements periodic temperature tracking in the driver. The tracking sequence is invoked every 100ms as recommended. This patch will be submitted in the new version of this patchset.
^ permalink raw reply
* [PATCH v6] mempool/dpaa: update for new mempool cache algorithm
From: Morten Brørup @ 2026-05-27 11:36 UTC (permalink / raw)
To: dev, Andrew Rybchenko, Bruce Richardson, Jingjing Wu,
Praveen Shetty, Hemant Agrawal, Sachin Saxena
Cc: Morten Brørup
In-Reply-To: <20260527113644.222538-1-mb@smartsharesystems.com>
As a consequence of the improved mempool cache algorithm, the mempool
driver was updated to not modify the mempool cache's flushthresh field,
which is now obsolete, and modifying it has no effect.
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
Depends-on: patch-164427 ("mempool: improve cache behaviour and performance")
---
drivers/mempool/dpaa/dpaa_mempool.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/mempool/dpaa/dpaa_mempool.c b/drivers/mempool/dpaa/dpaa_mempool.c
index 2f9395b3f4..2f8555a026 100644
--- a/drivers/mempool/dpaa/dpaa_mempool.c
+++ b/drivers/mempool/dpaa/dpaa_mempool.c
@@ -58,8 +58,6 @@ dpaa_mbuf_create_pool(struct rte_mempool *mp)
struct bman_pool_params params = {
.flags = BMAN_POOL_FLAG_DYNAMIC_BPID
};
- unsigned int lcore_id;
- struct rte_mempool_cache *cache;
MEMPOOL_INIT_FUNC_TRACE();
@@ -129,18 +127,6 @@ dpaa_mbuf_create_pool(struct rte_mempool *mp)
rte_memcpy(bp_info, (void *)&rte_dpaa_bpid_info[bpid],
sizeof(struct dpaa_bp_info));
mp->pool_data = (void *)bp_info;
- /* Update per core mempool cache threshold to optimal value which is
- * number of buffers that can be released to HW buffer pool in
- * a single API call.
- */
- for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
- cache = &mp->local_cache[lcore_id];
- DPAA_MEMPOOL_DEBUG("lCore %d: cache->flushthresh %d -> %d",
- lcore_id, cache->flushthresh,
- (uint32_t)(cache->size + DPAA_MBUF_MAX_ACQ_REL));
- if (cache->flushthresh)
- cache->flushthresh = cache->size + DPAA_MBUF_MAX_ACQ_REL;
- }
DPAA_MEMPOOL_INFO("BMAN pool created for bpid =%d", bpid);
return 0;
--
2.43.0
^ permalink raw reply related
* [PATCH v6] mempool/dpaa2: update for new mempool cache algorithm
From: Morten Brørup @ 2026-05-27 11:36 UTC (permalink / raw)
To: dev, Andrew Rybchenko, Bruce Richardson, Jingjing Wu,
Praveen Shetty, Hemant Agrawal, Sachin Saxena
Cc: Morten Brørup
In-Reply-To: <20260527113644.222538-1-mb@smartsharesystems.com>
As a consequence of the improved mempool cache algorithm, the mempool
driver was updated to not modify the mempool cache's flushthresh field,
which is now obsolete, and modifying it has no effect.
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
Depends-on: patch-164427 ("mempool: improve cache behaviour and performance")
---
drivers/mempool/dpaa2/dpaa2_hw_mempool.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
index 02b6741853..ee001d8ce0 100644
--- a/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
+++ b/drivers/mempool/dpaa2/dpaa2_hw_mempool.c
@@ -54,8 +54,6 @@ rte_hw_mbuf_create_pool(struct rte_mempool *mp)
struct dpaa2_bp_info *bp_info;
struct dpbp_attr dpbp_attr;
uint32_t bpid;
- unsigned int lcore_id;
- struct rte_mempool_cache *cache;
int ret;
avail_dpbp = dpaa2_alloc_dpbp_dev();
@@ -152,18 +150,6 @@ rte_hw_mbuf_create_pool(struct rte_mempool *mp)
DPAA2_MEMPOOL_DEBUG("BP List created for bpid =%d", dpbp_attr.bpid);
h_bp_list = bp_list;
- /* Update per core mempool cache threshold to optimal value which is
- * number of buffers that can be released to HW buffer pool in
- * a single API call.
- */
- for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
- cache = &mp->local_cache[lcore_id];
- DPAA2_MEMPOOL_DEBUG("lCore %d: cache->flushthresh %d -> %d",
- lcore_id, cache->flushthresh,
- (uint32_t)(cache->size + DPAA2_MBUF_MAX_ACQ_REL));
- if (cache->flushthresh)
- cache->flushthresh = cache->size + DPAA2_MBUF_MAX_ACQ_REL;
- }
return 0;
err4:
--
2.43.0
^ permalink raw reply related
* [PATCH v6] net/idpf: update for new mempool cache algorithm
From: Morten Brørup @ 2026-05-27 11:36 UTC (permalink / raw)
To: dev, Andrew Rybchenko, Bruce Richardson, Jingjing Wu,
Praveen Shetty, Hemant Agrawal, Sachin Saxena
Cc: Morten Brørup
In-Reply-To: <20260408141315.904381-1-mb@smartsharesystems.com>
As a consequence of the improved mempool cache algorithm, the PMD was
updated regarding how much to backfill the mempool cache in the AVX512
code path.
Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
Depends-on: patch-164427 ("mempool: improve cache behaviour and performance")
---
.../net/intel/idpf/idpf_common_rxtx_avx512.c | 52 +++++++++++++++----
1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c b/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c
index 9af275cd9d..dd2263b8d7 100644
--- a/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c
+++ b/drivers/net/intel/idpf/idpf_common_rxtx_avx512.c
@@ -148,15 +148,31 @@ idpf_singleq_rearm(struct idpf_rx_queue *rxq)
/* Can this be satisfied from the cache? */
if (cache->len < IDPF_RXQ_REARM_THRESH) {
/* No. Backfill the cache first, and then fill from it */
- uint32_t req = IDPF_RXQ_REARM_THRESH + (cache->size -
- cache->len);
- /* How many do we require i.e. number to fill the cache + the request */
+ /* Backfill would exceed the cache bounce buffer limit? */
+ __rte_assume(cache->size / 2 <= RTE_MEMPOOL_CACHE_MAX_SIZE / 2);
+ if (unlikely(cache->size / 2 < IDPF_RXQ_REARM_THRESH)) {
+ idpf_singleq_rearm_common(rxq);
+ return;
+ }
+
+ /*
+ * Backfill the cache from the backend;
+ * move up the hot objects in the cache to the top half of the cache,
+ * and fetch (size / 2) objects to the bottom of the cache.
+ */
+ __rte_assume(cache->len < cache->size / 2);
+ rte_memcpy(&cache->objs[cache->size / 2], &cache->objs[0],
+ sizeof(void *) * cache->len);
int ret = rte_mempool_ops_dequeue_bulk
- (rxq->mp, &cache->objs[cache->len], req);
+ (rxq->mp, &cache->objs[0], cache->size / 2);
if (ret == 0) {
- cache->len += req;
+ cache->len += cache->size / 2;
} else {
+ /*
+ * No further action is required for roll back, as the objects moved
+ * in the cache were actually copied, and the cache remains intact.
+ */
if (rxq->rxrearm_nb + IDPF_RXQ_REARM_THRESH >=
rxq->nb_rx_desc) {
__m128i dma_addr0;
@@ -565,15 +581,31 @@ idpf_splitq_rearm(struct idpf_rx_queue *rx_bufq)
/* Can this be satisfied from the cache? */
if (cache->len < IDPF_RXQ_REARM_THRESH) {
/* No. Backfill the cache first, and then fill from it */
- uint32_t req = IDPF_RXQ_REARM_THRESH + (cache->size -
- cache->len);
- /* How many do we require i.e. number to fill the cache + the request */
+ /* Backfill would exceed the cache bounce buffer limit? */
+ __rte_assume(cache->size / 2 <= RTE_MEMPOOL_CACHE_MAX_SIZE / 2);
+ if (unlikely(cache->size / 2 < IDPF_RXQ_REARM_THRESH)) {
+ idpf_splitq_rearm_common(rx_bufq);
+ return;
+ }
+
+ /*
+ * Backfill the cache from the backend;
+ * move up the hot objects in the cache to the top half of the cache,
+ * and fetch (size / 2) objects to the bottom of the cache.
+ */
+ __rte_assume(cache->len < cache->size / 2);
+ rte_memcpy(&cache->objs[cache->size / 2], &cache->objs[0],
+ sizeof(void *) * cache->len);
int ret = rte_mempool_ops_dequeue_bulk
- (rx_bufq->mp, &cache->objs[cache->len], req);
+ (rx_bufq->mp, &cache->objs[0], cache->size / 2);
if (ret == 0) {
- cache->len += req;
+ cache->len += cache->size / 2;
} else {
+ /*
+ * No further action is required for roll back, as the objects moved
+ * in the cache were actually copied, and the cache remains intact.
+ */
if (rx_bufq->rxrearm_nb + IDPF_RXQ_REARM_THRESH >=
rx_bufq->nb_rx_desc) {
__m128i dma_addr0;
--
2.43.0
^ permalink raw reply related
* Re: [RFC v2 2/3] lib: add fastmem library
From: Mattias Rönnblom @ 2026-05-27 11:29 UTC (permalink / raw)
To: Morten Brørup, Bruce Richardson
Cc: Stephen Hemminger, dev, Konstantin Ananyev, Mattias Rönnblom,
Yogaraj Baskaravel
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35F658A1@smartserver.smartshare.dk>
On 5/27/26 13:17, Morten Brørup wrote:
>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
>> Sent: Wednesday, 27 May 2026 12.18
>>
>> On Wed, May 27, 2026 at 12:12:19PM +0200, Mattias Rönnblom wrote:
>>> On 5/26/26 15:23, Stephen Hemminger wrote:
>>>> On Tue, 26 May 2026 10:57:42 +0200
>>>> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>>>
>>>>> +__rte_experimental
>>>>> +void *
>>>>> +rte_fastmem_alloc(size_t size, size_t align, unsigned int flags)
>>>>> + __rte_alloc_size(1) __rte_alloc_align(2);
>>>>
>>>> Should also add attribute __rte_malloc which tells compiler
>>>> that pointer returned cannot alias other memory
>>>>
>>>> And add __rte_dealloc(rte_fastmem_free, 1)
>>>> which tells compiler that the returned pointer should only go
>>>> back to fastmem (not free, rte_free, etc).
>>>
>>> Done. Only works for the single-object ops (not bulk) though.
>>>
>>> I've had a look at how to extend fastmem to support larger
>> allocations
>>> (without suggesting this is the way to go).
>>>
>>> Seems to me that implementation should be something like
>>> a) slab allocator for small objects.
>>> b) a cache-less per-socket page run allocator for mid-sized objects.
>>> c) per-object memzones for large objects.
>
> All allocations through the library should somehow build on memzones.
> If an application wants "normal" memory, it can use libc free()/malloc().
>
All allocations come out of memzones. In the c) case, each object is a
memzone. There is a very limited number of memzones, so they can't be many.
>>>
>>> If one would implement that, you would essentially have a plug-in
>>> replacement for rte_malloc.h (maybe minus some debug and some more
>> esoteric
>>> DPDK heap features).
>>>
>>> Should fastmem be an outright replacement, or something that at least
>>> initially lives alongside the regular heap, maybe with a run- or
>>> compile-time option to make rte_malloc.h functions delegate to
>> fastmem? This
>>> is unclear to me at this point. I fear the more ambitious, cleaner
>> and more
>>> risky DPDK heap replacement path will go they way my attempts to
>> replace
>>> rte_memcpy or rte_timer went.
>>>
>>> I would agree with anyone saying that we should have only one heap-
>> like API
>>> for memory allocations. rte_malloc.h obviously needs to stay, for
>> backward
>>> compatibility reason, if nothing else. I would like to add bulk
>> alloc/free,
>>> and allow for smaller alignments than 64, since slabs can do that
>>> efficiently (DPDK heap per-object header is 128 bytes!). One could
>> either go
>>> about that by extending rte_malloc.h or deprecating that API and
>> starting
>>> anew. In the latter case, one could do many more minor tweaks, like
>> removing
>>> the type pointers (only a nuance), remove the validate function,
>> change and
>>> extend the stats interface, etc.
>>>
>> +1 for replacing rte_malloc. For a replacement, I'd tend towards aiming
>> for
>> compatibilty over trying to fix too many little things at once. While
>> changing a couple of things is ok, I'd rather not force applications to
>> make too many updates to their code when moving from one DPDK version
>> to
>> another.
>>
>> /Bruce
>
> +1 for taking the path that begins with starting anew.
> The new heap library can be designed specifically for the fast path, based on years of DPDK experience with what the fast path really needs.
> It will allow much broader experimentation along the way.
> We can easily get rid of all slow path legacy stuff in rte_malloc, like the "type" parameter.
>
> I think we all agree that replacing rte_malloc should be the end game.
> But IMO, it very important that the properties of rte_malloc do not impose any limits on the new heap library.
>
> Someday, when the new library has sufficiently matured, we can discuss how it can replace the rte_malloc library.
> Maybe some parts of rte_malloc are not replaceable, and need to be deprecated and removed.
> Maybe we all have switched to using the new heap, and nobody is using rte_malloc anymore, so it can simply be removed. ;-)
>
It would be useful to have a run- or compile-time switch to use fastmem
instead of the regular DPDK heap. In such a case, having it as an
external library will complicate things during initialization, if you
want all allocations to end up in fastmem.
Unfortunately, if a rte_malloc replacement, I think it belongs in the
EAL, just like today's heap. That doesn't mean it have to start in the EAL.
> Suggestion regarding naming:
> The prefix could be "rte_mem_" instead of "rte_fastmem_" - it is shorter, and most libraries are "fast".
> And then it could live in /lib/memory instead of /lib/fastmem.
>
mem is very generic, but point taken. Name should change, unclear to
what. Depends on the role it will serve.
^ permalink raw reply
* RE: [PATCH v4 16/20] net/txgbe: fix SFP module identification
From: Zaiyu Wang @ 2026-05-27 11:21 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517165225.6ba8b774@phoenix.local>
> ## Errors
>
> ### 1. Resource leak in `txgbe_identify_sfp_module()` - multiple error paths
> missing unlock
No code change
The review is incorrect — all error paths after acquiring the lock do properly release it. There is no lock leak.
> ### 2. `txgbe_write_i2c_byte()` removes STOP condition - probable hardware
No code change
This change was ported from our out‑of‑tree Linux kernel driver. It was found that the previous write_i2c interface had problems, which caused ethtool -m to read some 40G optical module information incorrectly. Removing the intermediate STOP is intentional and fixes that issue.
> ### 3. Removed validation logic without replacement - SFP vendor checking
No code change
The old code was inherited from the Intel driver. The restriction has been removed intentionally: the driver now supports all optical modules that comply with the relevant standards, without limiting to a specific vendor.
> ## Warnings
>
> ### 1. `txgbe_identify_qsfp_module()` GPIO config runs unconditionally
No code change.
The function txgbe_identify_qsfp_module is only called for the Amber‑Lite 40G NIC. Among the three NIC types supported by txgbe (Sapphire 10G, Amber‑Lite 25G, Amber‑Lite 40G), only the 40G variant uses QSFP; the other two use SFP. Based on the aml40 hardware design, we need to configure the GPIO before using I2C.
> ### 2. Missing error propagation - `txgbe_read_i2c_sff8636()` page select failure
> ignored
Fixed.
> ### 3. Implicit comparison on pointer
Fixed.
> ### 5. Function pointer assignments removed but functions still defined
Already verified.
^ permalink raw reply
* RE: [PATCH v4 17/20] net/txgbe: fix get module info operation
From: Zaiyu Wang @ 2026-05-27 11:18 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517165307.6249117a@phoenix.local>
> ## ERRORS
>
> ### 1. Resource leak on error path after lock acquisition
The AI review suggested moving the module presence checks inside the lock, which I believe
is not necessary. The presence checks occur before acquiring the semaphore, so returning
early cannot leak the lock. All error paths after lock acquisition do properly go through
the ERROR_IO label and release the semaphore.
I've changed the pre-lock error from -EIO to - EINVAL to better distinguish "module not
present" from actual I/O failures.
> ---
>
> ## WARNINGS
>
> ### 1. Implicit comparison with zero
Fixed.
> ### 2. Mixed type usage: `u8` vs `uint8_t`
Fixed.
> ### 3. Unnecessary variable initialization
Fixed.
> ## INFO
>
> ### 2. Error message clarity
> The error messages "I2C IO ERROR" and "SFF Module Type not recognized" are
> informative. Consider whether these should include additional context (e.g., the
> identifier value for unrecognized types) to aid debugging.
Added identifier value in error message.
^ permalink raw reply
* RE: [RFC v2 2/3] lib: add fastmem library
From: Morten Brørup @ 2026-05-27 11:17 UTC (permalink / raw)
To: Bruce Richardson, Mattias Rönnblom
Cc: Stephen Hemminger, dev, Konstantin Ananyev, Mattias Rönnblom,
Yogaraj Baskaravel
In-Reply-To: <ahbE6KvlgzG_XkSY@bricha3-mobl1.ger.corp.intel.com>
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 27 May 2026 12.18
>
> On Wed, May 27, 2026 at 12:12:19PM +0200, Mattias Rönnblom wrote:
> > On 5/26/26 15:23, Stephen Hemminger wrote:
> > > On Tue, 26 May 2026 10:57:42 +0200
> > > Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> > >
> > > > +__rte_experimental
> > > > +void *
> > > > +rte_fastmem_alloc(size_t size, size_t align, unsigned int flags)
> > > > + __rte_alloc_size(1) __rte_alloc_align(2);
> > >
> > > Should also add attribute __rte_malloc which tells compiler
> > > that pointer returned cannot alias other memory
> > >
> > > And add __rte_dealloc(rte_fastmem_free, 1)
> > > which tells compiler that the returned pointer should only go
> > > back to fastmem (not free, rte_free, etc).
> >
> > Done. Only works for the single-object ops (not bulk) though.
> >
> > I've had a look at how to extend fastmem to support larger
> allocations
> > (without suggesting this is the way to go).
> >
> > Seems to me that implementation should be something like
> > a) slab allocator for small objects.
> > b) a cache-less per-socket page run allocator for mid-sized objects.
> > c) per-object memzones for large objects.
All allocations through the library should somehow build on memzones.
If an application wants "normal" memory, it can use libc free()/malloc().
> >
> > If one would implement that, you would essentially have a plug-in
> > replacement for rte_malloc.h (maybe minus some debug and some more
> esoteric
> > DPDK heap features).
> >
> > Should fastmem be an outright replacement, or something that at least
> > initially lives alongside the regular heap, maybe with a run- or
> > compile-time option to make rte_malloc.h functions delegate to
> fastmem? This
> > is unclear to me at this point. I fear the more ambitious, cleaner
> and more
> > risky DPDK heap replacement path will go they way my attempts to
> replace
> > rte_memcpy or rte_timer went.
> >
> > I would agree with anyone saying that we should have only one heap-
> like API
> > for memory allocations. rte_malloc.h obviously needs to stay, for
> backward
> > compatibility reason, if nothing else. I would like to add bulk
> alloc/free,
> > and allow for smaller alignments than 64, since slabs can do that
> > efficiently (DPDK heap per-object header is 128 bytes!). One could
> either go
> > about that by extending rte_malloc.h or deprecating that API and
> starting
> > anew. In the latter case, one could do many more minor tweaks, like
> removing
> > the type pointers (only a nuance), remove the validate function,
> change and
> > extend the stats interface, etc.
> >
> +1 for replacing rte_malloc. For a replacement, I'd tend towards aiming
> for
> compatibilty over trying to fix too many little things at once. While
> changing a couple of things is ok, I'd rather not force applications to
> make too many updates to their code when moving from one DPDK version
> to
> another.
>
> /Bruce
+1 for taking the path that begins with starting anew.
The new heap library can be designed specifically for the fast path, based on years of DPDK experience with what the fast path really needs.
It will allow much broader experimentation along the way.
We can easily get rid of all slow path legacy stuff in rte_malloc, like the "type" parameter.
I think we all agree that replacing rte_malloc should be the end game.
But IMO, it very important that the properties of rte_malloc do not impose any limits on the new heap library.
Someday, when the new library has sufficiently matured, we can discuss how it can replace the rte_malloc library.
Maybe some parts of rte_malloc are not replaceable, and need to be deprecated and removed.
Maybe we all have switched to using the new heap, and nobody is using rte_malloc anymore, so it can simply be removed. ;-)
Suggestion regarding naming:
The prefix could be "rte_mem_" instead of "rte_fastmem_" - it is shorter, and most libraries are "fast".
And then it could live in /lib/memory instead of /lib/fastmem.
^ permalink raw reply
* Re: [RFC v2 2/3] lib: add fastmem library
From: Mattias Rönnblom @ 2026-05-27 11:17 UTC (permalink / raw)
To: Bruce Richardson
Cc: Stephen Hemminger, dev, Morten Brørup, Konstantin Ananyev,
Mattias Rönnblom, Yogaraj Baskaravel
In-Reply-To: <ahbE6KvlgzG_XkSY@bricha3-mobl1.ger.corp.intel.com>
On 5/27/26 12:18, Bruce Richardson wrote:
> On Wed, May 27, 2026 at 12:12:19PM +0200, Mattias Rönnblom wrote:
>> On 5/26/26 15:23, Stephen Hemminger wrote:
>>> On Tue, 26 May 2026 10:57:42 +0200
>>> Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>>
>>>> +__rte_experimental
>>>> +void *
>>>> +rte_fastmem_alloc(size_t size, size_t align, unsigned int flags)
>>>> + __rte_alloc_size(1) __rte_alloc_align(2);
>>>
>>> Should also add attribute __rte_malloc which tells compiler
>>> that pointer returned cannot alias other memory
>>>
>>> And add __rte_dealloc(rte_fastmem_free, 1)
>>> which tells compiler that the returned pointer should only go
>>> back to fastmem (not free, rte_free, etc).
>>
>> Done. Only works for the single-object ops (not bulk) though.
>>
>> I've had a look at how to extend fastmem to support larger allocations
>> (without suggesting this is the way to go).
>>
>> Seems to me that implementation should be something like
>> a) slab allocator for small objects.
>> b) a cache-less per-socket page run allocator for mid-sized objects.
>> c) per-object memzones for large objects.
>>
>> If one would implement that, you would essentially have a plug-in
>> replacement for rte_malloc.h (maybe minus some debug and some more esoteric
>> DPDK heap features).
>>
>> Should fastmem be an outright replacement, or something that at least
>> initially lives alongside the regular heap, maybe with a run- or
>> compile-time option to make rte_malloc.h functions delegate to fastmem? This
>> is unclear to me at this point. I fear the more ambitious, cleaner and more
>> risky DPDK heap replacement path will go they way my attempts to replace
>> rte_memcpy or rte_timer went.
>>
>> I would agree with anyone saying that we should have only one heap-like API
>> for memory allocations. rte_malloc.h obviously needs to stay, for backward
>> compatibility reason, if nothing else. I would like to add bulk alloc/free,
>> and allow for smaller alignments than 64, since slabs can do that
>> efficiently (DPDK heap per-object header is 128 bytes!). One could either go
>> about that by extending rte_malloc.h or deprecating that API and starting
>> anew. In the latter case, one could do many more minor tweaks, like removing
>> the type pointers (only a nuance), remove the validate function, change and
>> extend the stats interface, etc.
>>
> +1 for replacing rte_malloc. For a replacement, I'd tend towards aiming for
> compatibilty over trying to fix too many little things at once. While
> changing a couple of things is ok, I'd rather not force applications to
> make too many updates to their code when moving from one DPDK version to
> another.
>
What one could attempt to do is to be fully backward compatible with
rte_malloc.h (maybe minus some debug features that require per-object
headers?) and then expose a new API which is functionally a superset of
rte_malloc.h.
The new API would be something like a hybrid of rte_malloc.h, the
mempool APIs, and the kind of API you find on an in-kernel memory
manager (e.g., Solaris'), tuned for DPDK lcore use.
> /Bruce
^ permalink raw reply
* RE: [PATCH v4 20/20] net/txgbe: fix to enable Tx desc check
From: Zaiyu Wang @ 2026-05-27 11:14 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517165533.20cdb300@phoenix.local>
> Minor AI stuff here:
Thank you for the review. Based on the feedback, I have made the following adjustments:
> ## WARNINGS
>
> ### 1. Bit manipulation should use RTE_BIT32() macro
Fixed.
> ### 2. Conditional compilation without corresponding documentation update
No code change
This patch does not change the intended behavior of Tx desc check. Due to hardware
limitation, IPsec and desc check cannot be enabled at the same time; the original design
was to enable desc check by default when IPsec is not in use. The previous code had a bug
that prevented this from working correctly, and this patch simply fixes that. Therefore a
separate document update is not needed.
>
> ## INFO
>
> ### 1. Potential logic optimization
No code change
I chose not to extract the bitmask into a separate variable to keep the change minimal and
the code concise.
>
> ### 2. Parentheses around `using_ipsec` unnecessary
Fixed.
^ permalink raw reply
* RE: [PATCH v4 18/20] net/txgbe: fix get EEPROM operation
From: Zaiyu Wang @ 2026-05-27 11:11 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517165434.5a4e3a72@phoenix.local>
> I agree with AI that u8, u16 should not be used except in base/ code.
>
Fixed.
^ permalink raw reply
* RE: [PATCH v4 09/20] net/txgbe: fix link flow control config for Sapphire
From: Zaiyu Wang @ 2026-05-27 11:11 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517164612.4d1c622b@phoenix.local>
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, May 18, 2026 7:46 AM
> To: Zaiyu Wang <zaiyuwang@trustnetic.com>; Zaiyu Wang
> <zaiyuwang@trustnetic.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Jiawen Wu <jiawenwu@trustnetic.com>;
> Ferruh Yigit <ferruh.yigit@amd.com>; dev@dpdk.org; stable@dpdk.org; Jiawen
> Wu <jiawenwu@trustnetic.com>; Ferruh Yigit <ferruh.yigit@amd.com>
> Subject: Re: [PATCH v4 09/20] net/txgbe: fix link flow control config for Sapphire
>
> On Mon, 11 May 2026 18:35:51 +0800
> Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
>
> > SP chips have a hardware bug preventing XON flow control support, so
> > the driver disables it.
> >
> > Fixes: 69ce8c8a4ce3 ("net/txgbe: support flow control")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> > ---
>
> Comment vs code mismatch
>
>
> # Review of Patch 9/20: net/txgbe: fix link flow control config for Sapphire
>
> ## Errors
>
> None.
>
> ## Warnings
>
> ### W1: Conditional logic inverted (line 1042-1043)
Fixed.
> ### W2: Inconsistent capitalization in comment (line 1042)
Fixed.
^ permalink raw reply
* RE: [PATCH v4 04/20] net/ngbe: fix VF promiscuous and allmulticast
From: Zaiyu Wang @ 2026-05-27 11:09 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517163901.744df0fc@phoenix.local>
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, May 18, 2026 7:39 AM
> To: Zaiyu Wang <zaiyuwang@trustnetic.com>; Zaiyu Wang
> <zaiyuwang@trustnetic.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Jiawen Wu <jiawenwu@trustnetic.com>;
> dev@dpdk.org; stable@dpdk.org; Jiawen Wu <jiawenwu@trustnetic.com>
> Subject: Re: [PATCH v4 04/20] net/ngbe: fix VF promiscuous and allmulticast
>
> On Mon, 11 May 2026 18:35:46 +0800
> Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
>
> > The configuration of allmulti and promiscuous modes conflicts
> > together. For instance, if we enable promiscuous mode, then enable and
> > disable allmulti, then the promiscuous mode is wrongly disabled.
> >
> > To resolve this, the following changes are made:
> > - do nothing when we set/unset allmulti if promiscuous mode is on
> > - restore the proper mode (none or allmulti) when we disable
> > promiscuous mode
> >
> > Fixes: 7744e90805b5 ("net/ngbe: add promiscuous and allmulticast ops
> > for VF device")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> > ---
>
> Worth mentioning, but IMHO this is case of AI being overly pedantic.
> Patch is OK as is.
Thanks.
^ permalink raw reply
* RE: [PATCH v4 05/20] net/txgbe: fix inaccuracy in Tx rate limiting
From: Zaiyu Wang @ 2026-05-27 11:07 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517164018.7a58c68c@phoenix.local>
> AI feedback on this one. Looks ok as is but worth looking at.
Thank you for the review. Based on the feedback, I have made the following adjustments:
> # DPDK Patch Review: net/txgbe: fix inaccuracy in Tx rate limiting
> ### **Errors**
>
> None found.
>
> ### **Warnings**
>
> **1. TDM_RL_ADJ write placement causes per-queue reconfiguration**
No code change
`txgbe_set_queue_rate_limit()` is directly exposed as the
`eth_dev_ops.set_queue_rate_limit` to upper-layer applications. We cannot expect
applications to perform any additional setup before calling this interface, so the driver
must reconfig.
Writing the same value to this register multiple times is harmless. Therefore I prefer to
keep the current approach.
> **2. Magic number 21 lacks explanation**
>
> The value 21 is documented in the commit message but not in the code. Future
> maintainers will not see the breakdown (7B preamble + 1B SFD + 1B EFD + 12B
> IPG).
>
> **Suggested fix:** Add a comment or define:
>
> ```c
> /* Ethernet framing overhead: 7B preamble + 1B SFD + 1B EFD + 12B IPG */
> #define TXGBE_TDM_RL_ADJ_OVERHEAD 21 wr32(hw, TXGBE_TDM_RL_ADJ,
> TXGBE_TDM_RL_ADJ_OVERHEAD); ```
>
I have added a macro `TXGBE_FRAME_OVERHEAD`.
> **3. Missing release notes update**
>
> This is a fix that changes hardware behavior and improves accuracy. Users may
> observe different TX rates after upgrading. The change should be documented in
> release notes to inform users of the accuracy improvement.
>
> **Suggested fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or
> current release):
>
> ```rst
> * **net/txgbe: Improved TX rate limiting accuracy for Amber-lite NIC.**
>
> Fixed TX rate limiting accuracy for 64-byte packets from ~20% deviation
> to within ~5% by correcting hardware framing overhead configuration.
> ```
>
> ### **Info**
>
> **1. Consider validating tx_rate is non-zero**
>
> The code computes `factor_int = link_speed / tx_rate`. If `tx_rate` is somehow zero
> (due to caller error or invalid input), this would cause division by zero. The existing
> code may have validation elsewhere, but defensive programming would check
> this.
>
> ```c
> if (tx_rate == 0) {
> PMD_DRV_LOG(ERR, "TX rate cannot be zero");
> return -EINVAL;
> }
> ```
No code change
In this function, `tx_rate == 0` is a valid input meaning "remove rate limiting and
restore unlimited mode". No additional check is needed.>
> **2. Variable scope: factor_fra could be declared at initialization**
>
> The variable `factor_fra` is declared but not initialized, then assigned in both
> branches of the conditional. C99 style would allow:
>
> ```c
> u32 factor_fra = (frac * 16384) / 10000; if (factor_fra >= 8192)
> factor_fra = 0;
> ```
>
> This is stylistic only; the existing code is correct.
>
No code change
Just leave it as is.
^ permalink raw reply
* RE: [PATCH v4 07/20] net/txgbe: fix Tx desc free logic
From: Zaiyu Wang @ 2026-05-27 11:06 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517164444.7c0cb0e0@phoenix.local>
> Lots of AI feedback on this one.
>
>
> # Review of DPDK Patch: net/txgbe: fix Tx desc free logic
>
> ## Errors
>
> ### 1. Shared variable access without atomics (3 locations)
>
> **Lines affected:**
> - `drivers/net/txgbe/txgbe_rxtx.c:101` (txgbe_tx_free_bufs)
> - `drivers/net/txgbe/txgbe_rxtx.c:655` (txgbe_xmit_cleanup)
> - `drivers/net/txgbe/txgbe_rxtx_vec_common.h:98` (txgbe_tx_free_bufs)
>
> ```c
> volatile uint16_t head = (uint16_t)*txq->headwb_mem; ```
>
> **Problem:** The code reads a hardware-updated value (`*txq->headwb_mem`)
> using a `volatile` qualifier. The `volatile` keyword does NOT provide atomicity or
> memory ordering guarantees between the hardware and CPU. On some
> architectures, reading a `uint16_t` from DMA-accessible memory without proper
> barriers or atomic operations can result in torn reads, stale cached values, or
> reordering issues.
>
> **Fix:** Use atomic load with appropriate memory ordering:
>
> ```c
> uint16_t head = rte_atomic_load_explicit(
> (uint16_t *)txq->headwb_mem,
> rte_memory_order_acquire);
> ```
>
> The `acquire` ordering ensures that all descriptor writes by the hardware are visible
> to the CPU after reading the head pointer. This is critical for correctness in
> hardware write-back scenarios.
>
> Note: The `headwb_mem` field type may need adjustment to be an `rte_atomic`
> type or cast appropriately.
>
Fixed. All three reads from headwb_mem now use rte_atomic_load_explicit
with rte_memory_order_acquire, and the loaded value is declared const.
> ---
>
> ### 2. Whitespace/indentation error in vec_common.h
>
> **Line:** `drivers/net/txgbe/txgbe_rxtx_vec_common.h:97-98`
>
> ```c
> tx_last_dd -= txq->nb_tx_desc;
>
> volatile uint16_t head = (uint16_t)*txq->headwb_mem; ```
>
> **Problem:** Inconsistent indentation. The `volatile uint16_t head` declaration is
> indented with tabs-only (correct), but it visually appears to be part of a removed
> block based on the diff context showing the previous line had different
> indentation.
>
> **Fix:** Verify the indentation matches the surrounding code block. Based on the
> diff, this appears correct after the change, but confirm the tabs align with the
> function's indentation level.
Verified, the indentation is correct and matches the surrounding code.
>
> ---
>
> ## Warnings
>
> ### 1. Logic change correctness verification needed
>
> The patch adds three new early-return conditions checking if `head == tx_next_dd`.
> While this appears correct (hardware hasn't advanced past the point we want to
> free to), the logic for wrap-around cases should be verified:
>
> ```c
> if (txq->tx_next_dd == head)
> return 0;
> else if (txq->tx_next_dd > head && head > tx_last_dd)
> return 0;
> else if (tx_last_dd > txq->tx_next_dd &&
> (head > tx_last_dd || head < txq->tx_next_dd))
> return 0;
> ```
>
> **Concern:** The wrap-around logic assumes the ring wraps at `nb_tx_desc`. The
> condition `tx_last_dd > txq->tx_next_dd` handles the wrap case, but verify that:
> - `tx_last_dd` calculation correctly wraps (it does: `tx_last_dd -= txq->nb_tx_desc`)
> - The inequality checks are strict `>` not `>=` consistently
>
> The existing code appears correct, but this is complex boundary logic that warrants
> testing, especially the second `else if` condition in wrap-around scenarios.
The logic has been carefully reviewed and tested. The extracted helper
function preserves the exact same conditions as the original code, so
no behavioural change is introduced.
>
> ---
>
> ### 2. Missing const qualifier on read-only atomic load result
>
> **All three locations:** The `head` variable is read from hardware but never
> modified:
>
> ```c
> volatile uint16_t head = (uint16_t)*txq->headwb_mem; ```
>
> **Suggestion:** Once converted to atomic operations, declare as `const`:
>
> ```c
> const uint16_t head = rte_atomic_load_explicit(...); ```
>
> This documents that `head` is never modified after the initial read and prevents
> accidental writes.
Done, the loaded head value is now const.
>
> ---
>
> ## Info
>
> ### 1. Consider helper function for repeated logic
>
> The same boundary-check logic appears in three separate functions with only
> minor variations. Consider extracting it to a static inline helper:
>
> ```c
> static inline bool
> txgbe_hw_head_in_range(uint16_t hw_head, uint16_t start,
> uint16_t end, uint16_t ring_size) {
> if (start == hw_head)
> return false;
>
> if (start > hw_head)
> return hw_head <= end;
> else /* wrap case */
> return hw_head > end || hw_head < start; } ```
>
> This would reduce code duplication and make the logic easier to test and maintain.
I have extracted the common boundary-check logic into a new static
inline helper, txgbe_tx_headwb_desc_done(), placed in txgbe_rxtx.h.
All three call sites now use this helper.
>
> ---
>
> ### 2. Comment explaining write-back head semantics
>
> The commit message mentions "Tx head write-back mode" but the code lacks
> comments explaining the hardware behavior. Consider adding:
>
> ```c
> /*
> * In Amber-Lite NIC Tx head write-back mode, hardware periodically
> * updates *headwb_mem with the index of the next descriptor to process.
> * Descriptors before this head pointer are safe to free.
> */
> const uint16_t head = rte_atomic_load_explicit(...); ```
Added a detailed comment for the helper function explaining the
hardware behavior and the (last, next] interval semantics.
>
> ---
>
> ## Summary
>
> **Must fix (Errors):**
> 1. Replace `volatile` read with `rte_atomic_load_explicit()` with
> `rte_memory_order_acquire` ordering in all three locations
>
> **Should fix (Warnings):**
> 1. Add `const` qualifier to the loaded `head` value after converting to atomics 2.
> Verify wrap-around boundary logic with testing (appears correct but is complex)
>
> **Consider (Info):**
> 1. Extract repeated boundary-check logic to a helper function 2. Add comments
> documenting hardware write-back semantics
>
> The core fix (adding the `head == tx_next_dd` check) appears correct and
> addresses the described bug. The primary issue is the use of `volatile` instead of
> proper atomic operations for accessing hardware-updated shared memory.
>
>
^ permalink raw reply
* RE: [PATCH v4 03/20] net/ngbe: add missing CDR config for YT PHY
From: Zaiyu Wang @ 2026-05-27 11:06 UTC (permalink / raw)
To: 'Stephen Hemminger'; +Cc: dev
In-Reply-To: <20260517163740.52739478@phoenix.local>
> Semi-automated AI review feedback.
Thank you for the review. Based on the feedback, I have made the following adjustments:
>
> # DPDK Patch Review - net/ngbe: add missing CDR config for YT PHY
> ## WARNINGS
>
> ### 1. Missing error checks on PHY register writes
No code change.
In the long-term operation of the Wangxun Emerald Gigabit Ethernet controllers supported
by the ngbe driver, PHY register read/write failures are virtually never observed.
Consequently, ngbe_write_phy_reg_sds_ext_yt() effectively always returns 0.
We currently have no plan to add return value checks for PHY register access routines, and
we believe the existing approach is sufficiently robust in practice.
> ---
>
> ## INFO
>
> ### 1. Magic numbers in PHY register writes
No code change.
These values were provided by the PHY vendor (Motorcomm) and the bit definitions are not
documented in publicly available register description.
Replacing them with macros such as YT_CONFIG_REG_XXX would merely introduce meaningless
names without improving understandability. Therefore we prefer to keep the raw vendor
values as they are.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox