prov/ucx: Fix completion entries insertion for RMA operations. #11493

tatarintsevsv · 2025-10-12T12:15:14Z

Ignore the absence of the FI_COMPLETION flag if CQ configured without FI_SELECTIVE_COMPLETION flag.
Error completions must generated for all operations, including those for which a completion was not requested

j-xiong · 2025-10-13T17:26:28Z

prov/ucx/src/ucx_rma.c

 			ofi_ep_cntr_inc(&u_ep->ep, CNTR_RD);

-		if (flags & FI_COMPLETION)
+		if (flags & FI_COMPLETION || u_ep->ep.tx_op_flags & FI_COMPLETION)


tx_op_flags is already considered when ucx_proc_rma_msg is called.

j-xiong · 2025-10-13T17:28:59Z

prov/ucx/src/ucx_rma.c


-		if (ucx_req->completion.flags & FI_COMPLETION)
+		if (ucx_req->ep->ep.tx_op_flags & FI_COMPLETION ||
+		    ucx_req->completion.flags & FI_COMPLETION)


Same here.

Should use !(u_ep->ep.tx_op_flags & FI_SELECTIVE_COMPLETION) to check for the case that selective completion is not asked for.

I think you should just be able to check the completion.flags which are set when initializing the request. The op flags are set to force include FI_COMPLETION when FI_SELECTIVE_COMPLETION is not requested so it should be ok. See here

I think the missing thing in the current code is the OR'ing in of the msg flags on the *msg APIs to make sure FI_COMPLETION is set in the req->completion.flags for the non FI_SELECTIVE_COMPLETION case.

@tatarintsevsv Can you explain the selective completion path that is incorrectly writing a completion when it shouldn't be? I don't immediately see it.

I think you should just be able to check the completion.flags which are set when initializing the request. The op flags are set to force include FI_COMPLETION when FI_SELECTIVE_COMPLETION is not requested so it should be ok. See here

I think the missing thing in the current code is the OR'ing in of the msg flags on the *msg APIs to make sure FI_COMPLETION is set in the req->completion.flags for the non FI_SELECTIVE_COMPLETION case.

@tatarintsevsv Can you explain the selective completion path that is incorrectly writing a completion when it shouldn't be? I don't immediately see it.

According to manual, in this this case cq records must be generated

fi_ep_bind(ep, &cq->fid, FI_TRANSMIT|FI_RECV); fi_writemsg(ep, &msg, 0);

but it's so only if we pass FI_COMPLETION to fi_writemsg flags.

As far as I understand, the correct usage of FI_COMPLETION / FI_SELECTIVE_COMPLETION is

fi_ep_bind(ep, &cq->fid, FI_TRANSMIT|FI_RECV|FI_SELECTIVE_COMPLETION); fi_writemsg(ep, &msg, FI_COMPLETION); // completion must be generated fi_writemsg(ep, &msg, 0); // completion doesn't matter, will not be generated

and

fi_ep_bind(ep, &cq->fid, FI_TRANSMIT|FI_RECV); fi_writemsg(ep, &msg, FI_COMPLETION); // completion must be generated fi_writemsg(ep, &msg, 0); // completion must be generated anyway

U just need to use flags | util_ep->tx_msg_flags as the internal flags when you implement fi_*msg. tx_msg_flags will be 0 when FI_SELECTIVE_COMPLETION is used when ep bind, it will be FI_COMPLETION when there is no FI_SELECTIVE_COMPLETION

Then you check whether that internal flags have FI_COMPLETION to determine whether you want to generate completions

as it pointed by @j-xiong this is not correct - we must adds flags but not override.
fixed with only FI_COMPLETION is taken into account

tatarintsevsv · 2025-10-14T04:24:35Z

After looking more closely, it seems this error only occurs with ucx_writemsg and ucx_readmsg. I'll fix the PR.

tatarintsevsv · 2025-10-22T02:29:14Z

can anyone review last changes?

j-xiong · 2025-10-22T15:42:01Z

