FI_COMPLETION meaning is confusing

[goodell]

From fi_endpoint(3):

       FI_COMPLETION : By default, data transfer operations  generate  comple-
       tion  entries into a completion queue after they have successfully com-
       pleted.  Applications can use this bind flag to selectively enable when
       completions  are generated.  If FI_COMPLETION is specified, data trans-
       fer operations will not generate  entries  for  successful  completions
       unless FI_COMPLETION is set as an operational flag for the given opera-
       tion.  FI_COMPLETION must be OR'ed with FI_SEND and/or FI_RECV flags.

       When set the user must determine when a  request  that  does  NOT  have
       FI_COMPLETION  set  has completed indirectly, usually based on the com-
       pletion of a subsequent operation.  Use of this flag may  improve  per-
       formance  by  allowing the provider to avoid writing a completion entry
       for every operation.

       Example: An application can selectively generate  send  completions  by
       using the following general approach:

                fi_tx_attr::op_flags = 0; // default - no completion
                fi_ep_bind(ep, cq, FI_SEND | FI_COMPLETION);
                fi_send(ep, ...);                   // no completion
                fi_sendv(ep, ...);                  // no completion
                fi_sendmsg(ep, ..., FI_COMPLETION); // completion!

       Example:  An  application  can  selectively disable send completions by
       modifying the operational flags:

                fi_tx_attr::op_flags = FI_COMPLETION; // default - completion
                fi_ep_bind(ep, cq, FI_SEND | FI_COMPLETION);
                fi_send(ep, ...);       // completion
                fi_sendv(ep, ...);      // completion
                fi_sendmsg(ep, ..., 0); // no completion!

It's confusing to me that FI_COMPLETION is used as both a bind flag and an operation flag. This dual usage might not be so confusing other than it also has the opposite meaning in the two uses! If I specify it as a bind flag then it means "do not generate completions for send+sendv". If I specify it as either a default op_flags value or as a flag value to fi_sendmsg then it means "do generate completions".

It might be clearer to have two flags: FI_COMPLETION and FI_NO_COMPLETION (or FI_SUPPRESS_COMPLETION), where the former can only be passed as an operation flag and the latter can only be passed as a bind flag.

@shefty are we too late to improve this aspect of the API?

[shefty]

Let me think about this. We could introduce a new flag name with the same value (to conserve flag space) and update the documentation to reference the new flag name. I'm not sure of the new name. FI_SELECTIVE_COMPLETION?

[goodell]

Let me think about this. We could introduce a new flag name with the same value (to conserve flag space) and update the documentation to reference the new flag name.

That would be a good start. Though I'm a little concerned that a nontrivial number of applications/tests will not be appropriately updated to use the new flag name as long as the old value will have the same behavior.

I'm not sure of the new name. FI_SELECTIVE_COMPLETION?

That works for me, it's better than my suggestions.

[patrickmacarthur]

@goodell @shefty Just to throw my two cents in, I agree with this proposal. It's a confusing part of the API maybe not so much from the user's perspective but definitely from the provider's perspective (I had to really think hard about it when I was adding this support to the verbs provider).

FI_SELECTIVE_COMPLETION makes sense as a name.

[patrickmacarthur]

OTOH I also agree that it may be difficult to get all examples/tests updated in time for 1.0 unless we break API by changing the value of the new FI_SELECTIVE_COMPLETION flag, which we may not want to do this close to release.

[shefty]

Part of the intent of using the same value is so we don't break the apps. We're just creating an alias, so that the usage becomes a little clearer.

[jsquyres]

It might be worth the pain now to get this Right, however, and not be locked into backwards compatibility with 2 names with the same value (which is arguably less safe than 2 names with different values).

[shefty]

Well, I'm not sure that a name change is 'right'. Dave found it confusing. I originally thought that having the same name made is easy to see the link between the two. If you bind with FI_COMPLETION, you must use FI_COMPLETION as an op flag...

[jsquyres]

Yes, but having the same name meaning opposite things is confusing... (i.e., I agree with Dave :-) )

[pmmccorm]

What @jsquyres said: whatever pain we go through now from API changes is a tenth of what we will experience from leaving a confusing API.

[shefty]

We seem to agree with introducing a new flag name. The only concern is whether it should be defined as (1 << 24), which matches FI_COMPLETION, or (1 << 59) -- which ends up matching other flag values that are used in other operations. Regardless this change must go in before rc6. And I'd strongly prefer that all API changes be in before rc6, so we can have at least 1 rc that doesn't change the API. :)

[goodell]

The only concern is whether it should be defined as (1 << 24), which matches FI_COMPLETION, or (1 << 59) -- which ends up matching other flag values that are used in other operations.

I still prefer them to have separate values, which will make catching user errors easier. Right now is our best opportunity to fix it as cleanly as possible.

Regardless this change must go in before rc6. And I'd strongly prefer that all API changes be in before rc6, so we can have at least 1 rc that doesn't change the API. :)

Agreed.

If we can agree on a solution, I can code up the PRs to change everything in libfabric and fabtests today.

[shefty]

If no one else disagrees, I say go for it. fabtests doesn't use this flag anyway. Only the providers and any external apps would need to be updated. And external apps need updating for rc6 anyway.

[goodell]

We could still use a new test, per ofiwg/fabtests#245, but I'd say this specific issue is resolved now. Thanks everyone for the feedback.