util/av: Fix handling of double insertion of the same address

[dmitrygx]

This patch fixes problem when the same address is inserted into AV multiple times.

If the AV contains some address and this address is inserted again, the AV has to return already allocated index and don't allocate any additional resources/indices for this address.

Signed-off-by: Dmitry Gladkov dmitry.gladkov@intel.com

[shefty]

I intentionally didn't define the behavior for handling double insertions. I didn't want to force providers to do a linear search through what could potentially be a hardware based AV to see if an address had already been inserted. Some providers may detect duplicates naturally (e.g. UDP), others may simply insert the duplicate address twice.

[dmitrygx]

others may simply insert the duplicate address twice.

The previous version did this, but it produces new connection request to the remote side. It adds overhead and doesn't work correct (e.g. all MPI spawn test fail).

If I understood correctly, this is not actually linear search. This search takes O(1) by using hash key (slot). If the value wasn't found, this does linear search in the collision list.

[shefty]

This implementation doesn't do a linear search. However, some implementations may need to do so in order to prevent a duplicate insertion.

There's no defined behavior for a double insertion. There are several behaviors that are possible. 1. insert the address twice. 2. detect the duplicate and return the old fi_addr. 3. fail the insertion. The existing providers differ in their approaches, which is why no test stresses this.

These all have impacts at the application level depending on the AV type. For example, using AV_TABLE, 1 & 2 produce different results. Option 3 requires the provider to detect duplicates -- that detection may require linear searches.

The best option is for applications not to insert the same address twice at the same time. Does MPI actually need to insert the same address twice?

[dmitrygx]

This patch changes the approach used
from

1. insert the address twice

to

2. detect the duplicate and return the old fi_addr.

And at the same time, this eliminates problem that causes hang when the remote side rejects the connection request for the peer with which connection is already established. Also the returning of the old fi_addr solves the problem of multiple connection to the same peer.

The best option is for applications not to insert the same address twice at the same time. Does MPI actually need to insert the same address twice?

Sure, not to insert the same address twice is the best option. MPI does multiple insertion of the same address only in case of inter-communicators and spawn functionality. There is no possibility to lookup EP name (address) in the AV, i.e. there is no function call that will provide fi_addr based on EP name that MPI retrieves by means of fi_getname.
Yes, MPI could store addresses and establish one-to-one relationship (bijective ) between fi_addr and EP name. And before inserting into the AV, MPI can check whether this address is already inserted or not.
From MPI perspective AV is a black box that can translate EP names into fi_addrs.

[shefty]

Well, we don't want to force apps to maintain a mapping of addresses to fi_addr. Let me take this to the mailing list and see if we can get agreement on the behavior. This will be difficult because the behaviors already differ between providers. Let me see if there's a reasonable solution.

[dmitrygx]

Let me take this to the mailing list and see if we can get agreement on the behavior. This will be difficult because the behaviors already differ between providers. Let me see if there's a reasonable solution.

Sure, I'm waiting for this.

[shefty]

So, I think while we figure out what the API should do, we're fine merging this. This is simply changing unspecified API behavior to some other unspecified behavior, but one that lets MPI work. :)

Can you also open a PR with this against 1.6?

Once we specify what the real behavior should be, we'll need to update the providers and apps accordingly. :/ I suspect this will require adding a new flag to av_attr in order to maintain backward compatibility.

[dmitrygx]

This is simply changing unspecified API behavior to some other unspecified behavior

Hahah, yes :)

Can you also open a PR with this against 1.6?

Sure, I can.

I suspect this will require adding a new flag to av_attr in order to maintain backward compatibility.

Do you mean the flag that can control behavior (insert duplicated addr and generate new fi_addr or don't insert and return old fi_addr)?

[shefty]

Yes, I think we may need to add a flag that when set will define exactly what behavior to expect. But I'm not sure we'll need that, I'm only guessing at this point.

[dmitrygx]

@shefty Can we merge this?