prov/ucx/src/ucx_rma.c

 {
-	return ucx_proc_rma_msg(ep, msg, flags, UCX_DO_WRITE);
+	struct ucx_ep *u_ep = container_of(ep, struct ucx_ep, ep.ep_fid);
+	return ucx_proc_rma_msg(ep, msg, flags | u_ep->ep.tx_op_flags, UCX_DO_WRITE);


This is incorrect. The flags passed to the msg version override the default op flags, no adds to.

Agreed. I've fixed it so that only FI_COMPLETION is taken into account in the internal flags.

j-xiong · 2025-10-22T15:42:12Z

prov/ucx/src/ucx_rma.c

 {
-	return ucx_proc_rma_msg(ep, msg, flags, UCX_DO_READ);
+	struct ucx_ep *u_ep = container_of(ep, struct ucx_ep, ep.ep_fid);
+	return ucx_proc_rma_msg(ep, msg, flags | u_ep->ep.rx_op_flags, UCX_DO_READ);


zachdworkin · 2025-10-22T18:44:59Z

libfabric ci is failing udp fabtests. Can you please rebase to pickup the changes from #11501 to fix this issue?

aingerson

Please also take a look at your commits and remove Merge commits

aingerson · 2025-10-27T16:18:34Z

prov/ucx/src/ucx_rma.c

+	if (u_ep->ep.tx_op_flags & FI_COMPLETION)
+		flags |= FI_COMPLETION;


This is still incorrect.

The *msg APIs don't take into account the op_flags. They are specifically to override the op_flags so you shouldn't be using the op_flags on the *msg paths. You should only use the u_ep->tx_msg_flags here which only has FI_COMPLETION for the non FI_SELECTIVE_COMPLETION case and are 0 if FI_SELECTIVE_COMPLETION is being used. You should be able to always OR in the tx_msg_flags for the msg APIs

thanks for explaining. looks it just flags|msg_flags

- Ignore the absence of the FI_COMPLETION flag for fi_writemsg/fi_readmsg if CQ configured without FI_SELECTIVE_COMPLETION flag. - Error completions must generated for all operations, including those for which a completion was not requested Signed-off-by: Sergey Tatarintsev <[email protected]>

aingerson

The other *msg APIs look like they are incorrect as well. Could you fix those while you're here? Thanks!
https://github.com/ofiwg/libfabric/blob/main/prov/ucx/src/ucx_msg.c#L234
https://github.com/ofiwg/libfabric/blob/main/prov/ucx/src/ucx_tagged.c#L114

aingerson · 2025-10-28T13:00:55Z

prov/ucx/src/ucx_rma.c

 {
-	return ucx_proc_rma_msg(ep, msg, flags, UCX_DO_WRITE);
+	struct ucx_ep *u_ep = container_of(ep, struct ucx_ep, ep.ep_fid);
+	return ucx_proc_rma_msg(ep, msg, flags | u_ep->ep.tx_op_flags, UCX_DO_WRITE);


Should be return ucx_proc_rma_msg(ep, msg, flags | u_ep->ep.tx_msg_flags, UCX_DO_WRITE);

aingerson · 2025-10-28T13:02:58Z

prov/ucx/src/ucx_rma.c

 {
-	return ucx_proc_rma_msg(ep, msg, flags, UCX_DO_READ);
+	struct ucx_ep *u_ep = container_of(ep, struct ucx_ep, ep.ep_fid);
+	return ucx_proc_rma_msg(ep, msg, flags | u_ep->ep.rx_op_flags, UCX_DO_READ);


Should be return ucx_proc_rma_msg(ep, msg, flags | u_ep->ep.tx_msg_flags, UCX_DO_READ);
(A read is still a transmitted operation)

j-xiong reviewed Oct 13, 2025

View reviewed changes

tatarintsevsv force-pushed the ucx_selective_completion branch from 9eb350d to 8d6b373 Compare October 16, 2025 04:03

j-xiong reviewed Oct 22, 2025

View reviewed changes

tatarintsevsv force-pushed the ucx_selective_completion branch from 1fae2f8 to 0e0953f Compare October 27, 2025 09:35

aingerson reviewed Oct 27, 2025

View reviewed changes

tatarintsevsv force-pushed the ucx_selective_completion branch from 0e0953f to b37faa7 Compare October 28, 2025 03:11

aingerson reviewed Oct 28, 2025

View reviewed changes

		if (u_ep->ep.tx_op_flags & FI_COMPLETION)
		flags \|= FI_COMPLETION;

Uh oh!

prov/ucx: Fix completion entries insertion for RMA operations. #11493

Are you sure you want to change the base?

prov/ucx: Fix completion entries insertion for RMA operations. #11493

Conversation

tatarintsevsv commented Oct 12, 2025

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!

shijin-aws Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tatarintsevsv commented Oct 14, 2025

Uh oh!

tatarintsevsv commented Oct 22, 2025

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!

zachdworkin commented Oct 22, 2025

Uh oh!

aingerson left a comment

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!

aingerson left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shijin-aws Oct 14, 2025 •

edited

Loading