core/hmem: introduce hmem_data field to memory registration #8787

wenduwan · 2023-04-11T19:49:10Z

Introduce a wildcard field hmem_data to provide an extension point for HMEM interfaces that require additional information to device id.

The immediate use case is to allow different providers to communicate the presence of a GDR handle. This is achieved with a companion flag, i.e. FI_HMEM_DATA_GDR_HANDLE.

Furthermore, this change makes it possible to offload similar operations to the user application, e.g. GDR pinning.

wenduwan · 2023-04-11T19:50:28Z

include/rdma/fabric.h

@@ -171,6 +171,7 @@ typedef struct fid *fid_t;
 #define FI_XPU_TRIGGER		(1ULL << 44)
 #define FI_HMEM_HOST_ALLOC	(1ULL << 45)
 #define FI_HMEM_DEVICE_ONLY	(1ULL << 46)
+#define FI_HMEM_DATA_GDR	(1ULL << 47)


Though this value is equal to FI_HMEM, it shouldn't be a problem. They are not expected to be used in the same way.

Probably makes sense to just model it as an alias for FI_HMEM in that case

Suggested change

#define FI_HMEM_DATA_GDR (1ULL << 47)

#define FI_HMEM_DATA_GDR FI_HMEM

Oh? IMO it is confusing since it is incidental that these 2 flags are equal.

Or maybe I should simply pick a different value to avoid this confusion.

Having the same value and being an alias are not the same. Flags for mr_attr should go into fi_domain.h. The FI_HMEM_HOST_ALLOC and FI_HMEM_DEVICE_ONLY flags should have gone there (and probably have been given different values). See comment in fi_domain.h on memory registration flags. I would not choose a value that conflicts with FI_HMEM. The possible need to use that flag is too high.

In general, flags for functions typically start at bit 59 and go down from there, skipping any conflicts (mostly with caps). So, I'd make this bit 59, since that's still available.

wzamazon · 2023-04-11T20:08:53Z

I think FI_HMEM_DATA_GDR is not a good name. GDR is a much bigger concept than gdrcopy.

I think the flag should be FI_HMEM_CUDA_GDRCOPY_HANDLE

wenduwan · 2023-04-11T20:13:18Z

I think FI_HMEM_DATA_GDR is not a good name. GDR is a much bigger concept than gdrcopy.

I think the flag should be FI_HMEM_CUDA_GDRCOPY_HANDLE

I agree. Renamed the flag to FI_HMEM_DATA_GDR_HANDLE which is shorter.

shefty · 2023-04-11T20:12:44Z

include/rdma/fabric.h

@@ -171,6 +171,7 @@ typedef struct fid *fid_t;
 #define FI_XPU_TRIGGER		(1ULL << 44)
 #define FI_HMEM_HOST_ALLOC	(1ULL << 45)
 #define FI_HMEM_DEVICE_ONLY	(1ULL << 46)
+#define FI_HMEM_DATA_GDR	(1ULL << 47)


Having the same value and being an alias are not the same. Flags for mr_attr should go into fi_domain.h. The FI_HMEM_HOST_ALLOC and FI_HMEM_DEVICE_ONLY flags should have gone there (and probably have been given different values). See comment in fi_domain.h on memory registration flags. I would not choose a value that conflicts with FI_HMEM. The possible need to use that flag is too high.

In general, flags for functions typically start at bit 59 and go down from there, skipping any conflicts (mostly with caps). So, I'd make this bit 59, since that's still available.

shefty · 2023-04-11T20:14:15Z

man/fi_mr.3.md

@@ -736,6 +743,10 @@ The follow flag may be specified to any memory registration call.
  API AllocHost call (as opposed to using malloc-like host allocation,
  unified/shared memory allocation, or AllocDevice).

+*FI_HMEM_DATA_GDR_HANDLE*
+: This flag indicates that hmem_data points to a GDR handle. The owner of the
+handle is responsible for unsetting this flag if the handle is invalidated.


