Skip to content

Update L0 provider to use getpidfd function #558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 1, 2024
Merged

Update L0 provider to use getpidfd function #558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ccfe39
Add multiprocess test for L0 IPC
vinser52 Jun 19, 2024
a0850d8
Update L0 provider to use getpidfd function to open IPC handle from r…
vinser52 Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions src/provider/provider_level_zero.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,11 +285,16 @@ static umf_result_t ze_memory_provider_allocation_split(void *provider,
return UMF_RESULT_ERROR_NOT_SUPPORTED;
}

typedef struct ze_ipc_data_t {
int pid;
ze_ipc_mem_handle_t ze_handle;
} ze_ipc_data_t;

static umf_result_t ze_memory_provider_get_ipc_handle_size(void *provider,
size_t *size) {
(void)provider;
ASSERT(size != NULL);
*size = sizeof(ze_ipc_mem_handle_t);
*size = sizeof(ze_ipc_data_t);
return UMF_RESULT_SUCCESS;
}

Expand All @@ -301,17 +306,19 @@ static umf_result_t ze_memory_provider_get_ipc_handle(void *provider,
ASSERT(providerIpcData != NULL);
(void)size;
ze_result_t ze_result;
ze_ipc_mem_handle_t *ze_ipc_handle = (ze_ipc_mem_handle_t *)providerIpcData;
ze_ipc_data_t *ze_ipc_data = (ze_ipc_data_t *)providerIpcData;
struct ze_memory_provider_t *ze_provider =
(struct ze_memory_provider_t *)provider;

ze_result =
g_ze_ops.zeMemGetIpcHandle(ze_provider->context, ptr, ze_ipc_handle);
ze_result = g_ze_ops.zeMemGetIpcHandle(ze_provider->context, ptr,
&ze_ipc_data->ze_handle);
if (ze_result != ZE_RESULT_SUCCESS) {
LOG_ERR("zeMemGetIpcHandle() failed.");
return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
}

ze_ipc_data->pid = utils_getpid();

return UMF_RESULT_SUCCESS;
}

