Ompi: BTL UCT causing problems with UCX memory hooks

[jsquyres]

From https://www.mail-archive.com/[email protected]/msg20895.html and https://www.mail-archive.com/[email protected]/msg20904.html:

The issue below is caused by btl_uct – it mistakenly calls ucm_set_external_event() without checking opal_mem_hooks_support_level().
This leads UCX to believe that memory hooks would be provided by OMPI, but in fact they are not, so pinned physical pages become out-of-sync with process virtual address.

btl_uct wrong call: https://github.com/open-mpi/ompi/blob/master/opal/mca/btl/uct/btl_uct_component.c#L132
Correct way: https://github.com/open-mpi/ompi/blob/master/opal/mca/common/ucx/common_ucx.c#L104

FYI @yosefe

v4.0.x RMs: this feels like a blocker for v4.0.x. Thoughts?

[yosefe]

related to https://github.com/openucx/ucx/issues/3581

[hppritcha]

@jsquyres yes I agree.

[hppritcha]

I really don't want to bother fixing this. @gpaulsen we should discuss on Friday. I'm in the mood to by default not build the UCT BTL at all. That would also solve the API breakage issue.

[hjelmn]

@yosefe Could have just tagged me and the fix would have been in within an hour or so. Good grief.

[jsquyres]

My bad -- I shouldn't have set #6668 to auto-close this issue. Leaving it open until the fix is applied to v4.0.x.

[gpaulsen]

In today's web-ex @jsquyres mentioned some discussion he's had with @hjelmn about #6668.
To summarize, @jsquyres likes #6668 as a start, but would also like to see some UCX version checking (both high and low) when Open MPI would build btl_uct by default.
On the low side, the version of UCX we test v4.0.0 with, and on the high side the current working version of UCX. This configury would be applied to both master and v4.0.x branch (in time for v4.0.2).
And of course with this change, he proposed a new configury option to override this default.
Neither @hppritcha or @hjelmn were able to be there at today's web-ex. Hopefully you can comment on this idea here.

[jsquyres]

Specifically, my proposal is:

• If UCX is between vX and v1.6, the BTL UCT configure.m4 should build by default.
  
  
  
  • I do not know the value of X (i.e., the lower bound UCX version that the BTL UCT supports).
  
  
• If UCX is < vX, do not build.
• If UCX is > v1.6, do not build by default. But if --enable-btl-uct-build-anyway (or something similar) is specified, go ahead and build.

[gpaulsen]

Thanks @jsquyres. I Should also mention that PR #6670 is also related.

[hjelmn]

@jsquyres My problem with the proposal is we do not put such a restriction on any other component. I plan to keep up with the changes in the future. I was not able to get to v1.6 support quickly for obvious reasons. v1.7 support will be coming in the near future.

[jsquyres]

@hjelmn The reason for this proposal is that the UCT APIs are not guaranteed to be stable between releases. Other components do not use internal APIs like this. We really do not want to be in the situation where a shipping Open MPI fails a default compile because we're using internal UCX APIs that disappeared/changed in a subsequent UCX release.

[hjelmn]

@jsquyres The UCP API is not guaranteed to be stable between release either. If this is pushed on btl/uct it must also be pushed on pml/ucx and spml/ucx as well. Again, UCT is not internal. It is an exposed API of Open UCX.

[jsquyres]

We talked about this exact issue on the call this morning. We were told by both Mellanox that the APIs that they use fall under the UCX stability guarantees. Is this not correct?

EDIT: s/that they are/that they use/

[hjelmn]

@jsquyres I can't quite parse that.

I was told by the Open UCX community that the stability guarantee extends to UCT. They wanted to make some changes before locking the API. I believe v1.7 represents that point. @yosefe Correct me if I am wrong.

[jsquyres]

If the UCT APIs are stable, then great: I agree that this proposal becomes moot. The only reason this proposal exists is because we were told that the UCT APIs are not stable.

[hjelmn]

@jsquyres That is incorrect or the UCX community misled me.

[hjelmn]

FWIW UCX has not broken the UCT API in a release series (that I know of). Like UCP any changes occur in a 1.x.0 release. The endpoint initialization changes occured at v1.6.0, the discovery APIs are changing in v1.7.0.

[yosefe]

adding @shamisp
UCP API (which is used by pml/ucx, spml/ucx, ...) is already stable and will not break in a release series.
UCT is going to be stable, but it's not there yet, the current plan is to make it stable starting from v1.7 (but there is still quite a lot of work to get there).

[jsquyres]

@yosefe Thanks for the clarification; that's very helpful to know.

So if there's potentially going to be more UCT changes between v1.6 and v1.7, I think the version number check in configury is still a good idea. Once UCX hits v1.7 / UCT becomes stable and we adapt the BTL UCT to adjust to the stable UCT API, then I think we can drop the version check and just do normal configury-style functionality checks.

[hppritcha]

closed on 4.0.x via #6748

[hppritcha]

well actually the commit was cherry-picked back to 4.0.x so removing 4.0.x label and closing.

[gpaulsen]

Closing since both PRs were merged to both master and v4.0.x