Skip to content

Conversation

@Gonzalosilvalde
Copy link

Summary

This Merge Request enhances libfabric’s portability on 32-bit systems by:

  • Replacing hard-coded format specifiers (%lx, %zx, etc.) with the corresponding <inttypes.h> macros (PRIuPTR, PRIxPTR, PRIx64, PRIu64, etc.).
  • Adjusting pointer and integer casts to use uintptr_t instead of fixed-width types (e.g. uint64_t) for address arithmetic.
  • Introducing size_t temporaries when consuming I/O vectors to ensure correct sizing on all architectures.
  • Format specifier correction: Fixed error messages that used %lu for size_t, replacing them with %zu.

Types of changes

- "buf %p len %zu addr %zu data %lu " \
- "flags 0x%zx ctx %p\n", \
- buf, len, addr, (uint64_t)data, \
- (uint64_t)flags, context); \
+ "buf %p len %zu addr %" PRIuPTR " data 0x%" PRIx64 " " \
+ "flags 0x%" PRIxPTR " ctx %p\n", \
+ (void *)(buf), (size_t)(len), (uintptr_t)(addr), (uint64_t)(data), \
+ (uintptr_t)flags, (void *)(context)); \
- ofi_hmem_dev_unregister(attr.iface, (uint64_t)attr.hmem_data);
+ ofi_hmem_dev_unregister(attr.iface, (uintptr_t)attr.hmem_data);

References

Comment on lines 67 to 74
size_t local_size = local_cnt;
size_t remote_size = remote_cnt;

ofi_consume_iov(local, &local_size, (size_t)ret);
ofi_consume_iov(remote, &remote_size, (size_t)ret);

local_cnt = local_size;
remote_cnt = remote_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should change the type of local_cnt and remote_cnt instead.

@aingerson The type of the parameters passed in for this call is actually size_t.

FI_REMOTE_READ | FI_REMOTE_WRITE;
attr.offset = 0;
attr.requested_key = (uint64_t) *hmem_desc;
attr.requested_key = (uintptr_t) *hmem_desc;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we are indeed expecting uint64_t. The conversion works for both 32-bit and 64-bit machines is (uint64_t)(uintptr_t) *hmem_desc

rc = ofi_mr_cache_search(&lnx_dom->ld_mr_cache, &info, mre);
if (rc) {
ofi_hmem_dev_unregister(attr.iface, (uint64_t)attr.hmem_data);
ofi_hmem_dev_unregister(attr.iface, (uintptr_t)attr.hmem_data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, use (uint64_t)(uintptr_t).

struct ofi_mr *mr = (struct ofi_mr *)entry->data;

ofi_hmem_dev_unregister(mr->iface, (uint64_t) mr->hmem_data);
ofi_hmem_dev_unregister(mr->iface, (uintptr_t) mr->hmem_data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

memcpy(mr_map, map->raw_key, map->key_size);

*(map->key) = (uint64_t)mr_map;
*(map->key) = (uintptr_t)mr_map;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Comment on lines 738 to 749
vrb_gl_data.dmabuf_support ? "enabled" : "disabled");
vrb_gl_data.dmabuf_support ? "enabled" : "disabled");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Comment on lines 741 to 754
if (vrb_get_param_str("iface", "The prefix or the full name of the "
"network interface associated with the verbs "
"device", &vrb_gl_data.iface)) {
if (vrb_get_param_str("iface",
"Network interface prefix or full name",
&vrb_gl_data.iface)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Comment on lines 751 to 765
if (vrb_get_param_bool("dgram_use_name_server", "The option that "
"enables/disables OFI Name Server thread used "
"to resolve IP-addresses to provider specific "
"addresses. If MPI is used, the NS is disabled "
"by default.", &vrb_gl_data.dgram.use_name_server)) {

if (vrb_read_param_bool("dgram_use_name_server",
"Enable/disable OFI Name Server thread",
&vrb_gl_data.dgram.use_name_server)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Comment on lines 759 to 774
if (vrb_get_param_int("dgram_name_server_port", "The port on which "
"the name server thread listens incoming "
"requests.", &vrb_gl_data.dgram.name_server_port) ||
(vrb_gl_data.dgram.name_server_port < 0 ||
vrb_gl_data.dgram.name_server_port > 65535)) {

if (vrb_get_param_int("dgram_name_server_port",
"Port for name server thread",
&vrb_gl_data.dgram.name_server_port) ||
vrb_gl_data.dgram.name_server_port < 0 ||
vrb_gl_data.dgram.name_server_port > 65535) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

Comment on lines 87 to 89
mr = ibv_reg_dmabuf_mr(pd, offset, len, (uint64_t)buf/* iova */,
fd, vrb_access);
{
uintptr_t buf_addr = (uintptr_t)buf;
uint64_t iova = (uint64_t)buf_addr;
mr = ibv_reg_dmabuf_mr(pd, offset, len, iova,
fd, vrb_access);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can simply cast buf to (uint64_t)(uintptr_t)

@j-xiong
Copy link
Contributor

j-xiong commented May 15, 2025

Also please sign off your commit.

Copy link
Contributor

@j-xiong j-xiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good except for a few formatting issues.

For the DCO error: Commit sha: c9b9374, Author: Gonzalosilvalde, Committer: Gonzalosilvalde; Expected "Gonzalosilvalde gonzalo.silvalde@udc.es", but got "Gonzalo Silvalde gonzalo.silvalde@gmail.com". It seems that the machine you used to create the commit has git email configured differently from the one used in "Signed-off-by".

void *res_buf = resp_hdr->data + offset;
void *dst_buf = (void *) req_hdr->rma_ioc[i].addr;

void *dst_buf = (void *)(uintptr_t) req_hdr->rma_ioc[i].addr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the empty line here.

mr = ibv_reg_dmabuf_mr(pd, offset, len, (uint64_t)buf/* iova */,
fd, vrb_access);
mr = ibv_reg_dmabuf_mr(pd, offset, len, (uint64_t)(uintptr_t)buf/* iova */,
fd, vrb_access);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alignment of L88 is off.

@j-xiong
Copy link
Contributor

j-xiong commented May 29, 2025

Any changed files under prov/sm2 need to be run through clang-format to pass the format check. That is for the sm provider only.

@Gonzalosilvalde
Copy link
Author

Sorry for the delay in making these small updates, I was on vacation without a computer handy. I see there are some merge conflicts, I’m looking into exactly what’s causing them now and will resolve the issues shortly. Apologies for the inconvenience, and thanks for your patience.

Signed-off-by: Gonzalo Silvalde <gonzalo.silvalde@gmail.com>
@Gonzalosilvalde
Copy link
Author

Good afternoon, @j-xiong . After a couple of months, I've come back to this problem, but it's still failing. I thought that the failures related to Jenkins and AWS CI had to do with fabtest not being adapted to 32 bits, so I made some changes, but I see that this isn't the problem and the errors persist. I don't know if you could give me some guidance on what might be failing. Best regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants