Topic/topo numa distance part2 by ofirfarjun7 · Pull Request #8853 · openucx/ucx

ofirfarjun7 · 2023-02-07T11:23:00Z

What

Move numa latency calculations to UCP layer
Remove libnuma from UCX

Why ?

Remove UCX libnuma dependency

How ?

Move device memory latency calculation to UCP
Remove/Replace libnuma dependent code.
Remove libnuma from UCX

ofirfarjun7 · 2023-03-19T14:37:10Z

pls don't review until first part is merged.

config/m4/sysdep.m4

docs/source/faq.md

src/tools/info/sys_info.c

test/gtest/ucs/test_topo.cc

test/apps/uct_info/Makefile.in

src/tools/info/sys_info.c

gleon99 · 2023-03-22T12:56:01Z

Please look for numa / NUMA across the repo and cleanup where possible, seems there are several places.

ofirfarjun7 · 2023-04-08T10:40:41Z

In the FAQ we have "Important note" saying that (if I understand correctly) numactl is a dependency of rdma-core, and as a result also dependency of ucx-ib. Is it still true?
I've tried to run a docker without numactl installed to see if I can compile UCX but it complained that MLNX_OFED requires libnuma:
"Error: One or more required packages for installing MLNX_OFED_LINUX are missing.
Please install the missing packages using your Linux distribution Package Management tool.
Run:
yum install numactl-libs"
In numa.c and numa.h we have definitions of numa_policy, Do we still need it?
In ProtoV2 (proto_common.c) why we consider lane distance only for zcopy send?

src/ucp/proto/proto_common.c

src/uct/api/uct.h

src/tools/info/sys_info.c

yosefe · 2023-04-16T08:48:20Z

src/tools/info/sys_info.c

+    print_row_separator(distance_width, name_width, num_devices, ' ', '|');
+    print_row_separator(distance_width, name_width, num_devices, '-', '+');


can you give example of the output?

#8853 (comment)

can you pls fix units/dev name?
see #8853 (comment)

src/tools/info/tl_info.c

src/ucp/core/ucp_worker.c

gleon99 · 2023-04-13T13:09:15Z

src/tools/info/sys_info.c

-    unsigned num_devices            = ucs_topo_num_devices();
-    static const int distance_width = 10;
-    const char *distance_unit       = "MB/s";
+    unsigned num_devices                    = ucs_topo_num_devices();


src/ucp/core/ucp_worker.c

yosefe · 2023-04-16T14:23:28Z

src/tools/info/sys_info.c

+    print_row_separator(distance_width, name_width, num_devices, ' ', '|');
+    print_row_separator(distance_width, name_width, num_devices, '-', '+');


can you pls fix units/dev name?
see #8853 (comment)

src/ucs/sys/topo/base/topo.c

yosefe · 2023-04-17T07:27:27Z

src/ucp/core/ucp_worker.c

+ucp_worker_get_sys_device_memory_distance(ucp_worker_iface_t *wiface)
+{
+    ucs_sys_dev_distance_t *distance = &wiface->memory_distance;
+    ucs_sys_device_t sys_dev         = ucp_worker_get_sys_device(wiface);


maybe ucs_topo_get_memory_distance should return void?

We can take it even further, maybe we need to add a constrain to the topo providers API definition to have fallback behavior?
This way both get_distance and get_memory_distance will return void...
But maybe in another PR?

I can do it for the memory_distance for now and add it to the API description.

let's do it for memory distance for now

yosefe · 2023-04-17T07:29:03Z

src/ucp/core/ucp_worker.c

-            ucp_worker_iface_add_distance(&wiface->attr, &distance);
-        }
+        ucp_worker_iface_add_distance(&wiface->attr, &wiface->distance);
    }


why need to save memory_distance on the wiface?
seems it's used only during initialization

Will reduce the calls to ucs_topo_get_memory_distance in ucp_worker_iface_estimate_perf

but ucp_worker_iface_estimate_perf still calls UCT estimate perf
and ucs_topo_get_memory_distance should be quite fast since the NUMA distances are saved in a hash in ucs/topo

This is true. Will revert.

yosefe

@gleon99 can you pls take another look?

src/ucs/sys/topo/base/topo.h

