[PWCI] "dpdk: Update dpdk-lcore-mask handling."#19
Conversation
Moving Russell to emeritus status at his request. Thanks, Russell, for all your contributions and support! Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
OVS adds matches on the DF/CSUM/etc bits of the tunnel info flags, but the DF bit is never actually extracted from the outer IP header during the tunnel decapsulation. This is not a huge problem, as we'll only match on what was parsed out, since matching on DF flag is not exposed through OpenFlow to the users. And since the bit is never extracted, we'll just always match on -df, which sort of "works", because the bit is never extracted and so it is never set. However, this causes misleading -df matches in the datapath flow dumps even if the packets actually have the DF bit set, which it is by default. Fix that by actually extracting the bit from the outer header while decapsulating tunneled traffic. Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Add some more details about dpdk-lcore-mask and remove the recommendation to use it. Signed-off-by: Kevin Traynor <ktraynor@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Default values are not used here, so remove the option for them. Suggested-by: David Marchand <david.marchand@redhat.com> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
OVS currently uses other_config:dpdk-lcore-mask <coremask> directly in DPDK rte_eal_init() with '-c <coremask>' argument. '-c' argument is now deprecated from DPDK and will be removed in DPDK 25.11, so OVS will no longer be able to use the '-c <coremask>' argument. Convert dpdk-lcore-mask core mask to a core list that can be used with '--lcores' and add some tests. The core list is validated to prevent invalid cores being passed to DPDK rte_eal_init(). Using the '--lcores' argument also adds compability for using a core in the core mask that is greater than the max lcore, similar to commit fe53b47 ("dpdk: Fix main lcore on systems with many cores.") Signed-off-by: Kevin Traynor <ktraynor@redhat.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideRefactors dpdk-lcore-mask handling by introducing a hexadecimal mask-to-lcore-list converter integrated via a new param_conversion framework, updates related command-line option mapping and documentation, and fixes tunnel metadata parsing flags. Class diagram for updated DPDK option handling in dpdk.cclassDiagram
class dpdk_options_map {
+ovs_configuration: const char*
+dpdk_option: const char*
+param_conversion: char*(*)(const char*)
}
class cmask_to_lcore_list {
+cmask_to_lcore_list(cmask: const char*): char*
}
dpdk_options_map --> cmask_to_lcore_list: uses param_conversion
class svec {
+add(value: const char*)
}
class smap {
+get(key: const char*): const char*
}
class construct_dpdk_options {
+construct_dpdk_options(ovs_other_config: smap*, args: svec*)
}
construct_dpdk_options --> dpdk_options_map: iterates opts[]
construct_dpdk_options --> svec: adds dpdk options
construct_dpdk_options --> smap: reads config
construct_dpdk_options --> cmask_to_lcore_list: converts mask if needed
Class diagram for tunnel metadata parsing flag updatesclassDiagram
class flow_tnl {
+ip_tos
+ip_ttl
+flags
}
class dp_packet {
}
class ip_extract_tnl_md {
+ip_extract_tnl_md(packet: dp_packet*, tnl: flow_tnl*, ...)
}
ip_extract_tnl_md --> flow_tnl: sets flags
ip_extract_tnl_md --> dp_packet: reads packet
class parse_gre_header {
+parse_gre_header(packet: dp_packet*, tnl: flow_tnl*, ...)
}
parse_gre_header --> flow_tnl: modifies flags
parse_gre_header --> dp_packet: reads packet
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughUpdates DPDK option handling to convert dpdk-lcore-mask into --lcores via a new converter, adjusts tunnel flag propagation (DF, GRE CSUM), revises tests to match DF and MPLS expectations, documents lcore mask behavior, adds a CPU discovery test macro, adds DPDK lcore-mask system tests, and updates maintainers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as ovs-vswitchd
participant O as OVS config (ovsdb)
participant D as DPDK Args Builder
participant Conv as param_conversion
C->>O: Read dpdk options (e.g., dpdk-lcore-mask)
O-->>C: Key-value pairs
C->>D: construct_dpdk_args(options)
loop For each option with value
D->>Conv: Convert value (if converter exists)
alt Conversion succeeds
Conv-->>D: Converted value (e.g., --lcores list)
D-->>C: Append option and converted value
else Conversion fails
Conv-->>D: NULL
D-->>C: Log warning (skip option, reference ovs key)
end
end
C-->>C: Launch DPDK with constructed argv
sequenceDiagram
autonumber
participant M as cmask_to_lcore_list
participant N as ovs_numa_dump_cores_with_cmask
participant S as Sort
M->>N: Get cores for hexadecimal mask
N-->>M: [core_id, socket_id, ...] list
M->>S: Sort by core_id
S-->>M: Sorted list
M-->>M: Truncate to RTE_MAX_LCORE
alt Non-empty
M-->>M: Format "i@core_id" comma list
M-->>Caller: "--lcores" value
else Empty
M-->>Caller: NULL (no valid cores)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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 |
![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 - here's some feedback:
- In cmask_to_lcore_list, consider validating that ovs_numa_dump_cores_with_cmask didn’t return NULL and add cleanup for core_ids on all error paths to avoid potential leaks or null dereferences.
- In construct_dpdk_options, move the free(dpdk_val) inside the block where param_conversion is non‐NULL to make it clear you only free allocated strings and avoid freeing NULL pointers for readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In cmask_to_lcore_list, consider validating that ovs_numa_dump_cores_with_cmask didn’t return NULL and add cleanup for core_ids on all error paths to avoid potential leaks or null dereferences.
- In construct_dpdk_options, move the free(dpdk_val) inside the block where param_conversion is non‐NULL to make it clear you only free allocated strings and avoid freeing NULL pointers for readability.
## Individual Comments
### Comment 1
<location> `lib/dpdk.c:159` </location>
<code_context>
+ }
svec_add(args, opts[i].dpdk_option);
svec_add(args, value);
+ free(dpdk_val);
} else {
VLOG_WARN("Ignoring database defined option '%s' due to "
</code_context>
<issue_to_address>
Potential double-free if param_conversion returns the original pointer.
Verify that param_conversion never returns the original pointer, or document this requirement to prevent double-free errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| svec_add(args, opts[i].dpdk_option); | ||
| svec_add(args, value); | ||
| free(dpdk_val); |
There was a problem hiding this comment.
issue (bug_risk): Potential double-free if param_conversion returns the original pointer.
Verify that param_conversion never returns the original pointer, or document this requirement to prevent double-free errors.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/packet-type-aware.at (1)
695-697: Fix ovs-ofctl option placement and action syntax (parsing hazards).
- Place -OOpenFlowXX immediately after ovs-ofctl (ovs-ofctl -OOpenFlow13 add-flow ...).
- Use decap() instead of decap and use output:PORT (or bare PORT name) instead of output=PORT.
- Affects: tests/packet-type-aware.at:695-697, 745-747, 783-786, 861-863 — update those lines accordingly.
🧹 Nitpick comments (7)
tests/ovs-macros.at (1)
420-434: New CHECK_CPU_DISCOVERED helperPractical and re-usable. Optional: consider POSIX “test” instead of ‘[[ … ]]’ for broader shell portability.
lib/dpdk.c (1)
76-122: Hex mask → --lcores converter (cmask_to_lcore_list)
- Correctly derives cores via ovs_numa_dump_cores_with_cmask(), sorts, truncates to RTE_MAX_LCORE, and formats “i@core_id”.
- Returns NULL on empty/invalid masks, enabling clean fallback.
Optional: truncation might warrant WARN instead of INFO to surface mask over-selection.
Apply if desired:
- VLOG_INFO("Truncating DPDK lcore list as dpdk-lcore-mask has >" - " %u lcores", RTE_MAX_LCORE); + VLOG_WARN("Truncating DPDK lcore list; dpdk-lcore-mask selects more than %u lcores", + RTE_MAX_LCORE);vswitchd/vswitch.xml (2)
350-353: Tighten wording and capitalization; clarify defaults when unset.Use consistent "CPU" capitalization and make the default behavior explicit.
Apply this diff:
- If this option is not specified the value for DPDK initialization - will be auto-determined and all OVS main, revalidator and handler - threads will float on the OVS cpu affinity list. + If this option is not specified, DPDK main/core selection is + auto-determined. OVS main, revalidator, and handler threads are not + pinned by this option and follow OVS CPU affinity defaults.
321-335: Confirm accepted value formats vs. schema.The column is typed as integer, but the text says “hex string (e.g., ‘0x123’)”. Please confirm both decimal and hex inputs are accepted and documented consistently, or adjust the text to “integer (decimal or hex, e.g., 15 or 0xf)”.
If both are accepted, consider:
- Value should be in - the form of a hex string (so '0x123') similar to the 'taskset' mask - input. + Value is an integer mask (decimal or hex, e.g., 15 or 0x0f), + similar to the taskset CPU mask format.Also applies to: 337-353
tests/packet-type-aware.at (3)
512-512: Fix typos in test titles/comments.
- “prerequisits” → “prerequisites”
- “droped” → “dropped”
-AT_SETUP([ptap - check decap() prerequisits]) +AT_SETUP([ptap - check decap() prerequisites])-# Encap(ethernet) on Ethernet frame -> should be droped +# Encap(ethernet) on Ethernet frame -> should be dropped-# Encap(ethernet) on VLAN tagged Ethernet frame -> should be droped +# Encap(ethernet) on VLAN tagged Ethernet frame -> should be droppedAlso applies to: 568-568, 590-590
695-695: Consistency: always print as decap() (matches dumps and prior tests).Use decap() form for clarity and consistency with dumps and earlier rules.
- ovs-ofctl add-flow br0 in_port=n0,actions=decap,output=p0 -OOpenFlow13 && + ovs-ofctl -OOpenFlow13 add-flow br0 in_port=n0,actions=decap(),output:p0 && - ovs-ofctl add-flow br0 -OOpenFlow13 in_port=n0,actions=decap,output=p0 && + ovs-ofctl -OOpenFlow13 add-flow br0 in_port=n0,actions=decap(),output:p0 && - ovs-ofctl add-flow br0 in_port=n0,actions=decap,output=p0 -OOpenFlow13 && + ovs-ofctl -OOpenFlow13 add-flow br0 in_port=n0,actions=decap(),output:p0 &&Also applies to: 741-741, 783-783
978-980: Minor: prefer goto_table:20 over resubmit for readability in this test.Equivalent here, but goto_table:20 keeps the control flow explicit.
- ovs-ofctl add-flow int-br "table=0,in_port=tunnel,actions=pop_mpls:0x800,goto_table:20" && - ovs-ofctl add-flow int-br "table=20,actions=dec_ttl,output:LOCAL" + ovs-ofctl add-flow int-br "table=0,in_port=tunnel,actions=pop_mpls:0x800,goto_table:20" && + ovs-ofctl add-flow int-br "table=20,actions=dec_ttl,output:LOCAL"(Second line unchanged; included for context.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
MAINTAINERS.rst(1 hunks)lib/dpdk.c(2 hunks)lib/netdev-native-tnl.c(2 hunks)tests/nsh.at(2 hunks)tests/ovs-macros.at(1 hunks)tests/packet-type-aware.at(8 hunks)tests/pmd.at(0 hunks)tests/system-dpdk.at(1 hunks)tests/tunnel-push-pop.at(1 hunks)vswitchd/vswitch.xml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/pmd.at
🧰 Additional context used
🧬 Code graph analysis (2)
lib/netdev-native-tnl.c (1)
include/sparse/netinet/in.h (1)
htons(158-161)
lib/dpdk.c (4)
lib/ovs-numa.c (3)
ovs_numa_dump_cores_with_cmask(472-511)ovs_numa_dump_count(551-555)ovs_numa_dump_destroy(557-579)lib/util.c (1)
xmalloc(172-177)lib/dynamic-string.c (3)
ds_put_format(137-145)ds_truncate(46-53)ds_steal_cstr(360-366)lib/svec.c (1)
svec_add(68-72)
⏰ 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). (4)
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-13-5-snap
- GitHub Check: Sourcery review
- GitHub Check: linux rpm fedora
- GitHub Check: linux deb no dpdk
🔇 Additional comments (16)
MAINTAINERS.rst (1)
94-95: Move to Emeritus looks correctEntry formatting and alphabetical placement are consistent with the surrounding table.
tests/tunnel-push-pop.at (1)
688-689: Geneve flags expectation updated to +df-csum+keyMatches DF propagation and CSUM flag preservation changes. Looks good.
lib/netdev-native-tnl.c (2)
155-158: Propagate IPv4 DF to tunnel metadataSetting FLOW_TNL_F_DONT_FRAGMENT when IP_DF is set is correct and endian-safe.
474-475: Preserve existing tunnel flags when GRE CSUM presentSwitching to |= avoids clobbering FLOW_TNL_F_KEY and others. Good fix.
tests/system-dpdk.at (4)
56-70: dpdk-lcore-mask single-bit → lcores conversionAssertion against “lcores 0@0” is appropriate and stable via EAL ARGS logging.
75-88: dpdk-lcore-mask multi-bit (0xf) conversionPre-checking CPU count with CHECK_CPU_DISCOVERED(4) avoids spurious failures; expected mapping is correct.
93-106: dpdk-lcore-mask non-contiguous (0xca) conversionOrdering by core-id then mapping i@core_id is verified by the expected string. Good.
111-124: dpdk-lcore-mask zero handled gracefullyTest validates warning and fallback auto-determined main core; log filter on STOP is a nice touch.
tests/nsh.at (2)
722-724: Tunnel flags updated to +df-csum+keyMatches DF flag propagation and CSUM handling; expectation aligns with datapath behavior.
776-779: Follow-on flags updates across recirc pathConsistent with the earlier change; keeps tests aligned with metadata semantics.
lib/dpdk.c (4)
67-75: Comparator for sorting core IDsStraightforward qsort comparator; fine.
130-134: Option map adds converter hookAdding param_conversion enables centralized transforms; mapping dpdk-lcore-mask → --lcores via converter is clean.
145-160: Conversion flow and error handling
- Skips option on converter failure with a clear warning.
- Avoids duplication when dpdk-extra already supplies the option.
Memory ownership (freeing converted value post svec_add) is correct.
404-435: Auto-determination fallback for --lcoresBehaves correctly when no -c/-l/--lcores are present (e.g., zero mask case); restores thread affinity afterward.
vswitchd/vswitch.xml (1)
344-348: Clarify dpdk-lcore-mask semantics; don’t imply OVS threads are pinned.dpdk-lcore-mask selects the CPU cores that DPDK will use for its lcores (including the DPDK main thread); it does NOT pin ovs-vswitchd main/handler/revalidator threads. PMD threads are unaffected — control their affinity with pmd-cpu-mask. Note: if dpdk-lcore-mask is unset, OVS may auto-determine a DPDK main core from the current thread affinity but restores OVS affinity after initializing DPDK. See lib/dpdk.c (cmask_to_lcore_list / construct_dpdk_args) and lib/dpif-netdev.c (pmd-cpu-mask handling).
Likely an incorrect or invalid review comment.
tests/packet-type-aware.at (1)
327-327: LGTM: DF+CSUM flag and MPLS recirc expectations look correct.The updates to expect flags(+df-csum) on GRE decap paths and the MPLS packet_type transition with recirc are consistent and self-consistent across the dumps.
Also applies to: 345-345, 363-363, 381-382, 400-402, 419-419, 505-505, 1018-1019
| The lowest set core will be assigned in DPDK as the DPDK main thread. | ||
| Other set bits will be assigned by DPDK as DPDK worker threads. | ||
| As OVS implements it's own threading for the datapath (see | ||
| <ref column="other_config" key="pmd-cpu-mask"/>), DPDK worker | ||
| threads will be unused. | ||
| </p> |
There was a problem hiding this comment.
Fix grammar and scope the statement about worker threads.
- "it's" → "its".
- Clarify that OVS does not use DPDK worker lcores for datapath threads, but DPDK services may still use them depending on configuration.
Apply this diff:
- As OVS implements it's own threading for the datapath (see
- <ref column="other_config" key="pmd-cpu-mask"/>), DPDK worker
- threads will be unused.
+ As OVS implements its own datapath threading (see
+ <ref column="other_config" key="pmd-cpu-mask"/>), DPDK worker
+ lcores are not used by OVS datapath threads. Some DPDK services may
+ still use worker lcores depending on configuration.📝 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.
| The lowest set core will be assigned in DPDK as the DPDK main thread. | |
| Other set bits will be assigned by DPDK as DPDK worker threads. | |
| As OVS implements it's own threading for the datapath (see | |
| <ref column="other_config" key="pmd-cpu-mask"/>), DPDK worker | |
| threads will be unused. | |
| </p> | |
| The lowest set core will be assigned in DPDK as the DPDK main thread. | |
| Other set bits will be assigned by DPDK as DPDK worker threads. | |
| As OVS implements its own datapath threading (see | |
| <ref column="other_config" key="pmd-cpu-mask"/>), DPDK worker | |
| lcores are not used by OVS datapath threads. Some DPDK services may | |
| still use worker lcores depending on configuration. | |
| </p> |
🤖 Prompt for AI Agents
In vswitchd/vswitch.xml around lines 337 to 342, fix the grammatical error and
narrow the scope of the statement about worker threads: change "it's" to "its"
and rephrase the paragraph to state that the lowest set core becomes the DPDK
main thread, other set bits become DPDK worker lcores, and that OVS does not use
DPDK worker lcores for its datapath threads (it implements its own threading via
other_config pmd-cpu-mask), while noting that DPDK services or other components
may still use those worker lcores depending on configuration.
3 participants
Auto-submission for "http://patchwork.ozlabs.org/project/openvswitch/list/?series=473242"
Summary by Sourcery
Update DPDK core mask handling by converting hex masks into sorted lcore lists, introduce a generic parameter conversion hook for DPDK options, fix tunnel metadata flag propagation, and clarify dpdk-lcore-mask behavior in documentation.
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit