Dynamic Loading IB verbs#253
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an IBV_DIRECT build/runtime switch intended to support resolving libibverbs symbols either via direct linkage or via dlopen/dlsym, and adds a runtime check to block NIC executor usage when verbs aren’t available.
Changes:
- Added
IBV_DIRECTmode plumbing (Makefile + CMake option) to toggle direct symbol binding vsdlsymresolution. - Introduced
pfn_*function pointers and updated some verbs call sites/macros to call through them. - Added
System::IbvLoaded()and a guard to error out when NIC executor is requested but verbs aren’t loaded.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/header/TransferBench.hpp | Adds libibverbs dynamic-loading infrastructure (pfn_*, Ibvdl()), runtime loaded-state tracking, and uses pfn_* in some verbs calls. |
| Makefile | Adds DISABLE_IBV_DIRECT flag and sets -DIBV_DIRECT=1 by default when NIC exec is enabled. |
| CMakeLists.txt | Adds ENABLE_IBV_DIRECT option and defines IBV_DIRECT=1 when enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LDFLAGS += -libverbs | ||
| NIC_ENABLED = 1 | ||
| ifneq ($(DISABLE_IBV_DIRECT),1) | ||
| COMMON_FLAGS += -DIBV_DIRECT=1 |
There was a problem hiding this comment.
When DISABLE_IBV_DIRECT=1 (dlsym path), the build still unconditionally links -libverbs. Any remaining direct ibv_* calls (and even an unused -libverbs without --as-needed) can keep libibverbs as a hard runtime dependency, undermining the goal of optional runtime loading. Consider omitting -libverbs when using the dlsym path (and ensure all ibv_* usage goes through pfn_*), and add -ldl for toolchains/glibc versions where dlopen is not in libc.
| LDFLAGS += -libverbs | |
| NIC_ENABLED = 1 | |
| ifneq ($(DISABLE_IBV_DIRECT),1) | |
| COMMON_FLAGS += -DIBV_DIRECT=1 | |
| NIC_ENABLED = 1 | |
| ifneq ($(DISABLE_IBV_DIRECT),1) | |
| COMMON_FLAGS += -DIBV_DIRECT=1 | |
| LDFLAGS += -libverbs | |
| else | |
| LDFLAGS += -ldl |
| target_link_libraries(TransferBench PRIVATE ${IBVERBS_LIBRARY}) | ||
| target_compile_definitions(TransferBench PRIVATE NIC_EXEC_ENABLED) | ||
| if(ENABLE_IBV_DIRECT) |
There was a problem hiding this comment.
ENABLE_IBV_DIRECT=OFF claims to resolve symbols via dlsym at runtime, but the target still links against ${IBVERBS_LIBRARY} unconditionally when IBVERBS_FOUND. This keeps libibverbs as a hard dependency and defeats optional loading. If runtime optionality is intended, avoid linking ${IBVERBS_LIBRARY} when ENABLE_IBV_DIRECT is OFF and ensure all ibv_* usage is through the loaded function pointers.
| target_link_libraries(TransferBench PRIVATE ${IBVERBS_LIBRARY}) | |
| target_compile_definitions(TransferBench PRIVATE NIC_EXEC_ENABLED) | |
| if(ENABLE_IBV_DIRECT) | |
| target_compile_definitions(TransferBench PRIVATE NIC_EXEC_ENABLED) | |
| if(ENABLE_IBV_DIRECT) | |
| target_link_libraries(TransferBench PRIVATE ${IBVERBS_LIBRARY}) |
| // Query the number of IBV devices | ||
| int numIbvDevices = 0; | ||
| ibv_device** deviceList = ibv_get_device_list(&numIbvDevices); | ||
| ibv_device** deviceList = pfn_ibv_get_device_list(&numIbvDevices); | ||
|
|
There was a problem hiding this comment.
Within GetIbvDeviceList(), most verbs calls were converted to pfn_* (e.g., pfn_ibv_get_device_list/pfn_ibv_open_device/etc.), but the function still calls ibv_free_device_list(deviceList) directly later in the same function. In the dlsym path this reintroduces a direct libibverbs symbol reference and breaks the “all calls through pfn_*” model. Switch that cleanup call to pfn_ibv_free_device_list as well.
|
i'd say just merge into |
Motivation
Technical Details
Created a separate
IbvDynload.hppheader in src for the purpose of dynamic loading of ibverbs, which originalTransferBench.hppwould depend on.Test Plan
Tested all combination of
NICIBVNVMLPODMPIenablement and confirmed no compilation or linking error.Test Result
Submission Checklist