[PWCI] "net/mlx5: remove representor matching devarg" #252
Conversation
As announced in 25.07 release deprecation notice [1], this patch removes repr_matching_en device argument from mlx5 driver. Applications which disabled this option were able to receive traffic from any physical port/VF/SF on any representor. Specifically, in most cases, this was used to process all traffic on representor which is a transfer proxy port. Similar behavior in mlx5 PMD can be achieved without the use of additional device arguments, by using RTE_FLOW_ACTION_TYPE_RSS flow action in transfer flow rules. [1] https://doc.dpdk.org/guides-25.07/rel_notes/deprecation.html Signed-off-by: Dariusz Sosnowski <dsosnowski@nvidia.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis patch removes the deprecated "repr_matching" configuration and all associated code paths from the MLX5 PMD. It simplifies various flow-setup conditional checks by eliminating representor-matching guards and cleans up default Tx metadata-copy routines and documentation accordingly. Sequence diagram for simplified flow setup without repr_matchingsequenceDiagram
participant "mlx5_dev_spawn()"
participant "mlx5_shared_dev_ctx_args_config()"
participant "mlx5_flow_isolate()"
"mlx5_dev_spawn()"->>"mlx5_shared_dev_ctx_args_config()": configure device
"mlx5_shared_dev_ctx_args_config()"-->>"mlx5_dev_spawn()": returns config
"mlx5_dev_spawn()"->>"mlx5_flow_isolate()": (no longer checks repr_matching)
"mlx5_flow_isolate()"-->>"mlx5_dev_spawn()": isolated mode set if requested
Sequence diagram for flow setup with dv_esw_en (repr_matching removed)sequenceDiagram
participant "mlx5_traffic_enable_hws()"
participant "mlx5_flow_hw_tx_repr_matching_flow()"
"mlx5_traffic_enable_hws()"->>"mlx5_flow_hw_tx_repr_matching_flow()": create tx repr matching flow (no repr_matching check)
"mlx5_flow_hw_tx_repr_matching_flow()"-->>"mlx5_traffic_enable_hws()": returns status
Class diagram for removal of repr_matching from mlx5_sh_config and related structuresclassDiagram
class mlx5_sh_config {
uint32_t lro_allowed
uint32_t fdb_def_rule
uint32_t txq_mem_algn
cnt_svc cnt_svc_member
}
class cnt_svc {
uint16_t service_core
uint32_t cycle_time
}
class mlx5_flow_hw_ctrl_fdb {
rte_flow_pattern_template *port_items_tmpl
rte_flow_actions_template *jump_one_actions_tmpl
rte_flow_template_table *hw_esw_zero_tbl
rte_flow_pattern_template *lacp_rx_items_tmpl
rte_flow_actions_template *lacp_rx_actions_tmpl
rte_flow_template_table *hw_lacp_rx_tbl
}
mlx5_sh_config <|-- mlx5_flow_hw_ctrl_fdb
mlx5_sh_config o-- cnt_svc
Class diagram for removed function and template creation routinesclassDiagram
class mlx5_flow_hw_create_tx_default_mreg_copy_flow {
// REMOVED
}
class flow_hw_create_tx_default_mreg_copy_pattern_template {
// REMOVED
}
class flow_hw_create_tx_default_mreg_copy_actions_template {
// REMOVED
}
class flow_hw_create_tx_default_mreg_copy_table {
// REMOVED
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis PR removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `doc/guides/rel_notes/release_25_11.rst:203-204` </location>
<code_context>
+* net/mlx5: ``repr_matching_en`` device argument has been removed.
+ Applications which disabled this option were able to receive traffic
+ from any physical port/VF/SF on any representor.
+ Specifically, in most cases, this was used to process all traffic on representor
+ which is a transfer proxy port.
+ Similar behavior in mlx5 PMD can be achieved without the use of additional device arguments,
+ by using ``RTE_FLOW_ACTION_TYPE_RSS`` flow action in transfer flow rules.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding 'a' before 'representor' for grammatical correctness.
Change the sentence to: 'Specifically, in most cases, this was used to process all traffic on a representor, which is a transfer proxy port.'
```suggestion
Specifically, in most cases, this was used to process all traffic on a representor,
which is a transfer proxy port.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| Specifically, in most cases, this was used to process all traffic on representor | ||
| which is a transfer proxy port. |
There was a problem hiding this comment.
suggestion (typo): Consider adding 'a' before 'representor' for grammatical correctness.
Change the sentence to: 'Specifically, in most cases, this was used to process all traffic on a representor, which is a transfer proxy port.'
| Specifically, in most cases, this was used to process all traffic on representor | |
| which is a transfer proxy port. | |
| Specifically, in most cases, this was used to process all traffic on a representor, | |
| which is a transfer proxy port. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
drivers/net/mlx5/mlx5_txq.c (1)
1456-1461: Preserve original rte_errno when cleaning up on failure.Destroying the SQ miss flow may overwrite rte_errno, masking the original failure of tx repr-matching flow creation. Save and restore it before returning.
- if (mlx5_flow_hw_tx_repr_matching_flow(dev, sq_num, true)) { - if (sq_miss_created) - mlx5_flow_hw_esw_destroy_sq_miss_flow(dev, sq_num); - return -rte_errno; - } + if (mlx5_flow_hw_tx_repr_matching_flow(dev, sq_num, true)) { + int saved_errno = rte_errno; + if (sq_miss_created) + mlx5_flow_hw_esw_destroy_sq_miss_flow(dev, sq_num); + rte_errno = saved_errno; + return -rte_errno; + }drivers/net/mlx5/mlx5_flow_hw.c (1)
5308-5329: Egress group translation is out of sync with default TX repr-tagging; can leak user flows into reserved group 0You always create default egress control flows when dv_esw_en is set (see flow_hw_setup_tx_repr_tagging), but here you only bump user-specified egress groups when dv_xmeta_en == MLX5_XMETA_MODE_META32_HWS. If dv_esw_en is true and dv_xmeta_en is not META32_HWS, external flows can still target group 0 and collide with PMD’s reserved egress rules.
Align the conditions by bumping egress groups for any dv_esw_en, not just META32_HWS. Also update the comment accordingly.
Proposed fix:
- } else if (config->dv_esw_en && - config->dv_xmeta_en == MLX5_XMETA_MODE_META32_HWS && - external && - flow_attr->egress) { + } else if (config->dv_esw_en && + external && + flow_attr->egress) { /* - * On E-Switch setups, default egress flow rules are inserted to allow - * representor matching and/or preserving metadata across steering domains. + * On E-Switch setups, default egress control flows are inserted (group 0) + * to support representor TX tagging/preservation. Group 0 is reserved. * These flow rules are inserted in group 0 and this group is reserved by PMD - * for these purposes. + * for these purposes. * - * As a result, if representor matching or extended metadata mode is enabled, - * group provided by the user must be incremented to avoid inserting flow rules - * in group 0. + * As a result, the user-provided egress group must be incremented to avoid + * inserting flow rules in group 0. */
🧹 Nitpick comments (5)
doc/guides/nics/mlx5.rst (1)
3164-3166: Wording consistency: prefer “E‑Switch (transfer) domain”.To align with the rest of the guide, consider rephrasing to explicitly say “In the E‑Switch (transfer) domain, compare is not supported for ingress rules,” preserving the direction detail.
doc/guides/rel_notes/release_25_11.rst (1)
200-207: Add a short migration pointer.Consider appending one sentence to guide users: “To replicate previous behavior, use RTE_FLOW_ACTION_TYPE_RSS in the E‑Switch (transfer) domain; see Port/Represented Port flow sections in mlx5.rst for examples.”
drivers/net/mlx5/mlx5_trigger.c (1)
1135-1140: dr_ctx check is fine but redundant.dev_start initializes HWS (dr_ctx) earlier in the same path. Keeping this check is harmless; alternatively drop it to avoid duplicate validation.
drivers/net/mlx5/mlx5_flow_hw.c (2)
8552-8562: Represented-port item domain check OK; consider clearer error textBlocking represented_port in ingress/egress domains is correct (transfer-only). Consider consolidating the two errors into a single message stating “represented_port item requires transfer domain (no ingress/egress)”, to match action-level validation and reduce confusion.
11907-11912: Unconditional TX repr-tagging setup matches intent; ensure translate_group bump matchesCreating default TX repr-tagging tables whenever dv_esw_en is enabled looks good. This must be paired with egress group translation that always bumps user groups under dv_esw_en (see __translate_group). Otherwise users may target reserved group 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
doc/guides/nics/mlx5.rst(1 hunks)doc/guides/rel_notes/deprecation.rst(0 hunks)doc/guides/rel_notes/release_25_11.rst(1 hunks)drivers/net/mlx5/linux/mlx5_os.c(0 hunks)drivers/net/mlx5/mlx5.c(0 hunks)drivers/net/mlx5/mlx5.h(0 hunks)drivers/net/mlx5/mlx5_flow.c(1 hunks)drivers/net/mlx5/mlx5_flow.h(0 hunks)drivers/net/mlx5/mlx5_flow_hw.c(3 hunks)drivers/net/mlx5/mlx5_trigger.c(2 hunks)drivers/net/mlx5/mlx5_txq.c(1 hunks)
💤 Files with no reviewable changes (5)
- drivers/net/mlx5/mlx5.h
- drivers/net/mlx5/linux/mlx5_os.c
- drivers/net/mlx5/mlx5.c
- drivers/net/mlx5/mlx5_flow.h
- doc/guides/rel_notes/deprecation.rst
🧰 Additional context used
🧬 Code graph analysis (1)
drivers/net/mlx5/mlx5_txq.c (1)
drivers/net/mlx5/mlx5_flow_hw.c (1)
mlx5_flow_hw_tx_repr_matching_flow(15805-15859)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
drivers/net/mlx5/mlx5_flow.c (1)
8783-8807: Isolate-mode gating simplified — verified cleanThe devarg
repr_matching_enremoval is documented in release notes, and the verification confirms no stale references exist. The internal functionmlx5_flow_hw_tx_repr_matching_flow()remains active in mlx5_txq.c and mlx5_trigger.c—this is expected and correct. The change tomlx5_flow_isolate()removing the repr_matching-based restriction is consistent with the user-facing devarg removal while allowing internal functionality to continue operating.drivers/net/mlx5/mlx5_trigger.c (1)
1624-1630: Request verification: TX repr matching gating strategy and SQ miss flow alignment.The code at lines 1624-1630 calls
mlx5_flow_hw_tx_repr_matching_flow()without gating on(priv->representor || priv->master), while the preceding SQ miss flow block (lines 1616-1622) does gate on these conditions. However, the same ungated pattern appears inmlx5_txq.c:1456, suggesting this may be intentional rather than an oversight.To confirm whether gating is needed:
- Verify whether
hw_tx_repr_tagging_tblis always successfully created for non-representor ports whendv_esw_en=1, or whether table creation fails for certain port types.- If the table creation can fail for non-representor ports, both
mlx5_trigger.c:1625andmlx5_txq.c:1456would need the same fix.- If the table is always created for all
dv_esw_enports, the asymmetry with SQ miss flow may be acceptable (SQ miss has additionalfdb_def_rulecondition).The error logged at
mlx5_flow_hw.c:15842suggests the table may not always exist, supporting the review's concern—but the code pattern is consistent across multiple call sites, which requires verification to distinguish between a genuine bug and defensive design.
NOTE: This is an auto submission for "net/mlx5: remove representor matching devarg".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36478" for details.
Summary by Sourcery
Remove the deprecated 'representor matching' device argument and all associated code paths, templates and checks from the mlx5 PMD and documentation.
Enhancements:
Documentation:
Summary by CodeRabbit
Release Notes
Removed Features
repr_matching_endevice argument from mlx5 driver. Use RTE_FLOW_ACTION_TYPE_RSS in transfer flow rules to achieve equivalent traffic distribution behavior.Documentation