[PWCI] "net/mlx5: add RSS TIR registration API"#605
Conversation
mlx5 PMD is a bifurcated driver, which uses rdma-core APIs to communicate with HW instead of UIO or VFIO directly. It is possible for external libraries built on rdma-core to steer traffic to DPDK-managed Rx queues using mlx5dv flow API. This requires access to TIR object handles. TIR (Transport Interface Receive) is a hardware object that defines how incoming packets are distributed across Rx queues. It encodes the complete RSS configuration including the hash function type, hash key, set of destination queues, and which packet fields (IP addresses, ports, etc.) participate in the hash calculation. When a flow rule uses a TIR as its destination, the hardware applies this RSS configuration to matching packets. Add the following functions to mlx5 PMD private API: - rte_pmd_mlx5_rss_tir_register(): Create a TIR DevX object based on the provided RSS configuration. The returned TIR handle can be used as a destination action in mlx5dv flow steering APIs from rdma-core. - rte_pmd_mlx5_rss_tir_unregister(): Release the TIR object when no longer needed. Signed-off-by: Shani Peretz <shperetz@nvidia.com> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideAdds a new experimental mlx5 PMD API to register and unregister RSS TIR DevX objects, wiring it into the hardware steering RSS implementation and exposing the necessary types and exports for external consumers. Sequence diagram for RSS TIR registration and unregistrationsequenceDiagram
participant Ext as External_consumer
participant PMD as mlx5_pmd_api
participant ETH as rte_eth_dev
participant HRXQ as mlx5_hrxq
participant TIR as DevX_TIR_object
Ext->>PMD: rte_pmd_mlx5_rss_tir_register(port_id, rss, devx)
PMD->>PMD: rte_eth_dev_is_valid_port(port_id)
alt invalid_port
PMD-->>Ext: return -ENODEV
else valid_port
PMD->>ETH: get rte_eth_devices[port_id]
PMD->>PMD: mlx5_hws_active(ETH)
alt hws_not_active_or_invalid_rss
PMD-->>Ext: return -EINVAL
else ok
PMD->>PMD: build mlx5_flow_rss_desc from rss
PMD->>HRXQ: mlx5_hrxq_get(ETH, rss_desc)
alt allocation_failed
PMD-->>Ext: return error
else success
HRXQ->>TIR: provide tir->obj, tir->id
PMD->>PMD: devx->destroy_handle = HRXQ
PMD->>PMD: devx->obj = TIR.obj
PMD->>PMD: devx->id = TIR.id
PMD-->>Ext: return 0
end
end
end
Ext->>PMD: rte_pmd_mlx5_rss_tir_unregister(port_id, devx.destroy_handle)
PMD->>PMD: rte_eth_dev_is_valid_port(port_id)
alt invalid_port
PMD-->>Ext: return -ENODEV
else valid_port
PMD->>ETH: get rte_eth_devices[port_id]
PMD->>HRXQ: mlx5_hrxq_obj_release(ETH, destroy_handle)
PMD-->>Ext: return 0
end
Class diagram for new mlx5 RSS TIR registration APIclassDiagram
class rte_pmd_mlx5_rss_devx {
void* obj
void* destroy_handle
uint32_t id
}
class rte_flow_action_rss {
<<external>>
}
class mlx5_flow_rss_desc {
<<internal>>
uint32_t hws_flags
uint32_t queue_num
const uint16_t* const_q
uint8_t key[MLX5_RSS_HASH_KEY_LEN]
uint32_t key_len
uint64_t types
uint64_t hash_fields
uint8_t symmetric_hash_function
uint8_t tunnel
}
class mlx5_hrxq {
<<internal>>
mlx5_tir* tir
}
class mlx5_tir {
void* obj
uint32_t id
}
class rte_eth_dev {
<<external>>
}
class mlx5_pmd_api {
<<module>>
int rte_pmd_mlx5_rss_tir_register(uint16_t port_id, const rte_flow_action_rss* rss, rte_pmd_mlx5_rss_devx* devx)
int rte_pmd_mlx5_rss_tir_unregister(uint16_t port_id, void* handle)
}
mlx5_pmd_api --> rte_pmd_mlx5_rss_devx : fills
mlx5_pmd_api --> rte_flow_action_rss : consumes
mlx5_pmd_api --> mlx5_flow_rss_desc : builds
mlx5_pmd_api --> mlx5_hrxq : uses
mlx5_hrxq --> mlx5_tir : owns
mlx5_pmd_api --> rte_eth_dev : accesses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR introduces two new public API functions to the mlx5 driver for registering and unregistering RSS TIR (Traffic Indication Record) DevX objects. The implementation validates inputs, manages HRXQ/DevX resources, and enables external libraries to direct traffic to DPDK-managed receive queues. Changes
Sequence DiagramsequenceDiagram
participant Client as External Library
participant Driver as mlx5 Driver
participant HWS as HWS/HRXQ
rect rgb(200, 220, 255)
Note over Client,HWS: RSS TIR Register Flow
Client->>Driver: rte_pmd_mlx5_rss_tir_register(port, rss_config, devx)
Driver->>Driver: Validate port & RSS config
Driver->>Driver: Construct RSS description
Driver->>HWS: Allocate/retrieve HRXQ resource
HWS-->>Driver: Return DevX object
Driver->>Driver: Populate devx struct (obj, id, handle)
Driver-->>Client: Return 0 (success)
end
rect rgb(255, 220, 200)
Note over Client,HWS: RSS TIR Unregister Flow
Client->>Driver: rte_pmd_mlx5_rss_tir_unregister(port, handle)
Driver->>Driver: Validate port
Driver->>Driver: Cast handle to HRXQ object
Driver->>HWS: Release HRXQ resource
HWS-->>Driver: Acknowledgement
Driver-->>Client: Return 0 (success)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 - I've found 1 issue, and left some high level feedback:
- Consider adding basic parameter validation (e.g., NULL checks for
rssanddevxinrte_pmd_mlx5_rss_tir_register()and forhandleinrte_pmd_mlx5_rss_tir_unregister()) before dereferencing or casting to make the APIs more robust against misuse by external callers. - In
rte_pmd_mlx5_rss_tir_unregister(), you trust thathandleis a validstruct mlx5_hrxq *for the givenport_id; if feasible, add a sanity check (such as verifying the object belongs to the device or matches expected type) to avoid undefined behavior when external consumers pass an incorrect handle.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding basic parameter validation (e.g., NULL checks for `rss` and `devx` in `rte_pmd_mlx5_rss_tir_register()` and for `handle` in `rte_pmd_mlx5_rss_tir_unregister()`) before dereferencing or casting to make the APIs more robust against misuse by external callers.
- In `rte_pmd_mlx5_rss_tir_unregister()`, you trust that `handle` is a valid `struct mlx5_hrxq *` for the given `port_id`; if feasible, add a sanity check (such as verifying the object belongs to the device or matches expected type) to avoid undefined behavior when external consumers pass an incorrect handle.
## Individual Comments
### Comment 1
<location> `drivers/net/mlx5/mlx5_flow_hw.c:580` </location>
<code_context>
+RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mlx5_rss_tir_register, 26.03)
+int
+rte_pmd_mlx5_rss_tir_register(uint16_t port_id,
+ const struct rte_flow_action_rss *rss,
+ struct rte_pmd_mlx5_rss_devx *devx)
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider validating the `devx` output pointer before dereferencing it.
This function writes to `devx->destroy_handle`, `devx->obj`, and `devx->id` without first confirming `devx` is non-NULL, so a NULL argument would cause a crash. Please add a `if (devx == NULL)` guard (e.g., return `-EINVAL` and set `rte_errno`) to match typical DPDK argument validation and make the API safer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @drivers/net/mlx5/rte_pmd_mlx5.h:
- Around line 636-665: The implementation of rte_pmd_mlx5_rss_tir_register lacks
NULL checks and documentation clarity: add defensive parameter validation at the
start of rte_pmd_mlx5_rss_tir_register to return -EINVAL (or appropriate error)
if rss is NULL or devx is NULL before any dereference (e.g., before using
rss->queue_num or writing devx->destroy_handle/devx->obj/devx->id); treat
rss->key as optional (use default key when rss->key is NULL) and ensure any
early return leaves devx untouched on error; update the function comment to
state that rss and devx must be non-NULL, that rss->key is optional, summarize
thread-safety expectations, and describe TIR object lifecycle relative to port
stop/reconfigure/close.
🧹 Nitpick comments (3)
drivers/net/mlx5/mlx5_flow_hw.c (2)
578-640: RSS TIR register API is consistent with existing HRXQ/TIR handling, with a few minor robustness opportunitiesOverall,
rte_pmd_mlx5_rss_tir_register()looks correct and aligns with howflow_hw_tir_action_register()builds HRXQ/TIR objects:
- Port validation via
rte_eth_dev_is_valid_port(), queue array presence, and RSS key length checks are good and map to sensiblerte_errnovalues.- Using
mlx5_hws_active(dev)prevents use on non‑HWS ports, which is appropriate for DevX/HWS TIRs.- The RSS descriptor setup (default key when none is provided, default
RTE_ETH_RSS_IPwhenrss->types == 0, hash fields viaflow_hw_hashfields_set()plusflow_dv_action_rss_l34_hash_adjust(), and inner/tunnel bits forlevel > 1) mirrors existing code paths, which reduces risk.- Lifetime and sharing are reasonable:
mlx5_hrxq_get()will reuse or create an HRXQ, anddevx->destroy_handle = hrxq+mlx5_hrxq_obj_release()in unregister ensures reference‑counted cleanup as with internal flows.A couple of non‑blocking improvements you may want to consider:
Defensive NULL checks on inputs
Today the function assumes
rssanddevxare non‑NULL. A bad caller would hit a NULL dereference before you return an error. Adding early checks would make the API safer without changing normal behavior:if (!rss || !devx) { rte_errno = EINVAL; return -rte_errno; }Explicit comment on
hws_flagschoiceYou hardcode
.hws_flags = MLX5DR_ACTION_FLAG_ROOT_RX. Since this API only exposestir->obj/tir->idand nothrxq->action, it’s effectively using the existing HRXQ creation path just for the DevX TIR. A short comment explaining thatROOT_RXis intentionally used only to obtain a valid TIR, and that the returned handle is not the DR action, would help future maintainers avoid “fixing” this inadvertently.These are polish suggestions; the current implementation is functionally sound.
642-657: RSS TIR unregister API matches HRXQ lifetime model; consider propagating release errors
rte_pmd_mlx5_rss_tir_unregister()uses the same destruction path as internal flows:
- Validates
port_idand fetchesdev.- Casts
handleback tostruct mlx5_hrxq *and callsmlx5_hrxq_obj_release(dev, hrxq), which correctly reference‑counts and frees HRXQ/TIR objects (and is tolerant ofNULLhandles).Two small points you might refine:
Error propagation from
mlx5_hrxq_obj_release()The return value from
mlx5_hrxq_obj_release()is currently ignored. While other internal call sites also ignore it, this public‑facing API could reasonably surface a failure:int ret = mlx5_hrxq_obj_release(dev, hrxq); if (ret) { rte_errno = -ret; return ret; }Clarify handle contract in the header
Since
handleis an opaquevoid *that is actually astruct mlx5_hrxq *, it would be useful (in the header comment) to state explicitly that:
- The handle must come from
rte_pmd_mlx5_rss_tir_register().- It must not be used concurrently with unregister.
- It becomes invalid after a successful unregister.
Behavior‑wise, the current implementation is consistent with the HRXQ/TIR management used elsewhere in the driver.
drivers/net/mlx5/rte_pmd_mlx5.h (1)
667-680: Clarify documentation of idempotency and NULL handle behavior.The implementation safely handles NULL handles through
mlx5_hrxq_obj_release, which returns 0 on NULL input. However, the documentation should explicitly state:
- Idempotent behavior: Safe to call with NULL or already-released handles (returns 0)
- Invalid handle handling: The function assumes
handleis either NULL or a valid pointer fromrte_pmd_mlx5_rss_tir_devx(); no validation of corrupted pointers- Thread-safety: Clarify thread-safety guarantees for concurrent calls with the same handle or port
Note: The implementation does not currently return EINVAL; only ENODEV is returned for invalid port IDs.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
drivers/net/mlx5/mlx5_flow_hw.cdrivers/net/mlx5/rte_pmd_mlx5.h
🧰 Additional context used
🧬 Code graph analysis (2)
drivers/net/mlx5/mlx5_flow_hw.c (2)
drivers/net/mlx5/mlx5_flow_dv.c (1)
flow_dv_action_rss_l34_hash_adjust(16847-16863)drivers/net/mlx5/mlx5_rxq.c (2)
mlx5_hrxq_get(3111-3130)mlx5_hrxq_obj_release(3143-3153)
drivers/net/mlx5/rte_pmd_mlx5.h (1)
drivers/net/mlx5/mlx5_flow_hw.c (18)
int(237-253)int(256-269)int(918-935)int(1102-1116)int(1118-1134)int(1136-1151)int(1171-1187)int(1227-1258)int(1281-1303)int(1324-1341)int(1362-1378)int(1397-1472)int(1571-1586)int(1588-1599)int(1601-1622)int(1634-1763)rte_pmd_mlx5_rss_tir_register(579-640)rte_pmd_mlx5_rss_tir_unregister(643-657)
⏰ 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 (3)
drivers/net/mlx5/mlx5_flow_hw.c (1)
5-5: Export header inclusion is appropriateIncluding
<eal_export.h>here is correct and required forRTE_EXPORT_EXPERIMENTAL_SYMBOL. No issues from a build or layering perspective.drivers/net/mlx5/rte_pmd_mlx5.h (2)
12-12: LGTM!The include is necessary for the new RSS TIR registration API that uses
struct rte_flow_action_rss.
630-634: LGTM!The struct design is appropriate for an experimental API. The
destroy_handlefield beingvoid *provides flexibility while the API stabilizes.
| /** | ||
| * Register TIR DevX object for RSS flow action. | ||
| * | ||
| * TIR (Transport Interface Receive) is a hardware object that encodes | ||
| * RSS configuration (hash function, key, destination queues) and serves | ||
| * as the destination for steering rules directing traffic to Rx queues. | ||
| * | ||
| * This function creates a TIR object based on the provided RSS configuration. | ||
| * The returned DevX handle can be used as a destination action in mlx5dv | ||
| * flow steering API from rdma-core, enabling external libraries to direct | ||
| * traffic to DPDK-managed Rx queues. | ||
| * | ||
| * @param[in] port_id | ||
| * The port identifier of the Ethernet device. | ||
| * @param[in] rss | ||
| * RSS flow action configuration. | ||
| * @param[out] devx | ||
| * A pointer to an object that will store returned data. | ||
| * | ||
| * @return | ||
| * - (0) if successful. *devx* will hold DevX object. | ||
| * - (-ENODEV) if *port_id* invalid. | ||
| * - (-EINVAL) if *rss* data was incorrect or port HWS was not activated. | ||
| * - (-ENOMEM) memory allocation error. | ||
| */ | ||
| __rte_experimental | ||
| int | ||
| rte_pmd_mlx5_rss_tir_register(uint16_t port_id, | ||
| const struct rte_flow_action_rss *rss, | ||
| struct rte_pmd_mlx5_rss_devx *devx); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd drivers/net/mlx5 && find . -name "*.c" -o -name "*.h" | head -20Repository: ovsrobot/dpdk
Length of output: 479
🏁 Script executed:
# Search for the implementation of rte_pmd_mlx5_rss_tir_register
rg -n "rte_pmd_mlx5_rss_tir_register" drivers/net/mlx5/ --type cRepository: ovsrobot/dpdk
Length of output: 329
🏁 Script executed:
# Get the implementation and surrounding context
rg -B 5 -A 50 "^int\s+rte_pmd_mlx5_rss_tir_register" drivers/net/mlx5/ --type cRepository: ovsrobot/dpdk
Length of output: 39
🏁 Script executed:
# Get more context around the implementation
rg -B 2 -A 60 "rte_pmd_mlx5_rss_tir_register\(uint16_t" drivers/net/mlx5/mlx5_flow_hw.cRepository: ovsrobot/dpdk
Length of output: 1930
🏁 Script executed:
# Check the file around line 580
sed -n '575,640p' drivers/net/mlx5/mlx5_flow_hw.cRepository: ovsrobot/dpdk
Length of output: 1958
🏁 Script executed:
# Verify if there are any callers/tests that might be passing NULL
rg -n "rte_pmd_mlx5_rss_tir_register" --type c -A 2 | grep -E "(NULL|nullptr)" | head -20Repository: ovsrobot/dpdk
Length of output: 39
🏁 Script executed:
# Check if this is experimental API with documented limitations
rg -B 10 "__rte_experimental" drivers/net/mlx5/rte_pmd_mlx5.h | grep -A 10 "rte_pmd_mlx5_rss_tir_register"Repository: ovsrobot/dpdk
Length of output: 39
🏁 Script executed:
# Check for any tests of this function
find drivers/net/mlx5 -name "*test*" -o -name "*perf*" | xargs grep -l "rte_pmd_mlx5_rss_tir_register" 2>/dev/null || echo "No test files found with function"Repository: ovsrobot/dpdk
Length of output: 90
🏁 Script executed:
# Look for similar register/devx functions to see if NULL checks are typical
rg -n "void.*devx\)" drivers/net/mlx5/ --type c -B 3 | head -40Repository: ovsrobot/dpdk
Length of output: 39
🏁 Script executed:
# Check the exact header location and context around the function declaration
sed -n '630,670p' drivers/net/mlx5/rte_pmd_mlx5.hRepository: ovsrobot/dpdk
Length of output: 1468
Add NULL pointer validation to prevent crashes and update documentation.
The implementation has critical NULL dereference vulnerabilities:
-
Missing NULL check on
rssparameter: The code dereferencesrss->queue_numat line 597 without first validating thatrssis non-NULL, causing an immediate crash if NULL is passed. -
Missing NULL check on
devxparameter: The code writes todevx->destroy_handle,devx->obj, anddevx->idwithout validating thatdevxis non-NULL, causing a crash on output.
Add parameter validation checks for both pointers before use.
Additionally, update the function documentation to clarify:
- Both
rssanddevxpointers must be non-NULL - RSS key is optional (default key used if
rss->keyis NULL) - Thread-safety expectations
- TIR object lifecycle (validity when port is stopped/reconfigured/closed)
🤖 Prompt for AI Agents
In @drivers/net/mlx5/rte_pmd_mlx5.h around lines 636 - 665, The implementation
of rte_pmd_mlx5_rss_tir_register lacks NULL checks and documentation clarity:
add defensive parameter validation at the start of rte_pmd_mlx5_rss_tir_register
to return -EINVAL (or appropriate error) if rss is NULL or devx is NULL before
any dereference (e.g., before using rss->queue_num or writing
devx->destroy_handle/devx->obj/devx->id); treat rss->key as optional (use
default key when rss->key is NULL) and ensure any early return leaves devx
untouched on error; update the function comment to state that rss and devx must
be non-NULL, that rss->key is optional, summarize thread-safety expectations,
and describe TIR object lifecycle relative to port stop/reconfigure/close.
1 participant
NOTE: This is an auto submission for "net/mlx5: add RSS TIR registration API".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36986" for details.
Summary by Sourcery
Add an experimental API to register and unregister mlx5 RSS TIR DevX objects for external steering integration.
New Features:
Enhancements:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.