Skip to content
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

Make util_ns fields: run and is_initialized atomics #10570

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −13
Open

Make util_ns fields: run and is_initialized atomics #10570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions include/ofi_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1243,8 +1243,8 @@ struct util_ns {
size_t name_len;
size_t service_len;

int run;
int is_initialized;
ofi_atomic32_t run;
ofi_atomic32_t is_initialized;
ofi_atomic32_t ref;

ofi_ns_service_cmp_func_t service_cmp;
Expand Down
22 changes: 11 additions & 11 deletions prov/util/src/util_ns.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ static void *util_ns_accept_handler(void *args)
struct util_ns_cmd cmd;
int ret;

while (ns->run) {
while (ofi_atomic_get32(&ns->run)) {
conn_sock = accept(ns->listen_sock, NULL, 0);
if (conn_sock == INVALID_SOCKET)
break;
Expand Down Expand Up @@ -367,7 +367,7 @@ int ofi_ns_add_local_name(struct util_ns *ns, void *service, void *name)
.op = OFI_UTIL_NS_ADD,
};

if (!ns->is_initialized) {
if (!ofi_atomic_get32(&ns->is_initialized)) {
FI_WARN(&core_prov, FI_LOG_CORE,
"Cannot add local name - name server uninitialized\n");
return -FI_EINVAL;
Expand Down Expand Up @@ -415,7 +415,7 @@ int ofi_ns_del_local_name(struct util_ns *ns, void *service, void *name)
.op = OFI_UTIL_NS_DEL
};

if (!ns->is_initialized)
if (!ofi_atomic_get32(&ns->is_initialized))
return -FI_EINVAL;

write_buf = calloc(cmd_len + ns->service_len + ns->name_len, 1);
Expand Down Expand Up @@ -460,7 +460,7 @@ void *ofi_ns_resolve_name(struct util_ns *ns, const char *server, void *service)
.op = OFI_UTIL_NS_QUERY
};

if (!ns->is_initialized)
if (!ofi_atomic_get32(&ns->is_initialized))
return NULL;

sockfd = util_ns_connect_server(ns, server);
Expand Down Expand Up @@ -519,7 +519,7 @@ int ofi_ns_start_server(struct util_ns *ns)
{
int ret;

assert(ns->is_initialized);
assert(ofi_atomic_get32(&ns->is_initialized));
if (ofi_atomic_inc32(&ns->ref) > 1)
return 0;

Expand Down Expand Up @@ -547,7 +547,7 @@ int ofi_ns_start_server(struct util_ns *ns)
goto err2;
}

ns->run = 1;
ofi_atomic_set32(&ns->run, 1);
ret = -pthread_create(&ns->thread, NULL,
util_ns_accept_handler, (void *) ns);
if (ret)
Expand All @@ -556,7 +556,7 @@ int ofi_ns_start_server(struct util_ns *ns)
return 0;

err3:
ns->run = 0;
ofi_atomic_set32(&ns->run, 0);
util_ns_close_listen(ns);
err2:
rbtDelete(ns->map);
Expand All @@ -570,15 +570,15 @@ void ofi_ns_stop_server(struct util_ns *ns)
{
SOCKET sock;

assert(ns->is_initialized);
assert(ofi_atomic_get32(&ns->is_initialized));

if (ofi_atomic_dec32(&ns->ref))
return;

if (ns->listen_sock == INVALID_SOCKET)
return;

ns->run = 0;
ofi_atomic_set32(&ns->run, 0);
sock = util_ns_connect_server(ns, ns->hostname);
if (sock != INVALID_SOCKET)
ofi_close_socket(sock);
Expand All @@ -592,12 +592,12 @@ void ofi_ns_init(struct util_ns *ns)
{
assert(ns && ns->name_len && ns->service_len && ns->service_cmp);

if (ns->is_initialized)
if (ofi_atomic_get32(&ns->is_initialized))
return;

ofi_atomic_initialize32(&ns->ref, 0);
ns->listen_sock = INVALID_SOCKET;
if (!ns->hostname)
ns->hostname = OFI_NS_DEFAULT_HOSTNAME;
ns->is_initialized = 1;
ofi_atomic_set32(&ns->is_initialized, 1);
Copy link
Member

Choose a reason for hiding this comment

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

The name service is basically a simple way to support running fabtests. It's not something we expect to have enabled as a general purpose feature.

If it were, the changes here are not sufficient to support multiple threads. Locks would be needed. It's ultimately the responsibility of the caller to have appropriate serialization to the name service calls.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip about the name service. I found that it is by default enabled for verbs provider, which, in case of a multithreaded application, leads to data races triggered by e.g. parallel fi_close and util_ns_accept_handler execution. Don't you think, that by as you said: "not expecting this option to be enabled as a general purpose feature" you should default this option to OFF?

Copy link
Member

Choose a reason for hiding this comment

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

Can you describe what sort of endpoints the app is using? It looks like the name service is related to dgram ep's, and only started if a verbs domain is opened for dgram ep's. Note that this can occur through the rxd provider for rdm ep's.

@j-xiong - I see that the name server is enabled by default, but disabled if specific, unrelated environment variables are set (OMPI_COMM_WORLD_RANK or PMI_RANK). Should the default be to disable it, and use an environment variable to enable instead?

I'm unsure how frequently the verbs dgram provider is used in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shefty The name server is needed for verbs dgram to resolve "node" into dst_addr in fi_getinfo(). By default this is a feature an application may use. MPIs (OpenMPI, MPICH, Intel MPI) don't use this feature so the name server can be safely disabled.

Copy link
Author

@dsciebu dsciebu Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My experiments that revealed the issue were made on a Mellanox RDMA card. Using that hardware and setting up the hints the way that rdm fabtests do, I ended up in getting an RXD protocol 'based' endpoint.
When checking my libfabric based app's test, I noticed that Thread Sanitizer reports data race in libfabric and ended up here. Turning off the 'name service' with the proper env variable solves the problem completely.
So, IMO it should be:
a) turned off by default if not necessary or
b) made thread safe.

Copy link
Member

Choose a reason for hiding this comment

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

@dsciebu - I agree. Can you open an issue to track this? The name service should be made thread safe, which I'm guessing won't be that difficult. Likely just needs a lock around most of the functions you identified.

}