Expand All @@ -322,7 +329,7 @@ static umf_result_t ze_memory_provider_put_ipc_handle(void *provider,
ze_result_t ze_result;
struct ze_memory_provider_t *ze_provider =
(struct ze_memory_provider_t *)provider;
ze_ipc_mem_handle_t *ze_ipc_handle = (ze_ipc_mem_handle_t *)providerIpcData;
ze_ipc_data_t *ze_ipc_data = (ze_ipc_data_t *)providerIpcData;

if (g_ze_ops.zeMemPutIpcHandle == NULL) {
// g_ze_ops.zeMemPutIpcHandle can be NULL because it was introduced
Expand All @@ -331,8 +338,8 @@ static umf_result_t ze_memory_provider_put_ipc_handle(void *provider,
return UMF_RESULT_SUCCESS;
}

ze_result =
g_ze_ops.zeMemPutIpcHandle(ze_provider->context, *ze_ipc_handle);
ze_result = g_ze_ops.zeMemPutIpcHandle(ze_provider->context,
ze_ipc_data->ze_handle);
if (ze_result != ZE_RESULT_SUCCESS) {
LOG_ERR("zeMemPutIpcHandle() failed.");
return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
Expand All @@ -347,12 +354,29 @@ static umf_result_t ze_memory_provider_open_ipc_handle(void *provider,
ASSERT(providerIpcData != NULL);
ASSERT(ptr != NULL);
ze_result_t ze_result;
ze_ipc_mem_handle_t *ze_ipc_handle = (ze_ipc_mem_handle_t *)providerIpcData;
ze_ipc_data_t *ze_ipc_data = (ze_ipc_data_t *)providerIpcData;
struct ze_memory_provider_t *ze_provider =
(struct ze_memory_provider_t *)provider;
int fd_local = -1;
ze_ipc_mem_handle_t ze_ipc_handle = ze_ipc_data->ze_handle;

if (ze_ipc_data->pid != utils_getpid()) {
int fd_remote = -1;
memcpy(&fd_remote, &ze_ipc_handle, sizeof(fd_remote));
Copy link
Contributor

Choose a reason for hiding this comment

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

Using memcpy to copy single int is an overkill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compiler optimizes it. When the size is low enough there is now an actual call to memcpy, it is replaced with move instructions.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more code readability issue then performance. Assignment is shorter, and easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

regarding readability, I am not sure:

fd_remote = *(int*)&ze_ipc_handle;

vs

memcpy(&fd_remote, &ze_ipc_handle, sizeof(fd_remote));

and

*((int*)&ze_ipc_handle) = fd_local;

vs

memcpy(&ze_ipc_handle, &fd_local, sizeof(fd_local));

Are you still suggesting to get rid of memcpy? If so, I will do that. Please confirm.

Copy link
Contributor

@lplewa lplewa Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This IPC api in L0 is awful....

I checked definition of ze_ipc_mem_handle_t and as this is opaque struct, memcpy here make sense here.

NIT: We should consider something like this - as working on opaque structs without clear definition of it's layout is quite error prone.

union umf_ipc_mem_handle {
    ze_ipc_mem_handle_t ze;
    struct {
        int fd;
    } def;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all this is needed just because L0 IPC implementation is broken.
According to the L0 spec all these steps not needed at all. When L0 driver fix their implementation we will get rid of these changes.

umf_result_t umf_result =
utils_duplicate_fd(ze_ipc_data->pid, fd_remote, &fd_local);
if (umf_result != UMF_RESULT_SUCCESS) {
LOG_PERR("duplicating file descriptor failed");
return umf_result;
}
memcpy(&ze_ipc_handle, &fd_local, sizeof(fd_local));
}

ze_result = g_ze_ops.zeMemOpenIpcHandle(
ze_provider->context, ze_provider->device, *ze_ipc_handle, 0, ptr);
ze_provider->context, ze_provider->device, ze_ipc_handle, 0, ptr);
if (fd_local != -1) {
(void)utils_close_fd(fd_local);
}
if (ze_result != ZE_RESULT_SUCCESS) {
LOG_ERR("zeMemOpenIpcHandle() failed.");
return UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
Expand Down
6 changes: 3 additions & 3 deletions src/provider/provider_os_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ create_fd_for_mmap(umf_os_memory_provider_params_t *in_params,

err_close_file:
if (provider->fd > 0) {
(void)os_close_fd(provider->fd);
(void)utils_close_fd(provider->fd);
}

return result;
Expand Down Expand Up @@ -1247,7 +1247,7 @@ static umf_result_t os_open_ipc_handle(void *provider, void *providerIpcData,
(void)os_shm_unlink(os_provider->shm_name);
} else {
umf_result_t umf_result =
os_duplicate_fd(os_ipc_data->pid, os_ipc_data->fd, &fd);
utils_duplicate_fd(os_ipc_data->pid, os_ipc_data->fd, &fd);
if (umf_result != UMF_RESULT_SUCCESS) {
LOG_PERR("duplicating file descriptor failed");
return umf_result;
Expand All @@ -1262,7 +1262,7 @@ static umf_result_t os_open_ipc_handle(void *provider, void *providerIpcData,
ret = UMF_RESULT_ERROR_MEMORY_PROVIDER_SPECIFIC;
}

(void)os_close_fd(fd);
(void)utils_close_fd(fd);

return ret;
}
Expand Down
4 changes: 0 additions & 4 deletions src/provider/provider_os_memory_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,6 @@ size_t os_get_page_size(void);

void os_strerror(int errnum, char *buf, size_t buflen);

umf_result_t os_duplicate_fd(int pid, int fd_in, int *fd_out);

umf_result_t os_close_fd(int fd);

#ifdef __cplusplus
}
#endif
Expand Down
44 changes: 0 additions & 44 deletions src/provider/provider_os_memory_posix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,47 +89,3 @@ int os_purge(void *addr, size_t length, int advice) {
void os_strerror(int errnum, char *buf, size_t buflen) {
strerror_r(errnum, buf, buflen);
}

umf_result_t os_duplicate_fd(int pid, int fd_in, int *fd_out) {
// pidfd_getfd(2) is used to obtain a duplicate of another process's file descriptor.
// Permission to duplicate another process's file descriptor
// is governed by a ptrace access mode PTRACE_MODE_ATTACH_REALCREDS check (see ptrace(2))
// that can be changed using the /proc/sys/kernel/yama/ptrace_scope interface.
// pidfd_getfd(2) is supported since Linux 5.6
// pidfd_open(2) is supported since Linux 5.3
#if defined(__NR_pidfd_open) && defined(__NR_pidfd_getfd)
errno = 0;
int pid_fd = syscall(SYS_pidfd_open, pid, 0);
if (pid_fd == -1) {
LOG_PDEBUG("SYS_pidfd_open");
return UMF_RESULT_ERROR_UNKNOWN;
}

int fd_dup = syscall(SYS_pidfd_getfd, pid_fd, fd_in, 0);
close(pid_fd);
if (fd_dup == -1) {
LOG_PDEBUG("SYS_pidfd_getfd");
return UMF_RESULT_ERROR_UNKNOWN;
}

*fd_out = fd_dup;

return UMF_RESULT_SUCCESS;
#else
// TODO: find another way to obtain a duplicate of another process's file descriptor
(void)pid; // unused
(void)fd_in; // unused
(void)fd_out; // unused
errno = ENOTSUP;
return UMF_RESULT_ERROR_NOT_SUPPORTED; // unsupported
#endif /* defined(__NR_pidfd_open) && defined(__NR_pidfd_getfd) */
}

umf_result_t os_close_fd(int fd) {
if (close(fd)) {
LOG_PERR("close() failed");
return UMF_RESULT_ERROR_UNKNOWN;
}

return UMF_RESULT_SUCCESS;
}
12 changes: 0 additions & 12 deletions src/provider/provider_os_memory_windows.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,15 +151,3 @@ size_t os_get_page_size(void) {
void os_strerror(int errnum, char *buf, size_t buflen) {
strerror_s(buf, buflen, errnum);
}

umf_result_t os_duplicate_fd(int pid, int fd_in, int *fd_out) {
(void)pid; // unused
(void)fd_in; // unused
(void)fd_out; // unused
return UMF_RESULT_ERROR_NOT_SUPPORTED; // unsupported
}

umf_result_t os_close_fd(int fd) {
(void)fd; // unused
return UMF_RESULT_ERROR_NOT_SUPPORTED; // unsupported
}
8 changes: 8 additions & 0 deletions src/utils/utils_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
#include <stddef.h>
#include <stdint.h>

#include <umf/base.h>

#ifdef __cplusplus
extern "C" {
#endif
Expand Down Expand Up @@ -77,6 +79,12 @@ int utils_getpid(void);
// get the current thread ID
int utils_gettid(void);

// close file descriptor
int utils_close_fd(int fd);

// obtain a duplicate of another process's file descriptor
umf_result_t utils_duplicate_fd(int pid, int fd_in, int *fd_out);

#ifdef __cplusplus
}
#endif
Expand Down
66 changes: 66 additions & 0 deletions src/utils/utils_posix_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,22 @@
*
*/

#include <errno.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <unistd.h>

#include "utils_common.h"
#include "utils_concurrency.h"
#include "utils_log.h"

#ifndef __NR_pidfd_open
#define __NR_pidfd_open 434 /* Syscall id */
#endif
#ifndef __NR_pidfd_getfd
#define __NR_pidfd_getfd 438 /* Syscall id */
#endif

static UTIL_ONCE_FLAG Page_size_is_initialized = UTIL_ONCE_FLAG_INIT;
static size_t Page_size;
Expand All @@ -38,3 +48,59 @@ int utils_gettid(void) {
return syscall(SYS_gettid);
#endif
}

int utils_close_fd(int fd) { return close(fd); }

#ifndef __APPLE__
static umf_result_t errno_to_umf_result(int err) {
switch (err) {
case EBADF:
case EINVAL:
case ESRCH:
case EPERM:
return UMF_RESULT_ERROR_INVALID_ARGUMENT;
case EMFILE:
case ENOMEM:
return UMF_RESULT_ERROR_OUT_OF_HOST_MEMORY;
case ENODEV:
case ENOSYS:
case ENOTSUP:
return UMF_RESULT_ERROR_NOT_SUPPORTED;
default:
return UMF_RESULT_ERROR_UNKNOWN;
}
}
#endif

umf_result_t utils_duplicate_fd(int pid, int fd_in, int *fd_out) {
#ifdef __APPLE__
(void)pid; // unused
(void)fd_in; // unused
(void)fd_out; // unused
return UMF_RESULT_ERROR_NOT_SUPPORTED;
#else
// pidfd_getfd(2) is used to obtain a duplicate of another process's file descriptor.
// Permission to duplicate another process's file descriptor
// is governed by a ptrace access mode PTRACE_MODE_ATTACH_REALCREDS check (see ptrace(2))
// that can be changed using the /proc/sys/kernel/yama/ptrace_scope interface.
// pidfd_getfd(2) is supported since Linux 5.6
// pidfd_open(2) is supported since Linux 5.3
errno = 0;
int pid_fd = syscall(__NR_pidfd_open, pid, 0);
if (pid_fd == -1) {
LOG_PERR("__NR_pidfd_open");
return errno_to_umf_result(errno);
}

int fd_dup = syscall(__NR_pidfd_getfd, pid_fd, fd_in, 0);
close(pid_fd);
if (fd_dup == -1) {
LOG_PERR("__NR_pidfd_open");
return errno_to_umf_result(errno);
}

*fd_out = fd_dup;

return UMF_RESULT_SUCCESS;
#endif
}
13 changes: 13 additions & 0 deletions src/utils/utils_windows_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <processenv.h>
#include <processthreadsapi.h>

#include "utils_common.h"
#include "utils_concurrency.h"

#define BUFFER_SIZE 1024
Expand All @@ -33,3 +34,15 @@ size_t util_get_page_size(void) {
int utils_getpid(void) { return GetCurrentProcessId(); }

int utils_gettid(void) { return GetCurrentThreadId(); }

int utils_close_fd(int fd) {
(void)fd; // unused
return -1;
}

umf_result_t utils_duplicate_fd(int pid, int fd_in, int *fd_out) {
(void)pid; // unused
(void)fd_in; // unused
(void)fd_out; // unused
return UMF_RESULT_ERROR_NOT_SUPPORTED;
}
33 changes: 31 additions & 2 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,14 +219,14 @@ if(UMF_BUILD_GPU_TESTS AND UMF_BUILD_LEVEL_ZERO_PROVIDER)
# dlopen)
add_umf_test(
NAME provider_level_zero
SRCS providers/provider_level_zero.cpp
SRCS providers/provider_level_zero.cpp providers/level_zero_helpers.cpp
LIBS ${UMF_UTILS_FOR_TEST} ze_loader)
target_include_directories(umf_test-provider_level_zero
PRIVATE ${LEVEL_ZERO_INCLUDE_DIRS})

add_umf_test(
NAME provider_level_zero_dlopen
SRCS providers/provider_level_zero.cpp
SRCS providers/provider_level_zero.cpp providers/level_zero_helpers.cpp
LIBS ${UMF_UTILS_FOR_TEST})
target_compile_definitions(umf_test-provider_level_zero_dlopen
PUBLIC USE_DLOPEN=1)
Expand Down Expand Up @@ -322,6 +322,35 @@ if(LINUX)
common/ipc_os_prov_common.c)
add_umf_ipc_test(TEST ipc_os_prov_anon_fd)
add_umf_ipc_test(TEST ipc_os_prov_shm)
if(UMF_BUILD_GPU_TESTS AND UMF_BUILD_LEVEL_ZERO_PROVIDER)
build_umf_test(
NAME
ipc_level_zero_prov_consumer
SRCS
providers/ipc_level_zero_prov_consumer.c
common/ipc_common.c
providers/ipc_level_zero_prov_common.c
providers/level_zero_helpers.cpp
LIBS
ze_loader
${UMF_UTILS_FOR_TEST})
build_umf_test(
NAME
ipc_level_zero_prov_producer
SRCS
providers/ipc_level_zero_prov_producer.c
common/ipc_common.c
providers/ipc_level_zero_prov_common.c
providers/level_zero_helpers.cpp
LIBS
ze_loader
${UMF_UTILS_FOR_TEST})
target_include_directories(umf_test-ipc_level_zero_prov_producer
PRIVATE ${LEVEL_ZERO_INCLUDE_DIRS})
target_include_directories(umf_test-ipc_level_zero_prov_consumer
PRIVATE ${LEVEL_ZERO_INCLUDE_DIRS})
add_umf_ipc_test(TEST ipc_level_zero_prov SRC_DIR providers)
endif()
else()
message(STATUS "IPC tests are supported on Linux only - skipping")
endif()
Expand Down
Loading