src/ucp/core/ucp_worker.c

src/ucp/wireup/wireup.c

src/ucp/core/ucp_worker.c

gleon99

@ofirfarjun7 please squash.

ofirfarjun7 added the WIP-DNM Work in progress / Do not review label Feb 7, 2023

ofirfarjun7 force-pushed the topic/topo-numa-distance-part2 branch from b77fd85 to 42ad98c Compare February 21, 2023 18:33

ofirfarjun7 removed the WIP-DNM Work in progress / Do not review label Feb 21, 2023

ofirfarjun7 requested a review from gleon99 February 21, 2023 18:34

ofirfarjun7 force-pushed the topic/topo-numa-distance-part2 branch from 42ad98c to e118212 Compare February 21, 2023 18:37

ofirfarjun7 added the WIP-DNM Work in progress / Do not review label Mar 19, 2023

gleon99 reviewed Mar 22, 2023

View reviewed changes

ofirfarjun7 force-pushed the topic/topo-numa-distance-part2 branch 2 times, most recently from 8e84160 to b60fb6d Compare April 8, 2023 10:32

ofirfarjun7 removed the WIP-DNM Work in progress / Do not review label Apr 8, 2023

ofirfarjun7 force-pushed the topic/topo-numa-distance-part2 branch from b60fb6d to 5a6feea Compare April 9, 2023 13:41

ofirfarjun7 commented Apr 9, 2023

View reviewed changes

src/ucp/proto/proto_common.c Outdated Show resolved Hide resolved

ofirfarjun7 requested a review from gleon99 April 9, 2023 13:43

ofirfarjun7 added WIP-DNM Work in progress / Do not review and removed WIP-DNM Work in progress / Do not review labels Apr 9, 2023

ofirfarjun7 commented Apr 10, 2023

View reviewed changes

src/uct/api/uct.h Outdated Show resolved Hide resolved

yosefe reviewed Apr 16, 2023

View reviewed changes

gleon99 reviewed Apr 16, 2023

View reviewed changes

yosefe reviewed Apr 16, 2023

View reviewed changes

src/ucs/sys/topo/base/topo.c Show resolved Hide resolved

yosefe reviewed Apr 17, 2023

View reviewed changes

src/ucs/sys/topo/base/topo.h Outdated Show resolved Hide resolved

gleon99 reviewed Apr 18, 2023

View reviewed changes

yosefe previously approved these changes Apr 18, 2023

View reviewed changes

gleon99 previously approved these changes Apr 18, 2023

View reviewed changes

UCS/TOPO: Remove Numa dependency

91c1b25

ofirfarjun7 dismissed gleon99’s stale review via 91c1b25 April 18, 2023 12:04

ofirfarjun7 dismissed yosefe’s stale review via 91c1b25 April 18, 2023 12:04

ofirfarjun7 force-pushed the topic/topo-numa-distance-part2 branch from 08ba262 to 91c1b25 Compare April 18, 2023 12:04

yosefe approved these changes Apr 18, 2023

View reviewed changes

gleon99 enabled auto-merge April 18, 2023 12:15

gleon99 merged commit a793992 into openucx:master Apr 18, 2023

pentschev mentioned this pull request May 11, 2023

CUDA IPC performance regression after libnuma removal #9072

Closed

haampie mentioned this pull request Jun 19, 2023

Add numa variant in ucx package spack/spack#38423

Merged

This was referenced Mar 12, 2024

Drop unused libnl and libnuma & always build rdma & ibverbs support conda-forge/ucx-split-feedstock#160

Merged

Moving libnuma portions into a module #4570

Closed

		print_row_separator(distance_width, name_width, num_devices, ' ', '\|');
		print_row_separator(distance_width, name_width, num_devices, '-', '+');

Conversation

ofirfarjun7 commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why ?

How ?

Uh oh!

ofirfarjun7 commented Mar 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gleon99 commented Mar 22, 2023

Uh oh!

ofirfarjun7 commented Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gleon99 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ofirfarjun7 commented Feb 7, 2023 •

edited

Loading

ofirfarjun7 commented Mar 19, 2023 •

edited

Loading

ofirfarjun7 commented Apr 8, 2023 •

edited

Loading