2.0 deprecations #10250

j-xiong · 2024-07-31T01:22:09Z

This is the first part of reworked #10009. It contains all the API deprecations. Major changes from the original PR:

APIs are marked deprecated instead of removed.
Compiler directives are added to generate warnings when the deprecated APIs are used.
Provider support for these deprecated APIs is mostly retained (especially w.r.t. MR modes, wait set, and poll set).
Providers used Internal version of the deprecated APIs to maintain compatibility while avoiding compiler warnings.

Several defines and values should not have been exposed in the public header files. Remove or move the definitions into internal headers. This removes the chance of possible conflicts with application definitions and API breakage. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

j-xiong · 2024-07-31T06:17:22Z

bot:aws:retest

j-xiong · 2024-07-31T15:40:33Z

@shijin-aws Could you check the AWS CI error?

shijin-aws · 2024-07-31T15:54:12Z

Ompi 4.1.x failed to compile with this PR. @wenduwan The log is pretty long and I haven't seen how it could be caused by this PR

mtl_ofi.h: In function ‘ompi_mtl_ofi_context_progress’:
mtl_ofi_request.h:19:5: error: implicit declaration of function ‘container_of’ [-Werror=implicit-function-declaration]
   19 |     container_of((_ptr_ctx), struct ompi_mtl_ofi_request_t, ctx)
      |     ^~~~~~~~~~~~
mtl_ofi.h:121:31: note: in expansion of macro ‘TO_OFI_REQ’
  121 |                     ofi_req = TO_OFI_REQ(ompi_mtl_ofi_wc[i].op_context);
      |                               ^~~~~~~~~~
mtl_ofi_request.h:19:30: error: expected expression before ‘struct’
   19 |     container_of((_ptr_ctx), struct ompi_mtl_ofi_request_t, ctx)
      |                              ^~~~~~
mtl_ofi.h:121:31: note: in expansion of macro ‘TO_OFI_REQ’
  121 |                     ofi_req = TO_OFI_REQ(ompi_mtl_ofi_wc[i].op_context);
      |                               ^~~~~~~~~~
mtl_ofi_request.h:19:30: error: expected expression before ‘struct’
   19 |     container_of((_ptr_ctx), struct ompi_mtl_ofi_request_t, ctx)
      |                              ^~~~~~
mtl_ofi.h:167:23: note: in expansion of macro ‘TO_OFI_REQ’
  167 |             ofi_req = TO_OFI_REQ(error.op_context);

wenduwan · 2024-07-31T16:14:19Z

This looks odd. But one oddity in open mpi is mtl_ofi.h and mtl_ofi_request.h have circular dependency. Not sure how it was working before though.

wenduwan · 2024-07-31T16:16:57Z

@hppritcha Do you have any thoughts?

For libfabric 2.0 do we need to make configury changes in ompi?

j-xiong · 2024-07-31T16:40:27Z

This first commit in the PR removed container_of from the public headers:
6ac8242

That's probably the reason of the failure.

wenduwan · 2024-07-31T16:45:01Z

In this case I think open mpi should be more defensive, and define its own container_of - @hppritcha what do you think?

wenduwan · 2024-07-31T17:17:11Z

@j-xiong While I don't think open mpi is doing the right thing, I also wonder if there is a strong reason to move the container_of macro in libfabric?

As a result of this change, existing open mpi tarballs will not build with libfabric 2.0. There might be other problems as well but we can go from here.

shijin-aws · 2024-07-31T17:46:23Z

ompi cannot rely on container_of from libfabric headers, it's something broken on the ompi side

j-xiong · 2024-07-31T18:02:11Z

container_of is not supposed be defined in libfabric public headers. Libfabric core and providers use it so we still have it defined internally. Applications should define their own.

wenduwan · 2024-07-31T18:52:22Z

I agree with the long-term plan to have applications define container_of individually. But I would like to explore it is ok to keep it in libfabric header for now so open mpi(and maybe other affected packages) can take actions separately. In other words, does container_of have to be relocated as part of libfabric 2.0?

shefty

I didn't notice any issues. Not sure if the Pragma works for all builds (windows, OS X, etc.)

shefty · 2024-07-31T19:03:27Z

Updating the major version from 1 to 2 indicates that the changes may break apps. Otherwise, this is just another minor version update. The expectation is that apps may actually need to update to support the newer version. container_of() doesn't include a prefix, can conflict with other definitions, and was never defined as a usable API.

