Skip to content

Commit 2c238c0

Browse files
committed
libfabric: extract GDRCopy memhandle registration into a helper
Pulls the v1/v2 pin/map/get_info block and subsequent cpu_ptr offset and metadata bookkeeping out of the main register-handle flow and into a dedicated libfabric_gdr_register_memhandle() helper, grouped with the other GDRCopy-init helpers in the file's anonymous namespace. Also moves the now-local gdr_info_t into the helper. Pure refactor: no behavior change, verified via a clean full build. Note: also fixes a pre-existing leak — if pin_buffer[_v2] succeeded but a subsequent map[_v2] or get_info failed, the pinned (and possibly mapped) GDRCopy state was leaked. Adds best-effort unmap + unpin cleanup on all failure paths inside the extracted helper, logging but not overwriting the primary error status. Signed-off-by: Eric Raut <eraut@amazon.com>
1 parent 0726f42 commit 2c238c0

1 file changed

Lines changed: 80 additions & 40 deletions

File tree

src/modules/transport/libfabric/libfabric.cpp

Lines changed: 80 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,83 @@ int libfabric_configure_gdrcopy_v2(nvshmemt_libfabric_state_t *libfabric_state)
188188
out:
189189
return status;
190190
}
191+
192+
/* Register a device-memory buffer with GDRCopy: pin, map, compute the
193+
* user-visible CPU pointer (accounting for 64KB page alignment), and record
194+
* the mapping info on handle_info. Uses the v2 pin/map path (with
195+
* GDR_PIN_FLAG_FORCE_PCIE) when libfabric_state->use_gdrcopy_v2 is set, else
196+
* the v1 path. Returns 0 on success, non-zero on failure (with the GDRCopy
197+
* return code surfaced via an INFO/ERROR log). On failure the caller is
198+
* responsible for freeing handle_info. */
199+
int libfabric_gdr_register_memhandle(nvshmemt_libfabric_state_t *libfabric_state,
200+
nvshmemt_libfabric_memhandle_info_t *handle_info, void *buf,
201+
size_t length) {
202+
int status = 0;
203+
gdr_info_t info;
204+
bool pinned = false;
205+
bool mapped = false;
206+
207+
if (libfabric_state->use_gdrcopy_v2) {
208+
/* Coherent platform path: force a BAR1/PCIe mapping so the
209+
* staged-atomics protocol's ordering assumptions hold. */
210+
status = gdrcopy_ftable.pin_buffer_v2(gdr_desc, (unsigned long)buf, length,
211+
GDR_PIN_FLAG_FORCE_PCIE, &handle_info->mh);
212+
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
213+
"gdrcopy pin_buffer_v2 failed \n");
214+
pinned = true;
215+
216+
status = gdrcopy_ftable.map_v2(gdr_desc, handle_info->mh, &handle_info->cpu_ptr_base,
217+
length, GDR_MAP_FLAG_DEFAULT);
218+
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out, "gdrcopy map_v2 failed \n");
219+
mapped = true;
220+
} else {
221+
status = gdrcopy_ftable.pin_buffer(gdr_desc, (unsigned long)buf, length, 0, 0,
222+
&handle_info->mh);
223+
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
224+
"gdrcopy pin_buffer failed \n");
225+
pinned = true;
226+
227+
status = gdrcopy_ftable.map(gdr_desc, handle_info->mh, &handle_info->cpu_ptr_base, length);
228+
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out, "gdrcopy map failed \n");
229+
mapped = true;
230+
}
231+
232+
status = gdrcopy_ftable.get_info(gdr_desc, handle_info->mh, &info);
233+
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out, "gdrcopy get_info failed \n");
234+
235+
/* Mappings start on a 64KB boundary, so calculate the offset from the
236+
* head of the mapping to the beginning of the buffer. */
237+
handle_info->cpu_ptr =
238+
(void *)((char *)handle_info->cpu_ptr_base + ((char *)buf - (char *)info.va));
239+
handle_info->gdr_mapping_size = length;
240+
handle_info->ptr = buf;
241+
return 0;
242+
243+
out:
244+
/* Best-effort cleanup of any partially-established GDRCopy state so the
245+
* pin does not leak. Unmap first (if mapped), then unpin (if pinned).
246+
* Note: gdr_unmap and gdr_unpin_buffer are the same APIs for both v1 and
247+
* v2 handles, so a single cleanup path covers both paths above.
248+
* Cleanup errors are logged but do not overwrite the primary status. */
249+
if (mapped) {
250+
int rc = gdrcopy_ftable.unmap(gdr_desc, handle_info->mh, handle_info->cpu_ptr_base,
251+
length);
252+
if (rc != 0) {
253+
INFO(libfabric_state->log_level,
254+
"gdrcopy unmap failed during error cleanup (rc=%d); primary status=%d", rc,
255+
status);
256+
}
257+
}
258+
if (pinned) {
259+
int rc = gdrcopy_ftable.unpin_buffer(gdr_desc, handle_info->mh);
260+
if (rc != 0) {
261+
INFO(libfabric_state->log_level,
262+
"gdrcopy unpin_buffer failed during error cleanup (rc=%d); primary status=%d",
263+
rc, status);
264+
}
265+
}
266+
return status;
267+
}
191268
#endif
192269

