[PWCI] "config/riscv: add rv64gcv cross compilation target" #87
Conversation
The secondary process should not close socket file for MP channel before performing MP request synchronization. This prevents error logs when the secondary process exits without any operation on the crypto device while the primary process starts the device. Case situation: eal_bus_cleanup has been added in rte_eal_cleanup. But for the secondary process, rte_eal_cleanup firstly performs rte_mp_channel_cleanup, which closes socket file for the MP channel, making mp_fd invalid. Subsequently, eal_bus_cleanup triggers vdev_cleanup, which calls mp_request_sync to send a message via the MP channel. Since mp_fd is invalid, error logs occur. Error logs occur as below when the secondary process exit: EAL: failed to send to (/tmp/dpdk/l2hicu/mp_socket) due to Bad file descriptor EAL: Fail to send request /tmp/dpdk/l2hicu/mp_socket: ipsec_mb_mp_msg USER1: Create MR request to primary process failed. Function call trace: 1. rte_eal_cleanup->rte_mp_channel_cleanup->close_socket_fd 2. rte_eal_cleanup->eal_bus_cleanup->vdev_cleanup-> rte_vdev_driver->ipsec_mb_remove->ipsec_mb_qp_release-> ipsec_mb_secondary_qp_op->rte_mp_request_sync->mp_request_sync-> send_msg->sendmsg(mp_fd, &msgh, 0); Fixes: 1cab1a4 ("bus: cleanup devices on shutdown") Cc: stable@dpdk.org Signed-off-by: Yang Ming <mosesyyoung@gmail.com> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
When a secondary process tries to release a queue pair (QP) that
does not belong to it, error logs occur:
CRYPTODEV: ipsec_mb_ipc_request() line 373: Unable to release
qp_id=0
EAL: Message data is too long
EAL: Fail to handle message: ipsec_mb_mp_msg
EAL: Fail to recv reply for request /tmp/dpdk/l2hi/mp_socket:
ipsec_mb_mp_msg
From the code path, cryptodev->data is allocated in the primary
via rte_cryptodev_data_alloc() (inside
ipsec_mb_create-->rte_cryptodev_pmd_create
-->rte_cryptodev_pmd_allocate-->rte_cryptodev_data_alloc).
This memory is placed in a shared memzone
(rte_cryptodev_data_%u), so both primary and secondary processes
reference the same cryptodev->data, including nb_queue_pairs and
queue_pairs[].
As a result, when the secondary process exits, ipsec_mb_remove()
is called (inside
rte_eal_cleanup-->eal_bus_cleanup-->vdev_cleanup
-->rte_vdev_driver-->ipsec_mb_remove-->ipsec_mb_qp_release
-->ipsec_mb_secondary_qp_op) and it loops through all queue
pairs using:
for (qp_id = 0; qp_id < cryptodev->data->nb_queue_pairs; qp_id++)
ipsec_mb_qp_release(cryptodev, qp_id);
This causes the secondary to attempt releasing queue pairs it
doesn't own, triggering the error logs mentioned above.
This patch ensures that a secondary process only frees a QP if
it actually owns it, preventing conflicts and resolving the
issue.
Fixes: b35848b ("crypto/ipsec_mb: add multi-process IPC request handler")
Cc: stable@dpdk.org
Signed-off-by: Yang Ming <mosesyyoung@gmail.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
Add a cross file for rv64gcv, enable it in devtools/test-meson-builds.sh, and update the RISC-V cross-build guide to support the vector extension. Signed-off-by: Sun Yuechi <sunyuechi@iscas.ac.cn> Signed-off-by: 0-day Robot <robot@bytheb.org>
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adds support for the new RISC-V rv64gcv cross-compilation target by extending the build configuration files and test scripts, and includes two complementary fixes: reordering EAL cleanup calls for resource safety and restricting ipsec_mb QP release to the originating process. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/riscv/meson.build (1)
115-123: Ensure machine_args is always defined and applied for rv64gcv (remove fragile rv64gc guard)Current guard can skip initialization when cpu_instruction_set ≠ 'rv64gc', causing undefined machine_args and failing to apply -march=rv64gcv. Apply unconditionally and fix list append.
Apply this diff:
-if (cpu_instruction_set == 'rv64gc') - # apply supported machine args - machine_args = [] # Clear previous machine args - foreach flag: arch_config['machine_args'] - if cc.has_argument(flag) - machine_args += flag - endif - endforeach -endif +# apply supported machine args (unconditionally for RISC-V) +machine_args = [] # ensure defined +foreach flag: arch_config.get('machine_args', []) + if cc.has_argument(flag) + machine_args += [flag] + endif +endforeach
🧹 Nitpick comments (4)
lib/eal/freebsd/eal.c (1)
909-915: Cleanup order change makes sense; keep MP channel alive during bus teardown.Aligns with Linux: bus cleanup may rely on MP being available; shutting MP down after bus teardown is safer. Consider documenting the rationale inline.
Apply this diff to add a short comment:
struct internal_config *internal_conf = eal_get_internal_configuration(); rte_service_finalize(); eal_bus_cleanup(); +/* Keep MP channel alive until after bus cleanup (matches Linux); + * some teardown paths can rely on MP for coordination. + */ rte_mp_channel_cleanup(); rte_eal_alarm_cleanup();lib/eal/linux/eal.c (1)
1335-1341: LGTM — keep eal_bus_cleanup before VFIO MP sync; add inline commentLinux ordering is correct: eal_bus_cleanup() precedes vfio_mp_sync_cleanup() and rte_mp_channel_cleanup(). Add a short comment to lock the intent.
Location: lib/eal/linux/eal.c (around lines 1336–1341)
rte_service_finalize(); eal_bus_cleanup(); #ifdef VFIO_PRESENT +/* MP remains active until VFIO MP sync completes; keep this order. */ vfio_mp_sync_cleanup(); #endif rte_mp_channel_cleanup();config/riscv/meson.build (1)
140-144: Include stddef.h in the V-intrinsics probe to avoid size_t issuesSome toolchains may not pull size_t via riscv_vector.h. Safer to include stddef.h.
Apply this diff:
- if (cc.compiles('''#include <riscv_vector.h> - int main(void) { size_t vl = __riscv_vsetvl_e32m1(1); }''', args: machine_args)) + if (cc.compiles('''#include <stddef.h> +#include <riscv_vector.h> + int main(void) { size_t vl = __riscv_vsetvl_e32m1(1); (void)vl; }''', args: machine_args))devtools/test-meson-builds.sh (1)
293-296: Unify build directory naming (use hyphens like other targets)For consistency with existing targets (e.g., build-riscv64-generic-gcc), prefer hyphens.
Apply this diff:
-# RISC-V vector (rv64gcv) -f=$srcdir/config/riscv/riscv64_rv64gcv_linux_gcc -build build-riscv64_rv64gcv_gcc $f ABI $use_shared +# RISC-V vector (rv64gcv) +f=$srcdir/config/riscv/riscv64_rv64gcv_linux_gcc +build build-riscv64-rv64gcv-gcc $f ABI $use_shared
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
config/riscv/meson.build(1 hunks)config/riscv/riscv64_rv64gcv_linux_gcc(1 hunks)devtools/test-meson-builds.sh(1 hunks)doc/guides/linux_gsg/cross_build_dpdk_for_riscv.rst(1 hunks)drivers/crypto/ipsec_mb/ipsec_mb_ops.c(2 hunks)lib/eal/freebsd/eal.c(1 hunks)lib/eal/linux/eal.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/eal/freebsd/eal.c (1)
lib/eal/common/eal_common_proc.c (1)
rte_mp_channel_cleanup(670-682)
⏰ 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 (4)
config/riscv/meson.build (1)
46-48: Add rv64gcv arch variant — looks correctNew entry aligns with existing schema and enables passing -march=rv64gcv via arch_config.
doc/guides/linux_gsg/cross_build_dpdk_for_riscv.rst (1)
111-112: Docs: RV64GCV target entry — good additionEntry matches new cross file path and build target.
Consider adding a short note that V intrinsics require GCC ≥14.1 or Clang ≥18.1, mirroring the Meson check, to set user expectations.
config/riscv/riscv64_rv64gcv_linux_gcc (2)
1-7: Binaries block looks fineMatches established pattern (ccache-wrapped riscv64-linux-gnu toolchain).
Please confirm that ccache is available in CI where this cross file is used; otherwise Meson will fail early. If needed, mirror whatever approach is used in config/riscv/riscv64_linux_gcc.
8-18: Host/properties are consistent with vendor_generic rv64gcvcpu/arch_id/vendoring align with meson.build lookup and Debian/Ubuntu pkg-config path.
| ipsec_mb_qp_release(struct rte_cryptodev *dev, uint16_t qp_id) | ||
| { | ||
| struct ipsec_mb_qp *qp = dev->data->queue_pairs[qp_id]; | ||
| uint16_t process_id = (uint16_t)getpid(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Don’t truncate PID to 16 bits; use pid_t (or uint32_t) consistently.
Casting getpid() to uint16_t risks false mismatches or collisions on systems with pid_max > 65535, breaking legitimate releases or enabling cross‑PID collisions.
Apply this diff:
- uint16_t process_id = (uint16_t)getpid();
+ pid_t process_id = getpid();If needed outside this hunk, add near the includes:
#include <sys/types.h>Run to confirm the field widths of qp->qp_used_by_pid and ipsec_mb_mp_param.process_id:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate qp_used_by_pid type:"
rg -n -C3 'qp_used_by_pid'
echo
echo "Locate ipsec_mb_mp_param.process_id type:"
rg -n -C3 'struct\s+ipsec_mb_mp_param\b' -nP
rg -n -C2 '\bprocess_id\b.*;'Length of output: 2771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for struct/typedef 'ipsec_mb_mp_param' and any 'process_id' definitions"
rg -n --hidden --no-ignore-vcs 'struct\s+ipsec_mb_mp_param\b' || true
rg -n --hidden --no-ignore-vcs '\bipsec_mb_mp_param\b' || true
rg -n --hidden --no-ignore-vcs '\bprocess_id\b' || true
echo
echo "Show context for matches under drivers/crypto/ipsec_mb"
rg -n -C3 --hidden --no-ignore-vcs '\bprocess_id\b' drivers/crypto/ipsec_mb || trueLength of output: 5746
Make PID handling consistent — don't truncate to 16 bits
Both qp->qp_used_by_pid and ipsec_mb_mp_param.process_id are uint16_t, so changing only the local variable at drivers/crypto/ipsec_mb/ipsec_mb_ops.c:141 to pid_t is inconsistent and won't eliminate truncation risk.
- Recommended (safe): widen the fields to pid_t (or uint32_t) everywhere — change drivers/crypto/ipsec_mb/ipsec_mb_private.h (qp_used_by_pid at ~line 150, process_id at ~line 176) and all uses in drivers/crypto/ipsec_mb/ipsec_mb_ops.c (assign at ~line 118, local var at ~line 141, comparisons/assigns at ~lines 156, 367, 379). Add #include <sys/types.h> where needed and update any IPC/message layout/serialization that depends on struct size (e.g., len_param usage).
- Alternative (if 16-bit is intentional): keep uint16_t but add an explicit check/guard for getpid() > UINT16_MAX (and document this limitation) so overflow cannot silently occur.
| if (qp->qp_used_by_pid == process_id) | ||
| return ipsec_mb_secondary_qp_op(dev->data->dev_id, | ||
| qp_id, NULL, 0, | ||
| RTE_IPSEC_MB_MP_REQ_QP_FREE); | ||
| } |
There was a problem hiding this comment.
Return error on PID mismatch instead of silently succeeding.
Currently, secondary returns 0 when it does not own the QP, masking a failure and leaking resources. Preserve failure semantics (primary used to reject and signal error).
Apply this diff:
- if (qp->qp_used_by_pid == process_id)
- return ipsec_mb_secondary_qp_op(dev->data->dev_id,
- qp_id, NULL, 0,
- RTE_IPSEC_MB_MP_REQ_QP_FREE);
+ if ((pid_t)qp->qp_used_by_pid != process_id)
+ return -EPERM;
+ return ipsec_mb_secondary_qp_op(dev->data->dev_id,
+ qp_id, NULL, 0,
+ RTE_IPSEC_MB_MP_REQ_QP_FREE);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (qp->qp_used_by_pid == process_id) | |
| return ipsec_mb_secondary_qp_op(dev->data->dev_id, | |
| qp_id, NULL, 0, | |
| RTE_IPSEC_MB_MP_REQ_QP_FREE); | |
| } | |
| if ((pid_t)qp->qp_used_by_pid != process_id) | |
| return -EPERM; | |
| return ipsec_mb_secondary_qp_op(dev->data->dev_id, | |
| qp_id, NULL, 0, | |
| RTE_IPSEC_MB_MP_REQ_QP_FREE); | |
| } |
🤖 Prompt for AI Agents
In drivers/crypto/ipsec_mb/ipsec_mb_ops.c around lines 156 to 160, the secondary
path currently returns 0 when qp->qp_used_by_pid does not match process_id,
silently succeeding and leaking resources; change this to return a failure code
instead of 0. Specifically, detect the PID mismatch and return a non-zero error
(e.g., -EPERM or the module's standard error constant) immediately without
calling ipsec_mb_secondary_qp_op, so the secondary preserves the original
failure semantics the primary used to signal.
NOTE: This is an auto submission for "config/riscv: add rv64gcv cross compilation target".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36200" for details.
Summary by Sourcery
Introduce support for the RV64GCV cross-compilation target in the build system and test harness, while fixing queue release logic in the IPsec-MB driver and correcting cleanup ordering in EAL on FreeBSD and Linux.
New Features:
Bug Fixes:
Build:
Summary by CodeRabbit