RMA function & benchmarks refactor #47
Conversation
WalkthroughThe changes add a new shared library ( Changes
Sequence Diagram(s)sequenceDiagram
participant M as Main (Benchmark)
participant O as Options Parser
participant RM as Resource Manager
participant E as Executor
M->>O: Parse command-line arguments (rfaas::benchmark::options)
O-->>M: Return opts struct
alt executors_database provided
M->>E: Deserialize executor database (skip RM connection)
else
M->>RM: Attempt connection to resource manager
RM-->>M: Return connection status
end
M->>E: Lease executor (pass skip_resource_manager flag)
M->>M: Execute benchmarking operations (RMA post_write/post_read)
M-->>M: Log results and perform cleanup
sequenceDiagram
participant F as RMA Function ("empty")
participant P as RDMA Passive Connection
participant M as Memory Manager
F->>F: Cast arguments to RmaFunctionConfig
F->>P: Initialize RDMA passive connection (using IP & port)
P-->>F: Connection established
F->>M: Register memory buffers (memory_data, memory_cfg)
loop Polling for events (100ms timeout)
F->>P: Check for connection request
alt Connection received
P-->>F: Connection request event
F->>P: Send memory configuration (memory_cfg)
else
F->>F: Continue polling
end
end
F-->>F: Return size parameter after completion
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (18)
examples/rma_functions.hpp (1)
1-14: New RMA function configuration structure.The new header file defines a configuration structure for Remote Memory Access (RMA) functions, containing client IP address, port, and memory size parameters. This supports the new RMA functionality being introduced.
However, consider the following improvements:
- The constant
IPV4_ADDRESS_STRING_LENGTHis set to 16, but IPv4 addresses can be up to 15 characters (e.g., "255.255.255.255") plus a null terminator. While 16 is sufficient, consider adding a comment explaining this choice.- Consider using
std::stringfor the IP address instead of a fixed-size character array to avoid potential buffer overflows.benchmarks/warm_benchmark.cpp (1)
60-63: Unified lease check
Combining the lease checks into one block simplifies logic. If you want more clarity, you could log which lease acquisition path (resource manager vs. executors database) was used before failing. Otherwise, this is fine.examples/rma_functions.cpp (4)
7-14: Use a consistent logging mechanism
Currently, the function logs usingstd::cerr. Since the rest of the codebase usesspdlog, consider switching tospdlogfor consistency, better log formatting, and configurability.
16-21: Add error handling for connection setup
rdmalib::RDMAPassive _state(...)may fail for various reasons (e.g., port conflict, misconfiguration). Consider wrapping this in a try/catch or adding checks to handle initialization errors gracefully.
22-28: Avoid magic constants in memory configuration
Memory configuration uses a fixed 12-byte buffer. If it’s intended for address (8 bytes) plus rkey (4 bytes), define descriptive constants or a struct to avoid “12” and “8” as magic numbers.
29-53: Infinite loop risk on connection polling
Thewhile(true)loop will run indefinitely if no clients connect or disconnect. If that’s intended, consider adding a mechanism to exit gracefully after a certain time or upon receiving a stop signal, preventing potential resource leaks or hung processes in production.benchmarks/cold_benchmark.cpp (1)
45-58: Conditional resource manager usage
Introducingskip_resource_managerbased on whetherexecutors_databaseis provided is a good way to allow alternative execution paths. It could be helpful to log which path is being taken for clarity.benchmarks/rma.cpp (4)
18-18: Address the TODO comment.
There's a placeholder comment here. Consider clarifying the pending work or removing the TODO if it's no longer needed.Would you like me to help implement or open a new issue to track the missing functionality?
105-112: Avoid indefinite retries when connecting to the RMA function.
Currently, the code loops forever attempting to establish a connection. Consider adding a maximum retry limit or a timeout to prevent potential blocking if the server is unreachable.while (true) { rdmalib::RDMAActive tmp_active(rma_config.client_ip_address, rma_config.client_port, 32, 0); if (tmp_active.connect()) { active = std::move(tmp_active); break; } - std::this_thread::sleep_for(std::chrono::milliseconds(100)); + // Limit to e.g. 50 retries + for (int retries = 0; retries < 50; ++retries) { + if (tmp_active.connect()) { + active = std::move(tmp_active); + break; + } + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + } + if (!active.is_valid()) { + spdlog::error("Unable to connect after 50 attempts."); + return 1; + } }
124-130: Validate alignment when extracting remote address.
Although this is typically safe on most systems, consider explicitly checking buffer alignment or usingstd::memcpy()to avoid potential alignment issues when casting touint64_tanduint32_t.
145-152: Consider adding separate error handling for read vs. write operations.
Posting and polling are handled identically, but it might be helpful to differentiate error logs for read vs. write to improve debugging clarity.if (opts.rma_mode) { active.connection().post_write(input.sge(buf_size, 0), {r_address, r_key}, false); - spdlog::debug("Posted write {}", (input.data()[0])); + spdlog::debug("Posted write of size {}", buf_size); } else { active.connection().post_read(input.sge(buf_size, 0), {r_address, r_key}); - spdlog::debug("Posted read {}", (input.data()[0])); + spdlog::debug("Posted read of size {}", buf_size); }docs/benchmarks.md (4)
2-2: Insert a comma after “environment.”
To improve readability, add a comma after 'environment' as flagged by the static analysis.-To set up the benchmark environment please follow the [tutorial](tutorial.md) first. +To set up the benchmark environment, please follow the [tutorial](tutorial.md) first.🧰 Tools
🪛 LanguageTool
[typographical] ~2-~2: Consider adding a comma here.
Context: ...ons. To set up the benchmark environment please follow the tutorial firs...(PLEASE_COMMA)
15-16: Avoid duplicating the word “Benchmark” in headings and descriptions.
Static analysis hints indicate repeated words. Removing or rephrasing the second “Benchmark” can improve clarity.-## Warm Invocations Benchmark -Benchmark for warm function invocations. +## Warm Invocations Benchmark +Perform warm function invocations. -## Cold Invocations Benchmark -Benchmark for cold function invocations. +## Cold Invocations Benchmark +Perform cold function invocations. -## Parallel Invocations Benchmark -Benchmark for warm parallel function invocations. +## Parallel Invocations Benchmark +Perform parallel function invocations. -## RMA Function Benchmark -Benchmark for reads/writes to remote function memory. +## RMA Function Benchmark +Reads/writes to remote function memory.Also applies to: 23-24, 32-33, 42-43
🧰 Tools
🪛 LanguageTool
[duplication] ~15-~15: Possible typo: you repeated a word.
Context: ...base.json -s 1 ``` ## Warm Invocations Benchmark Benchmark for warm function invocations. * App: `...(ENGLISH_WORD_REPEAT_RULE)
20-21: Resolve Markdown indentation issues.
markdownlint-cli2reports inconsistent indentation levels for list items. Consider aligning them consistently to enhance readability.- * `-s <size>` for function invocation payload size in bytes - * `--output-stats <filename>` optional to output measurements as csv + * `-s <size>` for function invocation payload size in bytes + * `--output-stats <filename>` optional to output measurements as csv ...Also applies to: 28-30, 37-39, 47-51
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
11-11: Specify a language for fenced code blocks.
markdownlintflags these code blocks for missing language spec. Adding a language improves syntax highlighting.-``` +```bash <build-dir>/benchmarks/cpp_interface --config ...-``` +```bash <build-dir>/benchmarks/rma --config ...Also applies to: 54-54
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
benchmarks/cpp_interface.cpp (1)
69-70: Ensure theskip_resource_managerflag is passed consistently.
Passingskip_resource_managertoexecutor.allocateis structurally correct. Verify other calls toallocate(if any) handle this flag similarly.benchmarks/parallel_invocations.cpp (2)
40-42: Consider consistent naming for core countThere's an inconsistency between
opts.coresused here andsettings.benchmark.numcoresused throughout the rest of the file. Consider standardizing on one naming convention across the codebase for better maintainability.if(opts.cores > 0) { - settings.benchmark.numcores = opts.cores; + settings.benchmark.cores = opts.cores; }Then update all references to
numcoreselsewhere in the file to usecores.
48-64: Enhanced flexibility with resource manager connectionAdding the option to skip connecting to the resource manager when an executors database is provided is a good enhancement that improves flexibility and testing capabilities.
Consider improving error message consistency between the "Connection to resource manager failed!" and "Couldn't acquire a lease!" cases for better user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
CMakeLists.txt(1 hunks)benchmarks/cold_benchmark.cpp(3 hunks)benchmarks/cold_benchmark.hpp(0 hunks)benchmarks/cold_benchmark_opts.cpp(0 hunks)benchmarks/cpp_interface.cpp(2 hunks)benchmarks/cpp_interface.hpp(0 hunks)benchmarks/cpp_interface_opts.cpp(0 hunks)benchmarks/parallel_invocations.cpp(2 hunks)benchmarks/parallel_invocations.hpp(0 hunks)benchmarks/parallel_invocations_opts.cpp(0 hunks)benchmarks/rma.cpp(1 hunks)benchmarks/settings.cpp(2 hunks)benchmarks/settings.hpp(1 hunks)benchmarks/warm_benchmark.cpp(2 hunks)benchmarks/warm_benchmark.hpp(0 hunks)benchmarks/warm_benchmark_opts.cpp(0 hunks)cmake/benchmarks.cmake(1 hunks)cmake/dependencies.cmake(1 hunks)docs/benchmarks.md(1 hunks)docs/tutorial.md(1 hunks)examples/rma_functions.cpp(1 hunks)examples/rma_functions.hpp(1 hunks)rdmalib/include/rdmalib/connection.hpp(1 hunks)rdmalib/include/rdmalib/poller.hpp(0 hunks)rdmalib/lib/connection.cpp(1 hunks)rfaas/lib/executor.cpp(0 hunks)
💤 Files with no reviewable changes (10)
- benchmarks/cpp_interface.hpp
- rfaas/lib/executor.cpp
- benchmarks/cpp_interface_opts.cpp
- benchmarks/cold_benchmark.hpp
- rdmalib/include/rdmalib/poller.hpp
- benchmarks/warm_benchmark.hpp
- benchmarks/cold_benchmark_opts.cpp
- benchmarks/parallel_invocations.hpp
- benchmarks/parallel_invocations_opts.cpp
- benchmarks/warm_benchmark_opts.cpp
🧰 Additional context used
🧬 Code Definitions (1)
benchmarks/settings.cpp (1)
benchmarks/settings.hpp (1)
options(77-77)
🪛 LanguageTool
docs/benchmarks.md
[typographical] ~2-~2: Consider adding a comma here.
Context: ...ons. To set up the benchmark environment please follow the tutorial firs...
(PLEASE_COMMA)
[duplication] ~15-~15: Possible typo: you repeated a word.
Context: ...base.json -s 1 ``` ## Warm Invocations Benchmark Benchmark for warm function invocations. * App: `...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~23-~23: Possible typo: you repeated a word.
Context: ...easurements as csv ## Cold Invocations Benchmark Benchmark for cold function invocations. * App: `...
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~32-~32: Possible typo: you repeated a word.
Context: ...een iterations ## Parallel Invocations Benchmark Benchmark for warm parallel function invocations....
(ENGLISH_WORD_REPEAT_RULE)
[duplication] ~42-~42: Possible typo: you repeated a word.
Context: ...l functions to invoke ## RMA Function Benchmark Benchmark for reads/writes to remote function mem...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
docs/benchmarks.md
11-11: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
29-29: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
30-30: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (19)
docs/tutorial.md (1)
285-285: Improved hyperlink format.The hyperlink format has been improved to make the destination more explicit, changing from "at the documentation" to "benchmarks.md".
cmake/dependencies.cmake (1)
29-29: Enable position-independent code for spdlog.Setting POSITION_INDEPENDENT_CODE property to ON is appropriate when building shared libraries, as it ensures the code can be properly loaded at any memory address.
rdmalib/lib/connection.cpp (1)
261-270: Added RDMA read operation capability.The new
post_readmethod enables performing RDMA read operations by setting the appropriate opcode (IBV_WR_RDMA_READ) and leveraging the existing_post_writeimplementation. This is a logical extension of the existing RDMA functionality.rdmalib/include/rdmalib/connection.hpp (1)
146-146:❓ Verification inconclusive
Appropriate addition of RDMA read operation
The new
post_readmethod complements the existing write operations and enhances the RDMA connection API by providing read functionality. This is consistent with the PR goal of improving RMA (Remote Memory Access) capabilities.
🏁 Script executed:
#!/bin/bash # Check if the corresponding implementation for post_read exists # and is being properly used in the RMA functions # Find implementation of post_read rg -A 3 "post_read.*ScatterGatherElement.*RemoteBuffer" rdmalib/lib/ # Find usage of post_read in RMA functions rg "post_read" examples/rma_functions.cppLength of output: 366
Confirmation: RDMA Read Operation Implemented
The new
post_readmethod has been correctly implemented inrdmalib/lib/connection.cpp, matching the declaration inrdmalib/include/rdmalib/connection.hppand complementing the existing write operations. One thing to note: a search inexamples/rma_functions.cppdid not reveal any usages ofpost_read. Please verify that this functionality is integrated or exercised where required in your RMA workflow.benchmarks/settings.hpp (2)
61-75: Good centralization of benchmark optionsThe
Optionsstructure effectively centralizes configuration parameters across all benchmarks, including the new RMA functionality. This promotes consistency and reduces code duplication.
77-77: Well-designed function declaration for options parsingThis function provides a unified way to parse command line arguments across all benchmarks, supporting the refactoring goal of the PR.
CMakeLists.txt (1)
201-207:❓ Verification inconclusive
Correctly configured RMA functions library
The new
rma_functionsshared library is properly set up with position-independent code and appropriate dependencies. The setup mirrors the existingfunctionslibrary configuration for consistency.
🏁 Script executed:
#!/bin/bash # Check if the rma_functions.cpp file exists and contains appropriate content # Check for file existence if [[ -f "examples/rma_functions.cpp" ]]; then echo "File exists: examples/rma_functions.cpp" # Check for key RDMA operations in the file echo "Looking for key RDMA operations..." grep -E "post_(read|write)" examples/rma_functions.cpp else echo "Error: examples/rma_functions.cpp not found" exit 1 fiLength of output: 262
Action Required: Manual Confirmation of RDMA Operations in rma_functions.cpp
The CMake configuration for the new
rma_functionsshared library is consistent with the existingfunctionslibrary—ensuring position-independent code and setting the proper output directory along with the correct dependencies. The fileexamples/rma_functions.cppis present as expected. However, our automated check did not detect any occurrences of RDMA operations (e.g.,post_readorpost_write) in the file. Please manually verify whether the absence of these keywords is intentional or if additional RDMA-related code is required.
- Confirm whether
examples/rma_functions.cppshould contain key RDMA operations.- If these RDMA operations are expected, update the source accordingly; otherwise, document that their omission is by design.
benchmarks/settings.cpp (1)
26-66: Well-implemented options parsing functionThe implementation effectively:
- Configures command line options using cxxopts, including required RMA parameters
- Handles help display appropriately
- Populates the Options structure consistently
This implementation supports the PR's goal of unifying benchmark options across the codebase.
benchmarks/warm_benchmark.cpp (2)
20-20: Use of new unified options function
Switching from a benchmark-specific options parser torfaas::benchmark::optionsaligns the warm benchmark’s configuration with the refactored approach used across other benchmarks. This looks consistent and should help maintain a single source of truth for option parsing.
26-26: Log statement rename
Renaming the log message to “warm benchmark!” keeps it consistent with the naming in other files and clarifies what the benchmark does.benchmarks/cold_benchmark.cpp (3)
22-22: Adoption of unified options
Replacing the oldcold_benchmarker::optswithrfaas::benchmark::optionsimproves consistency across benchmarks and reduces code duplication.
28-28: Updated info log
Renaming the log statement to “Executing serverless-rdma test cold benchmark!” helps keep logs consistent with the rest of the refactored files.
71-77: Optional executor approach
Using anstd::optionalforleased_executoris cleaner than forcing a single approach. This retains the same error check if leasing fails. Discussions about adding extra logs (e.g. for partial leases or resource constraints) might be worthwhile, but the current approach is functionally sound.benchmarks/cpp_interface.cpp (2)
21-21: Configuration approach looks good.
Switching torfaas::benchmark::options(argc, argv)aligns this file with changes across other benchmarks and consolidates option handling.
45-55: Check if skipping the Resource Manager is intended.
Conditional logic forskip_resource_managerensures flexibility, but confirm that local executors are fully supported as an alternative.Please verify references to executors leased outside the resource manager, ensuring no missed initialization steps in the broader codebase.
cmake/benchmarks.cmake (2)
5-5: Good practice for third-party librariesAdding cxxopts as a SYSTEM include is good practice as it suppresses compiler warnings from third-party code, which helps distinguish your own codebase's warnings from those in external dependencies.
8-13: LGTM: Improved benchmark target organizationThe changes simplify benchmark executable definitions by removing separate options files, which improves maintainability. The addition of the new
rmaexecutable aligns perfectly with the PR objective of introducing RMA functionality.benchmarks/parallel_invocations.cpp (2)
21-21: Improved options handling consistencyThe change from a benchmark-specific options function to the common
rfaas::benchmark::optionsfunction aligns with good refactoring practices and ensures consistency across different benchmarks.
72-73: Updated allocation with skip_resource_manager parameterThe allocation call has been correctly updated to include the new
skip_resource_managerparameter, ensuring proper integration with the enhanced connection setup process.
1 participant
Summary by CodeRabbit
New Features
Documentation
Refactor