AV and SRX behavior when fi_av_insert calls are interspersed with fi_av_remove calls

[sunkuamzn]

FI_AV_TABLE man page clarification

According to the man page

When FI_AV_TABLE is used, the returned fi_addr_t is an index, with the index for an inserted address the same as its insertion order into the table. The index of the first address inserted into an FI_AV_TABLE will be 0, and successive insertions will be given sequential indices.

The man page does not specify what happens when there is one or more fi_av_remove calls in between the fi_av_insert calls. I did a quick test and the current util AV implementation re-uses fi_addrs of removed peers. I think we should update the man page to clarify what happens when there are fi_av_remove calls and make sure the util AV implementation conforms to the man page

SRX behavior

The current SRX implementation maintains two per peer lists of RX entries: (1) application posted receives and (2) unexpected messages from the peer. Currently, SRX does not have an API to remove a peer. So if an application calls fi_av_remove followed by fi_av_insert and the fi_addr is re-used, the new peer will re-use the per peer lists maintained by SRX which could contain RX entries from a peer that has since been removed.

The man page for fi_av_remove says

The behavior of operations in progress that reference the removed addresses is undefined.

So if the applications calls fi_av_remove with pending fi_recv operations and that corrupts the RX entry list with application posted receives, one can argue that the application should fix its behavior.

But the application has no knowledge of any pending unexpected receives in the SRX queue. The SRX should cleanly handle the case when the application calls fi_av_remove with pending unexpected messages. I think the best way to do that is to introduce an API to remove peers from the SRX

[aingerson]

Man page update:
I chatted with Jianxin and Stephen and we agree that it's fine to reuse fiaddrs on remove and we can (and should!) clarify that in the man pages. I can update the wording and I'll request your review.

As far as the SRX issue, this gets tricky because you're trying to define behavior for something that was deliberately left undefined for this reason. There are a lot of different cases to consider and so trying to define these cases for a use case that the application really shouldn't be doing is very difficult... My first question would be why is the application trying to do this and are you sure it's something you need to support and if so to what extent? The more bullet proof you make this, the more overhead you risk adding to everything else.
That said, it was documented as "undefined" and not "disallowed" so that it could be provider dependent so I think you can still support it for efa through the peer API if you need to. However, defining something in the peer API in order to support undefined behavior doesn't really make sense... So let's walk through all the cases that make this tricky

(1) Unmatched/posted: easy - cancel
(2) Unmatched/unexpected: the list of unmatched unexpected messages lives in the owner space so those messages can get freed by the owner on av_remove. The thing we'll have to handle in this case are any resources that the peer allocated to track the unexpected message. We can use the peer_ops->discard_msg() API to do that to let the peer know to throw this message away.
(3) Matched (expected and unexpected): the owner can keep track of a list of these entries and iterate over them to discard them. Without thinking about that too much, it should work.
Alternatively, (2) and (3) could get triggered through the av_remove call in the peer if the peer tracks these in progress messages. I'm assuming that when the application calls av_remove, you call av_remove on the peer/shm also so that could be the link you're trying to find between the owner and the peer?

Are you concerned at all about messages that come in from old peers after a new one has been inserted in the old one's place? I haven't fully thought out those cases...