193270
struct nvshmemi_options_s options;
@@ -2237,9 +2314,6 @@ static int nvshmemt_libfabric_get_mem_handle(nvshmem_mem_handle_t *mem_handle, v
22372314
void *curr_ptr;
22382315
CUdevice gpu_device_id;
22392316
nvshmemt_libfabric_memhandle_info_t *handle_info = NULL;
2240-
#ifdef NVSHMEM_USE_GDRCOPY
2241-
gdr_info_t info;
2242-
#endif
22432317

22442318
// for now, error out if mmap is used with libfabric
22452319
// TODO : Add workaround for mmap with libfabric
@@ -2337,44 +2411,10 @@ static int nvshmemt_libfabric_get_mem_handle(nvshmem_mem_handle_t *mem_handle, v
23372411
if (!is_host) {
23382412
#ifdef NVSHMEM_USE_GDRCOPY
23392413
if (use_gdrcopy) {
2340-
if (libfabric_state->use_gdrcopy_v2) {
2341-
/* Coherent platform path: force a BAR1/PCIe mapping so the
2342-
* staged-atomics protocol's ordering assumptions hold. */
2343-
status = gdrcopy_ftable.pin_buffer_v2(gdr_desc, (unsigned long)buf, length,
2344-
GDR_PIN_FLAG_FORCE_PCIE,
2345-
&handle_info->mh);
2346-
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
2347-
"gdrcopy pin_buffer_v2 failed \n");
2348-
2349-
status = gdrcopy_ftable.map_v2(gdr_desc, handle_info->mh,
2350-
&handle_info->cpu_ptr_base, length,
2351-
GDR_MAP_FLAG_DEFAULT);
2352-
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
2353-
"gdrcopy map_v2 failed \n");
2354-
} else {
2355-
status = gdrcopy_ftable.pin_buffer(gdr_desc, (unsigned long)buf, length, 0, 0,
2356-
&handle_info->mh);
2357-
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
2358-
"gdrcopy pin_buffer failed \n");
2359-
2360-
status = gdrcopy_ftable.map(gdr_desc, handle_info->mh,
2361-
&handle_info->cpu_ptr_base, length);
2362-
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
2363-
"gdrcopy map failed \n");
2364-
}
2365-
2366-
status = gdrcopy_ftable.get_info(gdr_desc, handle_info->mh, &info);
2367-
NVSHMEMI_NZ_ERROR_JMP(status, NVSHMEMX_ERROR_INTERNAL, out,
2368-
"gdrcopy get_info failed \n");
2369-
2370-
// remember that mappings start on a 64KB boundary, so let's
2371-
// calculate the offset from the head of the mapping to the
2372-
// beginning of the buffer
2373-
handle_info->cpu_ptr =
2374-
(void *)((char *)handle_info->cpu_ptr_base + ((char *)buf - (char *)info.va));
2414+
status = libfabric_gdr_register_memhandle(libfabric_state, handle_info, buf,
2415+
length);
2416+
if (status != 0) goto out;
23752417

2376-
handle_info->gdr_mapping_size = length;
2377-
handle_info->ptr = buf;
23782418
curr_ptr = buf;
23792419
do {
23802420
status = nvshmemt_mem_handle_cache_add(t, libfabric_state->cache, curr_ptr,

0 commit comments

Comments
 (0)