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.

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

Support for shared memory provider of libfabric in SST + simple support for manual data progress via threading #3964

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Dec 12, 2023

Background: For some of our intended SST workflows, we exchange data only within nodes. Using the system networks is an unnecessary detour in that case. Since libfabric implements shared memory, this is a somewhat low-hanging fruit to enable truly in-memory staging workflows with SST.

Necessary changes:

  • The most important change is that the shm provider requires manual data progress. This is hence also a follow-up to Adapt libfabric dataplane of SST to Cray CXI provider #3672: The CXI provider supported by that PR also technically requires manual data progress, but effectively works fine without it.
  • FI_MR_BASIC registration mode prints an error, but interestingly it still works anyway. This PR still replaces FI_MR_BASIC with the equivalent FI_MR_VIRT_ADDR | FI_MR_ALLOCATED | FI_MR_PROV_KEY | FI_MR_LOCAL.
  • Some subtleties in address handling

The manual data progress has turned out to be somewhat annoying. My idea was to spawn a thread that regularly pokes the provider, but this approach does not work well with any less than a busy loop.
Every call to fi_read() by the Reader requires an accompanying call to fi_cq_read() by the Writer. fi_read() will fail with EAGAIN until the writer has acknowledged the load request. It seems that (at least with my current approach) this requires a ping-pong sort of protocol: I tried decreasing latencies by processing fi_read() as well as fi_cq_read() in batches and it made no difference, the provider only processes one load request at a time. In consequence, the current implementation has extreme latencies since it puts the progress thread to sleep before poking the provider again.

@eisenhauer mentioned in a video call that the control plane of SST implements a potential alternative approach based on file-descriptor polling.

Further benefit of implementing manual progress:
One of the most common issues with a badly-configured installation of the libfabric dataplane are hangups. Having support in SST for manual progress might fix this in some instances.
I have observed that this PR also "unlocks" the tcp provider which previously did not work.

Future potential / ideas:

Both these following points are probably an immense amount of work. Just some ideas that I wanted to put out here on how SST might be used to implement zero-overhead staging workflows:

  • This might be used to introduce a notion of memory hierarchy into SST. Local data can be loaded via shared memory, while remote data is sent via the network. This might immensely decrease the load on the network for large-scale application runs.
    I imagine that this is probably not an easy change to implement, since the control plane would need to deal with two data planes at once.
  • Not sure if this is possible with libfabric's shared memory provider, but it might be possible to use the zero-copy Engine-Get call void Engine::Get<T>(Variable<T>, T**) const; for data from the same node (currently used by the Inline Engine).

TODO:

  • Lazy connecting of endpoints (endpoints might not be reachable in shm settings)
  • Parameterization: Batch reading, threaded reading in ucx
  • threaded reading in libfabric: make it depend on PROGRESS_MANUAL parameter
  • no threading on the reader end

@eisenhauer
Copy link
Member

I can take a look at enabling a manual progress thread, or possibly using EVPath tools to tie progress to FDs if supported by CXI, but realistically I have one week before I disappear for two weeks, and given the other things on my plate the odds of this happening before January are unfortunately small.

WRT the future work notes, yes, supporting different data planes between different ranks is probably impactical given how SST is architected. It would have to be a single data plane that supported both transport mechanisms, which is still a lot of work, but fits the way dataplanes integrate into SST. I've also long had in mind an extension to the data access mechanisms that might reduce the copy overheads for RDMA and shared memory data access, but it involves several changes from the BP5Deserializer, through the engine and down to the data plane, so it has remained on the to-do list for a long time. But it's something to re-examine at some point.

@franzpoeschel
Copy link
Contributor Author

I can take a look at enabling a manual progress thread, or possibly using EVPath tools to tie progress to FDs if supported by CXI, but realistically I have one week before I disappear for two weeks, and given the other things on my plate the odds of this happening before January are unfortunately small.

No problem, this is not urgent.
It turns out that the solution was simpler than I had expected. Instead of running fi_cq_read() (non-blocking) on the thread every five seconds, I now run fi_cq_sread() (blocking) on the thread with a timeout of five seconds. The shm provider becomes much more responsive with this change. With this, I expect that the control-plane-enabled solution might not be needed any longer.

WRT the future work notes, yes, supporting different data planes between different ranks is probably impactical given how SST is architected. It would have to be a single data plane that supported both transport mechanisms, which is still a lot of work, but fits the way dataplanes integrate into SST. I've also long had in mind an extension to the data access mechanisms that might reduce the copy overheads for RDMA and shared memory data access, but it involves several changes from the BP5Deserializer, through the engine and down to the data plane, so it has remained on the to-do list for a long time. But it's something to re-examine at some point.

Thank you for the info. My main motivation in posting these ideas was to get a rough estimation of how viable these are to implement. It does not surprise me very much that lots of work would be required.

@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Dec 15, 2023

It seems that the current thread-based implementation runs into a libfabric bug, fixed by ofiwg/libfabric#9644. The bug means that calls to fi_cq_sread() that the progress thread potentially might make at the end of the simulation will not return. Finalizing the dataplane will hence not work.