As a possible option, you could introduce a new header, e.g. fi-compat.h, where removed/deprecated definitions are relocated to. That header could be included using a compiler define. Then the upper app wouldn't need to change, but it would need to specify a define when building against the libfabric headers. (I think this might work...)

wenduwan · 2024-07-31T19:46:42Z

I see. So moving this macro likely requires major version bump. This makes sense.

Thanks for the suggestion. I need to poke the open mpi community on the mitigation for users who want to build old open mpi with libfabric 2.0.

j-xiong · 2024-07-31T22:19:52Z

The pragma works with macos. Result from macos build check:

common/check_hmem.c:47:34: warning: 'FI_MR_BASIC' is deprecated [-W#pragma-messages]
        hints->domain_attr->mr_mode = ~(FI_MR_BASIC | FI_MR_SCALABLE);
                                        ^
/Users/runner/work/libfabric/libfabric/fabtests/../install/include/rdma/fabric.h:232:22: note: expanded from macro 'FI_MR_BASIC'
#define FI_MR_BASIC             _Pragma("GCC warning \"'FI_MR_BASIC' is deprecated\"")          (1 << 0)
                                ^
<scratch space>:43:6: note: expanded from here
 GCC warning "'FI_MR_BASIC' is deprecated"
     ^

On windows it is not recognized and produces a warning but still serves the purpose of attracting the attention:

common\shared.c(281): warning C4068: unknown pragma 'GCC' [C:\projects\libfabric\fabtests\fabtests.vcxproj]

j-xiong · 2024-07-31T22:23:51Z

I am going to ignore the AWC CI failure for now. Please let me know if there are any other concerns about that.

shijin-aws · 2024-07-31T22:30:50Z

@j-xiong can you hold it for 1 day, we need to fix our CI otherwise all the future main branch PR will fail after this one is merged.

j-xiong · 2024-07-31T22:33:05Z

@shijin-aws Sure. Please let me know when you think it is ready.

aingerson · 2024-07-31T22:27:10Z

man/fi_av.3.md

@@ -13,7 +13,8 @@ fi_av_open / fi_close
 : Open or close an address vector

 fi_av_bind
-: Associate an address vector with an event queue.
+: Associate an address vector with an event queue. This funciton


Spelled function wrong

aingerson · 2024-07-31T22:29:00Z

man/fi_av.3.md

-to operate asynchronously by specifying the FI_EVENT flag to
-`fi_av_open`.  When requesting asynchronous operation, the application
-must first bind an event queue to the AV before inserting addresses.  See
+protocols.  AV operations are synchronous by default. See


Maybe remove the "by default" since there is no alternative anymore?

The alternative is in the deprecated section, but I think I can make it clearer.

aingerson · 2024-07-31T22:38:53Z

man/fi_av.3.md

+  Because the fi_addr_t values returned from an insertion call are
+  deterministic, applications may not need to provide the fi_addr_t
+  output parameters to insertion calls.  The exception is when
+  authentication keys are required for communication.


I noticed you removed the part in av_insert that specifies that the fi_addr_t can be used as an index into an array, presumably because it might be extended to into include other information in the upper 32 bits or something. It feels like that conflicts with this addition. The app should be providing the fi_addr, even in FI_AV_TABLE in order to save the full context of information, no?

The removed part just enumerates a few possible cases which table index as one of them. They are removed because we don't really need those details here, especially since we are deprecating the av map semantics.

The upper 32-bit usage is in a later patch, we don't need to count for that now.

I thought another exception is FI_AV_TABLE_USER_ID? In that case fi_addr_t is also an input.

I am changing this to: The exception is when the fi_addr_t parameters are also used as input for supplying authentication keys or user defined IDs

aingerson · 2024-07-31T22:44:19Z

prov/util/src/util_av.c

-			return -FI_ENOEQ;
-		}
-
-		if (flags & FI_SYNC_ERR) {


I've never used FI_SYNC_ERR before - it seems like it requires an EQ for the AV. Does this need to be deprecated as well? There seem to still be providers that check this value and use it. Just clarifying

This flag allows details of errors being reported for synchronous av insertion. It doesn't require av-eq binding, but requires the context parameter pointing to array for storing error codes.

aingerson · 2024-07-31T22:54:25Z

fabtests/functional/unexpected_msg.c

@@ -372,7 +372,6 @@ int main(int argc, char **argv)
 	hints->mode = FI_CONTEXT;
 	hints->domain_attr->mr_mode = opts.mr_mode;
 	hints->domain_attr->resource_mgmt = FI_RM_ENABLED;
-	hints->rx_attr->total_buffered_recv = 0;


Not important but if you're going to update the patch set anyway, "compatibility" is spelled wrong in the commit message

aingerson · 2024-07-31T22:59:12Z

fabtests/unit/getinfo_test.c

@@ -181,8 +181,7 @@ static int validate_tx_ordering_bits(char *node, char *service, uint64_t flags,
 		struct fi_info *hints, struct fi_info **info)
 {
 	return validate_bit_combos(node, service, flags, hints, info,


You should be able to just remove the entire validate_tx/rx_ordering_bits tests. The validate_bit_combos iterates over all combinations of the bits and tests all combinations of them so if it's set to 0, there are no bits to test so the call is redundant

wait. the value passed in is all 1, not 0.

oh sorry must have misread that!

aingerson · 2024-07-31T23:01:47Z

man/fi_endpoint.3.md

@@ -1048,11 +1048,6 @@ transfer operation in order to guarantee that ordering is met.
  update operations.  If not atomic updates may be
  transmitted out of order from their submission.

-*FI_ORDER_NONE*


Do you want to remove or add a deprecated note like the others?

This one was missed in the transition. Need to add a deprecated note.

Previously mtl_ofi_request.h inadvertently used container_of as defined by libfabric in include/rdma/fabric.h, however libfabric is removing that symbol from their headers as part of changes supporting libfabric 2.0 API. (see ofiwg/libfabric#10250). This change defines our own (compatible) container_of macro to remove the external dependence, but only if it is not already defined. Signed-off-by: Luke Robison <[email protected]>

shijin-aws · 2024-08-01T18:14:40Z

open-mpi/ompi#12724 has been merged to ompi main to fix the illegal container_of import

The current plan is to backport this fix to 4.1.x and 5.0.x branch by today, since we are always building the head of ompi branches in AWS CI, this should be ok

shijin-aws · 2024-08-01T20:39:02Z

man/fi_av.3.md

@@ -685,7 +676,7 @@ buffer associated with a failed or aborted insertion will be set to
 FI_ADDR_NOTAVAIL.

 All other calls return FI_SUCCESS on success, or a negative value corresponding
-to fabric errno on error. Fabric errno values are defined in `rdma/fi_errno.h`.
+to fabric errno on error.  Fabric errno values are defined in `rdma/fi_errno.h`.


nit: extra space

As a convention, double spaces have been used in the man pages between full sentences. Doesn't seem to affect the output though.

shijin-aws · 2024-08-01T21:44:07Z

prov/efa/src/efa_av.c

@@ -678,17 +678,10 @@ int efa_av_insert(struct fid_av *av_fid, const void *addr,

 	/* cancel remaining request and log to event queue */
 	for (; i < count ; i++) {
-		if (av->util_av.eq)


I am not sure why efa has to do this before with no support of FI_EVENT

shijin-aws · 2024-08-01T21:57:15Z

include/rdma/fi_domain.h

@@ -327,7 +327,7 @@ struct fi_ops_mr {
 };

 /* Domain bind flags */
-#define FI_REG_MR		(1ULL << 59)
+#define FI_REG_MR		(1ULL << 59)  /* deprecated */


Do we want to do the pragma trick in af29844 for all deprecated bits?

shijin-aws · 2024-08-01T22:00:48Z

prov/efa/src/efa_prov_info.c

@@ -73,7 +73,7 @@ const struct fi_domain_attr efa_domain_attr = {
 	.control_progress	= FI_PROGRESS_AUTO,
 	.data_progress		= FI_PROGRESS_AUTO,
 	.resource_mgmt		= FI_RM_DISABLED,
-	.mr_mode		= OFI_MR_BASIC_MAP | FI_MR_LOCAL | FI_MR_BASIC,
+	.mr_mode		= OFI_MR_BASIC_MAP | FI_MR_LOCAL | OFI_MR_BASIC,


I believe efa code has more occurrence for FI_LOCAL_MR to handle old API, but it's ok to leave as is for this PR

There is only one occurrence in efa unit test, which I think should leave it as it is (tests are only supposed to see public headers).

Deprecate support for asynchronous insertions and AV_MAP. The format of the fi_addr_t value will either be indexed based in the standard case or provider defined in more advanced use cases, based on the AV configuration (such as using auth_keys). Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Remove FI_BUFFERED_RECV as an exported API option. Since it's currently used internally between mrail and rxm, make it an internal only option. It has a limited use case for multirail over rxm over connected endpoints where shared receive queues are not available. With shared receive queues, the feature wouldn't be needed, as mrail could own the buffers outright. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Deprecate overly complicated threading models and focus on specific models to allow better alignment between application designs and provider implementation. Use FI_THREAD_DOMAIN as the preferred lockless threading model for standard endpoints. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Combine data and control progress into one progress option. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Completions are always unordered. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Field was deprecated and only serves as a placeholder for compatibility. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Feature is not implemented natively by providers. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Remove FI_ORDER_NONE (flag that's 0) and FI_ORDER_STRICT (which isn't a flag, and covers only a portion of the valid flags). Remove FI_ORDER_DATA, which will not be supported by version 2 in order to allow for greater provider optimization. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

Disallow users hand-crafting or hand-copying their own fi_info structs. This allows the library to allocate hidden fields for internal use. Plus, there's no need to apps to do this, given that the API call is way easier to use. Signed-off-by: Sean Hefty <[email protected]>

Mark 'struct fid_wait' and 'struct fid_poll' as deprecated, as well as the following functions that take objetcs of these types as input: fi_wait_open() fi_wait() fi_poll_open() fi_poll() fi_poll_add() fi_poll_del() Providers use internal version of these functions to avoid compiler warnings. Signed-off-by: Jianxin Xiong <[email protected]>

This wait object type is only implemened by the sockets provider as an exmaple. Signed-off-by: Jianxin Xiong <[email protected]>

FI_MR_BASIC / FI_MR_SCALABLE / FI_MR_UNSPEC / FI_LOCAL_MR have been deprecated since verison 1.5. However, no mechanism was in place to discourage their usage. Add compiler directives to generate warning messages if these defintions are used. Providers use internal version of these values to maintain application compatibility w/o generating warning messages. Signed-off-by: Jianxin Xiong <[email protected]>

wenduwan · 2024-08-02T02:53:01Z

@shijin-aws Can you restart AWS CI?

j-xiong · 2024-08-02T06:47:14Z

bot:aws:retest

j-xiong · 2024-08-02T15:52:32Z

@shijin-aws @wenduwan What is the current CI error?

wenduwan · 2024-08-02T15:56:07Z

No description provided.

wenduwan · 2024-08-02T15:57:33Z

No description provided.

wenduwan · 2024-08-02T16:03:02Z

server_command: ssh -n -o StrictHostKeyChecking=no -o ConnectTimeout=30 -o BatchMode=yes 172.31.33.58 'timeout 1000 /bin/bash --login -c '"'"'FI_LOG_LEVEL=warn ASAN_OPTIONS=detect_leaks=0:protect_shadow_gap=0 /home/ec2-user/PortaFiducia/build/libraries/libfabric/pr10250-debug-mempoison-asan/install/fabtests/bin/fi_efa_exhaust_mr_reg_rdm_pingpong -I 5 -v -S all -p efa -E'"'"''

Client error: Server has terminated

server_stdout:

bind: Address already in use


server returncode: 1

____________________ test_rma_bw_1G[delivery_complete-read] ____________________

efa/test_rma_bw.py:62: in test_rma_bw_1G

    efa_run_client_server_test(cmdline_args, command, 2,

efa/efa_common.py:26: in efa_run_client_server_test

    test.run()

../../../../../../../../../env/lib64/python3.8/site-packages/retrying.py:56: in wrapped_f

    return Retrying(*dargs, **dkw).call(f, *args, **kw)

../../../../../../../../../env/lib64/python3.8/site-packages/retrying.py:257: in call

    return attempt.get(self._wrap_exception)

../../../../../../../../../env/lib64/python3.8/site-packages/retrying.py:301: in get

    six.reraise(self.value[0], self.value[1], self.value[2])

../../../../../../../../../env/lib64/python3.8/site-packages/six.py:719: in reraise

    raise value

../../../../../../../../../env/lib64/python3.8/site-packages/retrying.py:251: in call

    attempt = Attempt(fn(*args, **kwargs), attempt_number, False)

common.py:573: in run

    check_returncode_list([server_process.returncode, client_returncode],

common.py:223: in check_returncode_list

    pytest.fail(reason)

E   Failed: returncode 1

Now I'm seeing a bunch of fabtests failures

wenduwan · 2024-08-02T16:09:45Z

Looks like our port randomization logic was lost somewhere 🤔

wenduwan · 2024-08-02T16:17:26Z

@j-xiong We have a couple CI changes coming up but it will take some time to deploy. Let me rerun the test locally.

wenduwan · 2024-08-02T20:16:28Z

@shijin-aws @j-xiong Could you restart AWS CI? Our fixes have been deployed.

j-xiong · 2024-08-02T20:33:51Z

bot:aws:restart

wenduwan · 2024-08-02T20:39:55Z

bot:aws:restart > bot:aws:retest

j-xiong · 2024-08-02T22:48:28Z

bot:aws:retest

j-xiong force-pushed the 2.0-deprecations branch 3 times, most recently from 74ef6ba to d833ed5 Compare July 31, 2024 03:55

j-xiong requested review from shefty, aingerson, shijin-aws, acgoldma and belynam July 31, 2024 16:57

shefty approved these changes Jul 31, 2024

View reviewed changes

aingerson reviewed Jul 31, 2024

View reviewed changes

j-xiong force-pushed the 2.0-deprecations branch from d833ed5 to 594dd91 Compare August 1, 2024 00:53

shijin-aws reviewed Aug 1, 2024

View reviewed changes

shefty and others added 12 commits August 1, 2024 16:20

core: Simplify progress definition

0cb6818

Combine data and control progress into one progress option. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

core: Deprecate comp_order attribute

1f44352

Completions are always unordered. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

core: Remove total_buffered_recv

044017f

Field was deprecated and only serves as a placeholder for compatibility. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

core: Deprecate support for async memory registration

bc62158

Feature is not implemented natively by providers. Signed-off-by: Sean Hefty <[email protected]> Signed-off-by: Jianxin Xiong <[email protected]>

core: Deprecate FI_WAIT_MUTEX_COND

8625276

This wait object type is only implemened by the sockets provider as an exmaple. Signed-off-by: Jianxin Xiong <[email protected]>

j-xiong force-pushed the 2.0-deprecations branch from 594dd91 to d689aa8 Compare August 1, 2024 23:25

belynam approved these changes Aug 2, 2024

View reviewed changes

j-xiong merged commit 2c364ea into ofiwg:main Aug 3, 2024
15 checks passed

j-xiong mentioned this pull request Aug 6, 2024

Ofi 2.0 changes #10009

Closed

2.0 deprecations #10250

2.0 deprecations #10250

Conversation

j-xiong commented Jul 31, 2024

j-xiong commented Jul 31, 2024

j-xiong commented Jul 31, 2024

shijin-aws commented Jul 31, 2024

wenduwan commented Jul 31, 2024

wenduwan commented Jul 31, 2024

j-xiong commented Jul 31, 2024

wenduwan commented Jul 31, 2024

wenduwan commented Jul 31, 2024

shijin-aws commented Jul 31, 2024

j-xiong commented Jul 31, 2024

wenduwan commented Jul 31, 2024

shefty left a comment

Choose a reason for hiding this comment

shefty commented Jul 31, 2024

wenduwan commented Jul 31, 2024

j-xiong commented Jul 31, 2024 • edited Loading

j-xiong commented Jul 31, 2024

shijin-aws commented Jul 31, 2024

j-xiong commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-xiong Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-xiong Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shijin-aws commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shijin-aws Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenduwan commented Aug 2, 2024

j-xiong commented Aug 2, 2024

j-xiong commented Aug 2, 2024

wenduwan commented Aug 2, 2024 • edited Loading

wenduwan commented Aug 2, 2024 • edited Loading

wenduwan commented Aug 2, 2024

wenduwan commented Aug 2, 2024

wenduwan commented Aug 2, 2024

wenduwan commented Aug 2, 2024

j-xiong commented Aug 2, 2024

wenduwan commented Aug 2, 2024

j-xiong commented Aug 2, 2024

j-xiong commented Jul 31, 2024 •

edited

Loading

j-xiong Aug 1, 2024 •

edited

Loading

j-xiong Aug 1, 2024 •

edited

Loading

shijin-aws Aug 1, 2024 •

edited

Loading

wenduwan commented Aug 2, 2024 •

edited

Loading

wenduwan commented Aug 2, 2024 •

edited

Loading