infra: add trace deepcopy #415
Conversation
christophefontaine
force-pushed
the
gr_mbuf_deepcopy
branch
from
a14ffda to
22aaf73
Compare
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdded Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/infra/datapath/gr_mbuf.h(1 hunks)modules/infra/datapath/trace.c(1 hunks)modules/ip/datapath/ip_fragment.c(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/datapath/gr_mbuf.hmodules/infra/datapath/trace.cmodules/ip/datapath/ip_fragment.c
🧬 Code graph analysis (3)
modules/infra/datapath/gr_mbuf.h (1)
modules/infra/datapath/trace.c (2)
gr_mbuf_trace_copy(621-653)gr_mbuf_trace_finish(655-671)
modules/infra/datapath/trace.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_traces(47-49)
modules/ip/datapath/ip_fragment.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_copy(79-87)
⏰ 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). (3)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: rpm
🔇 Additional comments (3)
modules/ip/datapath/ip_fragment.c (1)
99-99: LGTM! Trace-aware fragment copy.The switch to
gr_mbuf_copycorrectly preserves trace data when creating fragment copies, which is essential for fragments that take different paths through the network stack.modules/infra/datapath/gr_mbuf.h (1)
78-87: LGTM! Clean deep-copy helper.The implementation correctly performs a full mbuf copy including private data and traces. The sequence of operations (mbuf copy → priv data copy → trace copy) is sound.
modules/infra/datapath/trace.c (1)
621-653: LGTM with a note on intended usage.The implementation correctly performs a deep copy of all trace items. The allocation loop with eviction ensures robustness.
Note: Line 628's
STAILQ_INITwould leak any pre-existing independent traces indst. This is fine for the intended usage (viagr_mbuf_copyon a freshly copied mbuf), but calling this function standalone on an mbuf that already has traces would leak them. Consider adding a comment clarifying the expected usage pattern.
grout-bot
force-pushed
the
gr_mbuf_deepcopy
branch
from
22aaf73 to
44ee664
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/infra/datapath/gr_mbuf.h (1)
67-73: Return value doc mismatch already flagged.The implementation in trace.c always returns 0 (never fails). See past review.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/infra/datapath/gr_mbuf.h(1 hunks)modules/infra/datapath/trace.c(1 hunks)modules/ip/datapath/ip_fragment.c(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/datapath/trace.cmodules/infra/datapath/gr_mbuf.hmodules/ip/datapath/ip_fragment.c
🧬 Code graph analysis (3)
modules/infra/datapath/trace.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_traces(47-49)
modules/infra/datapath/gr_mbuf.h (1)
modules/infra/datapath/trace.c (2)
gr_mbuf_trace_copy(621-653)gr_mbuf_trace_finish(655-671)
modules/ip/datapath/ip_fragment.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_copy(79-87)
🔇 Additional comments (2)
modules/infra/datapath/trace.c (1)
621-653: LGTM.The deep copy logic is correct: resets destination, allocates new trace items (evicting oldest if necessary), copies all fields including payload. Pattern matches existing
gr_mbuf_trace_add.modules/ip/datapath/ip_fragment.c (1)
99-102: Good use ofgr_mbuf_copy, but note the upstream fix needed.The switch to
gr_mbuf_copycorrectly preserves traces for fragmented packets. Once the buffer overflow ingr_mbuf_copyis fixed (see gr_mbuf.h review), consider whether line 125's explicit*ip_output_mbuf_data(frag_mbuf) = *ip_output_mbuf_data(mbuf)is still needed—it may become redundant.
christophefontaine
force-pushed
the
gr_mbuf_deepcopy
branch
from
44ee664 to
47d1a30
Compare
|
@rjarry I updated the size of the copy, you'll want to double check before merging :) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/infra/datapath/gr_mbuf.h(1 hunks)modules/infra/datapath/trace.c(1 hunks)modules/ip/datapath/ip_fragment.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/ip/datapath/ip_fragment.c
- modules/infra/datapath/gr_mbuf.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/datapath/trace.c
🧬 Code graph analysis (1)
modules/infra/datapath/trace.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_traces(47-49)
christophefontaine
force-pushed
the
gr_mbuf_deepcopy
branch
2 times, most recently
from
a4107ab to
a3108b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
modules/infra/datapath/gr_mbuf.h (2)
67-71: Clarifygr_mbuf_trace_copypreconditions ondsttraces
gr_mbuf_trace_copyreinitializes the destination trace head before appending new items, so calling it on an mbuf that already has traces would drop (and effectively leak) the existing chain. If the intended invariant is “dst has no traces” (only used on fresh clones), it’d be good to document that here or enforce it with anassert(!gr_mbuf_is_traced(dst))to catch accidental misuse.
76-88: Document and verify assumptions betweenmbuf_data_typeandmbuf_data()layout
__gr_mbuf_copyalways copies from/tombuf_data(m)/mbuf_data(copy), whilepriv_lencomes fromsizeof(mbuf_data_type)via the macro. This relies on every caller passing a type whose size/layout matches the region pointed to bymbuf_data(), otherwise you risk silent over/under‑copy (and potential overwrite of adjacent private data). I’d explicitly state that contract in the comment (and ideally keep callers using a single typedef/alias for the mbuf private area) sogr_mbuf_copy(m, data_len, ...)can’t be accidentally used with an incompatible struct like a larger per‑feature view.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/infra/datapath/gr_mbuf.h(1 hunks)modules/infra/datapath/trace.c(1 hunks)modules/ip/datapath/ip_fragment.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/ip/datapath/ip_fragment.c
- modules/infra/datapath/trace.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/datapath/gr_mbuf.h
🧠 Learnings (1)
📚 Learning: 2025-10-03T13:34:31.801Z
Learnt from: rjarry
Repo: DPDK/grout PR: 329
File: modules/ip/cli/route.c:51-66
Timestamp: 2025-10-03T13:34:31.801Z
Learning: The `gr_api_client_stream_foreach` macro in `api/gr_api.h` always sets its `ret` parameter during the first outer loop condition evaluation (via `if (__first == 1) ret = __id;`), even when the stream yields zero items. Therefore, the `ret` variable does not need to be initialized at declaration when used with this macro.
Applied to files:
modules/infra/datapath/gr_mbuf.h
🧬 Code graph analysis (1)
modules/infra/datapath/gr_mbuf.h (1)
modules/infra/datapath/trace.c (2)
gr_mbuf_trace_copy(621-651)gr_mbuf_trace_finish(653-669)
⏰ 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). (8)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-cross-aarch64
- GitHub Check: deb
- GitHub Check: rpm
christophefontaine
force-pushed
the
gr_mbuf_deepcopy
branch
from
a3108b5 to
54e864f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
modules/infra/datapath/trace.c (1)
621-651: Makesrcconst and document/guard the “freshdst” assumptionThe copy loop never mutates
src, so the signature can safely be:-void gr_mbuf_trace_copy(struct rte_mbuf *dst, struct rte_mbuf *src) { +void gr_mbuf_trace_copy(struct rte_mbuf *dst, const struct rte_mbuf *src) {Also,
STAILQ_INIT(dst_traces)assumesdsthas no live traces; if this function is ever used on reused mbufs, existing trace chains would be dropped and leaked. Given current usage via freshly-allocated copies this is fine, but you might want a debug guard to lock in that contract:void gr_mbuf_trace_copy(struct rte_mbuf *dst, const struct rte_mbuf *src) { struct gr_trace_head *src_traces = gr_mbuf_traces(src); struct gr_trace_head *dst_traces = gr_mbuf_traces(dst); @@ - // Reset trace head - STAILQ_INIT(dst_traces); + // dst is expected to be a fresh mbuf without traces. + assert(STAILQ_EMPTY(dst_traces)); + STAILQ_INIT(dst_traces);
🧹 Nitpick comments (1)
modules/ip/datapath/ip_fragment.c (1)
83-90: Clarify whether duplicatedip_fragmenttrace entries on later fragments are intendedWith
gr_mbuf_copy(mbuf, ...)cloning all existing trace items and theip_fragment_trace_datafor fragment 0 being added tombufbefore the loop, every extra fragment now inherits that “frag=0 offset=0 MF” entry and then gets its ownfrag=ientry viagr_mbuf_trace_add(frag_mbuf, ...). Fori > 0, trace dumps will show twoip_fragmentrecords per packet.If you only want per-fragment entries (plus the shared pre-fragment history), consider reordering so that:
- you create the extra fragments with
gr_mbuf_copywhilembufstill has noip_fragment_trace_data, and- then add one
ip_fragment_trace_dataper fragment (including the first) afterwards.Otherwise, maybe add a short comment that this duplication is deliberate so future readers don’t second‑guess it.
Also applies to: 97-135
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/infra/datapath/gr_mbuf.h(1 hunks)modules/infra/datapath/trace.c(1 hunks)modules/ip/datapath/ip_fragment.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/infra/datapath/gr_mbuf.h
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/datapath/trace.cmodules/ip/datapath/ip_fragment.c
🧬 Code graph analysis (2)
modules/infra/datapath/trace.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_traces(47-49)
modules/ip/datapath/ip_fragment.c (1)
modules/infra/datapath/gr_mbuf.h (1)
gr_mbuf_copy(77-85)
⏰ 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). (7)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: deb
- GitHub Check: rpm
christophefontaine
force-pushed
the
gr_mbuf_deepcopy
branch
from
54e864f to
8e4ea61
Compare
christophefontaine
force-pushed
the
gr_mbuf_deepcopy
branch
from
8e4ea61 to
21f237c
Compare
grout-bot
force-pushed
the
gr_mbuf_deepcopy
branch
from
21f237c to
1bc286d
Compare
For packet duplication or fragmentation, traces must be duplicated as the packet may go through different paths. Add: - a helper to copy traces - a helper to copy the mbuf, its payload, specific mbuf_data and traces. Suggested-by: David Marchand <[email protected]> Signed-off-by: Christophe Fontaine <[email protected]> Reviewed-by: Anthony Harivel <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
grout-bot
force-pushed
the
gr_mbuf_deepcopy
branch
from
1bc286d to
adbab34
Compare
3 participants
For packet duplication or fragmentation, traces must be duplicated as the packet may go through different paths.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.