Ref. the meeting with the Maestro team: The fabric should not be
bombarded with too many requests at once. Batch size currently hardcoded
as 10.
Todo: Better than doing this, initialize endpoints on demand only
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Jul 26, 2024

@eisenhauer

I think that this is now mostly ready for review. I tested this on our local system today, still need to test on Frontier, forgot my hardware key today.

To summarize:

1. Batch processing
Implemented in engine/sst/SstReader.(c|h|t)pp. I'll leave it up to you if you want to merge this, it can also be reverted as it is orthogonal to the other changes.
Instead of pushing all operations to the data plane at once and then waiting for them, this enqueues and waits for them in batches.
This was necessary for full-scale runs on Frontier.

If we decide to merge it, then this should be configurable as an engine parameter, the batch size is currently a hardcoded as a constant.

2. libfabric DP
This adds:

  1. A progress thread that is automatically launched at the writer side if the engine indicates manual data progress. It can be turned on or off manually (also for the reader side) using export FABRIC_PROGRESS_THREAD=yes.
  2. On the reader side, fi_cq_sread() is called at appropriate places when the fabric requires manual progress.
  3. An environment variable FABRIC_PROVIDER to specify the fabric module to select for. Together with the old FABRIC_IFACE which specifies the domain, this allows for a very precise fabric selection. (The same domain can be implemented by several fabrics, leading to easy-to-break setups)
  4. With this, support for the shm fabric for shared memory communication. FABRIC_IFACE=shm must be specified, the fabric is not selected automatically since it does not allow cross-node communication.
  5. Some small fixes: More error-tolerant address handling and retrieval of CXI credentials.

Additionally I noticed, that some fabrics (such as psm3 on our local system) don't support FI_MR_PROV_KEY, meaning that in fi_mr_reg(), the requested_key parameter must not be 0, but something else. Those fabrics currently don't work, but would be relatively trivial to also support by just counting keys starting from 1... Maybe as a follow-up PR.

Example from the output of fi_info:

provider: sockets
    fabric: fe80::/64
    domain: eno1
    version: 117.0
    type: FI_EP_MSG
    protocol: FI_PROTO_SOCK_TCP
provider: tcp
    fabric: fe80::/64
    domain: eno1
    version: 117.0
    type: FI_EP_MSG
    protocol: FI_PROTO_SOCK_TCP

FABRIC_IFACE is the same for both configurations above, namely eno1, but FABRIC_PROVIDER is set to either sockets or tcp.

3. UCX DP

This also adds a progress thread capability similar to the libfabric DP. I found no way of automatically figuring out if a worker needs manual progress, so it's turned off by default and must be enabled via export SST_UCX_PROGRESS_THREAD=yes. This is only implemented on the writer side as it leads to errors in the reader, possibly due to interfering requests on main thread and progress thread.

Additionally fixes a forgotten call to ucp_rkey_destroy().

For shared memory communication, the following environment variables must be set:

export SST_UCX_PROGRESS_THREAD=yes
export UCX_TLS=shm # restricts UCX to shared memory
export UCX_POSIX_USE_PROC_LINK=n # workaround for some bug in UCX

When I tested this on multiple nodes (data exchange via SST only intra-node), I noticed that the UCX DP reader side will contact all writers upon initialization in UcxProvideWriterDataToReader(). I don't know if this is intentional (seems like a potential scaling issue), but this obviously cannot work in such shared memory setups. So I changed that function to ignore a connection failure at that point and print a warning instead.

I did not yet document anything except for inline comments.

@eisenhauer
Copy link
Member

Thanks Franz! I'm up to my neck in other things at the moment, but will look at this when I can...

@franzpoeschel franzpoeschel changed the title [WIP] support for shared memory provider of libfabric in SST + simple support for manual data progress via threading Support for shared memory provider of libfabric in SST + simple support for manual data progress via threading Jul 29, 2024
@franzpoeschel
Copy link
Contributor Author

No problem, take the time you need.
I just tested this in a two-node job on Frontier, RDMA via libfabric still works and now additionally shared memory via a self-compiled UCX installation. (Since the system libfabric does not have the shm provider, shared memory via libfabric cannot be used.)

@franzpoeschel
Copy link
Contributor Author

Additionally I noticed, that some fabrics (such as psm3 on our local system) don't support FI_MR_PROV_KEY, meaning that in fi_mr_reg(), the requested_key parameter must not be 0, but something else. Those fabrics currently don't work, but would be relatively trivial to also support by just counting keys starting from 1... Maybe as a follow-up PR.

This commit does this, but I've not added it to the PR yet. I can either do that or submit it as a follow-up.

@@ -798,6 +1027,11 @@ static int get_cxi_auth_key_from_env(CP_Services Svcs, void *CP_Stream, struct _
char const *slingshot_devices = getenv("SLINGSHOT_DEVICES");
char const *preferred_device = get_preferred_domain(Params);

if ((!preferred_device && strncmp("cxi", preferred_device, 3) != 0) || !slingshot_devices)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if ((!preferred_device && strncmp("cxi", preferred_device, 3) != 0) || !slingshot_devices)
if ((!preferred_device || strncmp("cxi", preferred_device, 3) != 0) || !slingshot_devices)

I need to check this logic again, currently this is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed with ba7d6c1

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