Remove comment about 'owner of the handle'. This man page is user-facing, and that comment doesn't make sense.

I will remove this for now. My intention is to define the expectation if hmem_data is passed from the application, such that libfabric does not own the handle.

Having the same value and being an alias are not the same. Flags for mr_attr should go into fi_domain.h. The FI_HMEM_HOST_ALLOC and FI_HMEM_DEVICE_ONLY flags should have gone there (and probably have been given different values). See comment in fi_domain.h on memory registration flags. I would not choose a value that conflicts with FI_HMEM. The possible need to use that flag is too high.
In general, flags for functions typically start at bit 59 and go down from there, skipping any conflicts (mostly with caps). So, I'd make this bit 59, since that's still available.

I see. Thanks for the tip. I will include a commit to move the flags.

wzamazon · 2023-04-11T20:16:23Z

FI_HMEM_DATA_GDR_HANDLE which is shorter.

should be GDRCOPY_HANDLE. GDR is different from GDRcopy

wenduwan · 2023-04-11T20:34:28Z

FI_HMEM_DATA_GDR_HANDLE which is shorter.

should be GDRCOPY_HANDLE. GDR is different from GDRcopy

I see. Updated the name.

shefty · 2023-04-11T21:53:23Z

include/rdma/fi_domain.h

@@ -115,6 +115,11 @@ struct fid_av {
 * Tracks registered memory regions, primarily for remote access,
 * but also for local access until we can remove that need.
 */
+
+#define FI_HMEM_HOST_ALLOC		(1ULL << 60)
+#define FI_HMEM_DEVICE_ONLY		(1ULL << 61)


We can't change these flags. This breaks the API. I was only saying that they should have been given a different value, not that can be.

phew.. near miss. I will revert this part. Thanks!

But I doubt if this will actually break anything at this point. Did a quick code search, and the usage is strictly local, i.e. no exchange between endpoints. So I guess their values can be changed.... later.

These have been exposed to existing applications, which could be using them as input into a memory registration call. E.g. fabtests uses them. So, they can't change even later.

shefty · 2023-04-11T21:54:16Z

include/rdma/fi_domain.h

+
+#define FI_HMEM_HOST_ALLOC		(1ULL << 60)
+#define FI_HMEM_DEVICE_ONLY		(1ULL << 61)
+#define FI_HMEM_DATA_GDRCOPY_HANDLE	(1ULL << 62)


The values need to start at 59 and go down (i.e. 58, 57, etc.). The bit values 60-63 are reserved for provider specific use (or by libfabric for internal use).

Got it. I was using a different endianess.

shefty · 2023-04-11T21:55:30Z

man/fi_mr.3.md

+An optional pointer to data defined for the HMEM interface. This field is
+used alongside flags to provide additional data required for the HMEM ops
+if device identifier alone is not sufficient. This field is ignored unless
+the iface field is valid.


add: is valid and an appropriate call flag is set.

Makes sense. Reworded this section.

wenduwan · 2023-04-12T17:15:43Z

@shefty Could you confirm if continuous-integration/jenkins/pr-head failure is caused by this change?

aingerson · 2023-04-12T17:39:42Z

@wenduwan It doesn't look related. We had our cluster go down last week and this looks like lingering issues from that (lack of resources causing timeouts). The builds passed and a few tests passed and the others just couldn't get nodes so I think it's fine.

shefty · 2023-04-12T23:42:11Z

include/rdma/fi_domain.h

+
+#define FI_HMEM_HOST_ALLOC		(1ULL << 45)
+#define FI_HMEM_DEVICE_ONLY		(1ULL << 46)
+#define FI_HMEM_DATA_GDRCOPY_HANDLE	(1ULL << 59)


Please move these to line 298 (where there's a comment regarding mr flags)

GDRCOPY_HANDLE should probably be something like GDR_MAP_HANDLE based on the comments in gdrapi.h:

https://github.com/NVIDIA/gdrcopy/blob/master/include/gdrapi.h

shefty · 2023-04-12T23:46:12Z

man/fi_mr.3.md

@@ -736,6 +743,9 @@ The follow flag may be specified to any memory registration call.
  API AllocHost call (as opposed to using malloc-like host allocation,
  unified/shared memory allocation, or AllocDevice).

+*FI_HMEM_DATA_GDRCOPY_HANDLE*
+: This flag indicates that hmem_data points to a gdrcopy_handle struct.


struct gdrcopy_handle is something internal to libfabric. A user of the API won't have this. For the external API, this should be *gdr_mh_t. If additional information beyond gdr_mh_t is needed, then we will need to define a structure in the exported API files for callers to use. Such a structure cannot create a dependency between the libfabric build and cuda (or any other GPU library).

We could actually expose gdrcopy_handle for the application to do the memory pinning etc. and pass it to libfabric.

// src/hmem_cuda_gdrcopy.c struct gdrcopy_handle { gdr_mh_t mh; /* memory handler */ void *cuda_ptr; /* page aligned gpu pointer */ void *user_ptr; /* user space ptr mapped to GPU memory */ size_t length; /* page aligned length */ };

Because of gdrcopy's page alignment requirements, gdr_mh_t alone is not sufficient for address translation, so we do need to define a different struct for callers to use.

libfabric build should not have a hard dependency on gdrcopy, and we should skip gdrcopy if it is not available.

We cannot have gdr_mh_t in any of the libfabric external header files (include/rdma/*.h) without forcing a build dependency on the gdrcopy library, which is a no-go. The register mr APIs already have parameters to pass in an array of addresses and lengths. To register this memory for use with gdr copy, which of the ptrs do we actually need?

To register this memory for use with gdr copy, which of the ptrs do we actually need?

All of these fields are actually required to use gdrcopy. cuda_ptr and user_ptr are both needed to calculate offset induced by page alignment.

We cannot have gdr_mh_t in any of the libfabric external header files (include/rdma/*.h) without forcing a build dependency on the gdrcopy library, which is a no-go.

This makes sense. Practically gdr_mh_t is simply

typedef struct gdr_mh_s { unsigned long h; } gdr_mh_t;

But for libfabric it is an opaque value so probably not a good idea to replace it with unsigned long.

In this case, we probably shouldn't advertise struct gdrcopy_handle and keep it for internal use only as you suggested.

I can remove the mentioning of struct gdrcopy_handle from man page.

include/ofi_mr.h

shefty · 2023-04-14T02:17:51Z

man/fi_mr.3.md

@@ -736,6 +743,10 @@ The follow flag may be specified to any memory registration call.
  API AllocHost call (as opposed to using malloc-like host allocation,
  unified/shared memory allocation, or AllocDevice).

+*FI_HMEM_DATA_GDRCOPY_HANDLE*
+: This flag indicates that hmem_data points to a gdrcopy handle. This flag is
+  ignored unless iface is set to FI_HMEM_CUDA.


If this is defined and exposed to users, it needs to be well defined, so that it's usable. If we're only going to use this for some internal calls, then we can use one of the reserved internal flags, with some internal structure.

These are the relevant gdrcopy calls:

gdr_pin_buffer(gdr_t g, unsigned long addr, size_t size, uint64_t p2p_token, uint32_t va_space, gdr_mh_t *handle) gdr_map(gdr_t g, gdr_mh_t handle, void **va, size_t size); gdr_copy_to_mapping(gdr_mh_t handle, void *map_d_ptr, const void *h_ptr, size_t size)

This is the flow I anticipated:

gdr_mh_t gdr_mem_handle; void *gdr_mapped_addr; struct fi_mr_attr mr_attr; struct iovec iov; gdr_pin_buffer(g, addr, len, ?, ?, &gdr_mem_handle); gdr_map(g, gdr_mem_handle, &gdr_mapped_addr, len); /* Register the gdr mapped buffer */ iov.iov_base = gdr_mapped_addr; iov.iov_len = len; mr_attr.iov = &iov; mr_attr. ... mr_attr.hmem_data = gdr_mem_handle; fi_mr_regattr(..., &mr_attr, FI_HMEM_GDR_MAPPED);

The grpcopy routine needs the gdr_mem_handle and gdr_mapped_addr.

This makes sense if gdr pinning and address translation are delegated to the application, e.g. application ensures page alignment, and always passes page-aligned addresses to libfabric.

It's different in the current gdrcopy implementation. libfabric encapsulates this process from the application, and caches the alignment information in struct gdrcopy_handle, used for address translation during gdr_copy_from/to_mapping.

If we're only going to use this for some internal calls, then we can use one of the reserved internal flags, with some internal structure.

This is a good suggestion. In this case an internal flag & struct should be clear and sufficient. I will propose a change accordingly.

wenduwan · 2023-04-17T15:09:36Z

include/rdma/fi_domain.h

+#define FI_HMEM_DEVICE_ONLY		(1ULL << 46)
+
+/* libfabric internal flags starting from 60 to 63 */
+#define FI_HMEM_DATA_GDRCOPY_HANDLE	(1ULL << 60)


Hi @shefty I set this flag to 60 based on your prior comment of internal flags. Please let me know if you prefer a different value/name.

You are still changing the public header files. These changes need to be kept internal

Ack. Addressed in updated.

wenduwan · 2023-04-17T17:16:30Z

include/ofi_mr.h

@@ -264,13 +264,17 @@ int ofi_mr_map_verify(struct ofi_mr_map *map, uintptr_t *io_addr,
 * registration.
 */

+/* libfabric internal flags starting from 60 to 63 */
+#define FI_HMEM_DATA_GDRCOPY_HANDLE	(1ULL << 60)


To my understanding ofi_mr.h is more suited for this declaration than ofi_hmem.h, since this flag should be used next to struct ofi_mr. Please let me know otherwise.

Please rename with OFI prefix to indicate that this is an internal flag.

Ack. Will address.

Before that I would like to clarify the change to struct fi_mr_attr. Please see the other response.

wenduwan · 2023-04-17T22:43:42Z

I don't imagine the CI failure is related. Please let me know otherwise @shefty / @aingerson

shefty

forgot to submit review

shefty · 2023-04-17T17:19:49Z

include/ofi_mr.h

@@ -264,13 +264,17 @@ int ofi_mr_map_verify(struct ofi_mr_map *map, uintptr_t *io_addr,
 * registration.
 */

+/* libfabric internal flags starting from 60 to 63 */
+#define FI_HMEM_DATA_GDRCOPY_HANDLE	(1ULL << 60)


Please rename with OFI prefix to indicate that this is an internal flag.

shefty · 2023-04-17T17:20:32Z

man/fi_mr.3.md

+: For libfabric internal use only. This flag indicates that hmem_data points to
+  a gdrcopy_handle data structure. This flag is ignored unless iface is set to
+  FI_HMEM_CUDA.
+


All updates to this man page should be removed.

Thanks. Addressed comments. Stripped away unrelated changes as well since we are not exposing the new OFI_HMEM_* flag. This makes the change minimal.

The CI appears to be down recently. Assuming the failure is unrelated.

include/rdma/fi_domain.h

Introduce a wildcard field hmem_data to provide an extension point for HMEM interfaces that require additional information to device id. The immediate use case is to allow different providers to communicate the presence of a GDR handle. This is achieved with a companion flag, i.e. OFI_HMEM_DATA_GDRCOPY_HANDLE. Furthermore, this change makes it possible to offload similar operations to the user application, e.g. GDR pinning. Signed-off-by: Wenduo Wang <wenduwan@amazon.com>

shefty

These changes work, and if not, we should be safe to change the implementation later. Thanks!

shefty · 2023-04-18T18:02:28Z

Recent appveyor changes seem to be widening race windows, resulting in more frequent failures. Ignore for now, since the CI only tests the socket provider. I plan on changing that in the near future.

wenduwan · 2023-04-18T22:33:45Z

@wzamazon Do you have more comments on this PR?

wzamazon · 2023-04-18T22:41:08Z

"continuous-integration/appveyor/pr" failure is not related.

wenduwan requested review from shefty and a team April 11, 2023 19:49

wenduwan commented Apr 11, 2023

View reviewed changes

wenduwan mentioned this pull request Apr 11, 2023

core/hmem_cuda: expose cuda_is_device_id API #8746

Closed

wenduwan force-pushed the extend_mr branch from eea8ffe to 93fdb0a Compare April 11, 2023 20:12

shefty reviewed Apr 11, 2023

View reviewed changes

wenduwan force-pushed the extend_mr branch from 93fdb0a to c35ccf5 Compare April 11, 2023 20:34

wenduwan force-pushed the extend_mr branch 3 times, most recently from b2ba0f7 to c27e273 Compare April 11, 2023 20:38

shefty reviewed Apr 11, 2023

View reviewed changes

wenduwan force-pushed the extend_mr branch from c27e273 to 709ce13 Compare April 11, 2023 22:15

shijin-aws approved these changes Apr 12, 2023

View reviewed changes

wenduwan requested a review from a team April 12, 2023 17:24

wenduwan requested a review from shefty April 12, 2023 23:39

shefty reviewed Apr 12, 2023

View reviewed changes

wenduwan force-pushed the extend_mr branch from 709ce13 to a259f5a Compare April 13, 2023 16:46

wenduwan requested a review from shefty April 13, 2023 19:07

shefty reviewed Apr 14, 2023

View reviewed changes

wenduwan force-pushed the extend_mr branch from a259f5a to f700288 Compare April 17, 2023 15:08

wenduwan requested a review from shefty April 17, 2023 15:08

wenduwan commented Apr 17, 2023

View reviewed changes

wenduwan force-pushed the extend_mr branch from f700288 to 7867a89 Compare April 17, 2023 17:14

wenduwan commented Apr 17, 2023

View reviewed changes

shefty requested changes Apr 17, 2023

View reviewed changes

wenduwan force-pushed the extend_mr branch from 7867a89 to 0d3c9e0 Compare April 18, 2023 00:39

wenduwan force-pushed the extend_mr branch from 0d3c9e0 to b8dc277 Compare April 18, 2023 00:40

wenduwan requested a review from shefty April 18, 2023 00:41

shefty approved these changes Apr 18, 2023

View reviewed changes

wzamazon approved these changes Apr 18, 2023

View reviewed changes

wzamazon merged commit 49d1a76 into ofiwg:main Apr 18, 2023

wenduwan deleted the extend_mr branch April 18, 2023 23:32

wenduwan mentioned this pull request May 8, 2023

Proposal: SHM cuda-to-cuda performance improvement #8745

Closed

	#define FI_HMEM_DATA_GDR (1ULL << 47)
	#define FI_HMEM_DATA_GDR FI_HMEM

core/hmem: introduce hmem_data field to memory registration #8787

core/hmem: introduce hmem_data field to memory registration #8787

Conversation

wenduwan commented Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzamazon commented Apr 11, 2023

wenduwan commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzamazon commented Apr 11, 2023

wenduwan commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenduwan commented Apr 12, 2023

aingerson commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenduwan commented Apr 17, 2023

shefty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shefty left a comment

Choose a reason for hiding this comment

shefty commented Apr 18, 2023

wenduwan commented Apr 18, 2023

wzamazon commented Apr 18, 2023

wenduwan commented Apr 11, 2023 •

edited

Loading