TL/MLX5: fix memtype in bcast reliability

[MamziB]

TL/MLX5: fix memtype in bcast reliability

[samnordmann]

I understand it is a fix, but I am surprised we didn't catch that bug before. It seems that we are not testing mcast bcast cuda memory at all, is it correct? If so, can you please add a test?

[MamziB]

@samnordmann you are correct, it was a shortcoming from my side. Reliablity protocol kicks when a packet is dropped and it does not happen in normal scenarios.
Previously, I tried forcing the dropped packet in the HOST version for testing purposes, but not for GPU. Since we saw a dropped packet on the ROCK system, where MTT runs, we saw this error, and here is a fix for that.

[samnordmann]

@samnordmann you are correct, it was a shortcoming from my side. Reliablity protocol kicks when a packet is dropped and it does not happen in normal scenarios. Previously, I tried forcing the dropped packet in the HOST version for testing purposes, but not for GPU. Since we saw a dropped packet on the ROCK system, where MTT runs, we saw this error, and here is a fix for that.

Can you please add a test, covering both Host and CUDA memtype, checking that the reliability is not boggy?

[MamziB]

@samnordmann OK I will open a separate PR soon

[janjust]

@MamziB no need for a separate PR, just push the commit here

[MamziB]

@janjust I have added the gtest before but it fails the github ucc tests for some unknown reasons. I rather open a new PR so that it does not affect this PR as I would like this PR to go in ASAP. I will open a PR today for that gtest changes.

[janjust]

I don't think this PR will go in until the test is in, open the other PR, but then this PR will be predicated on the test going in.

[MamziB]

I opened #1023 for testing mcast

[janjust]

great, thanks -now the hard part, figuring out why the other is failing

[janjust]

@samnordmann @Sergei-Lebedev
#1023

Here is the test

[samnordmann]

@samnordmann @Sergei-Lebedev #1023

Here is the test

Thank you, but shouldn't we also test the reliability protocol in case of dropped packets? The present PR fixes a bug that we cannot catch nor reproduce, so how can we ensure that it is effectively fixed?

I don't want to block the PR if there are some time considerations though, so let me know what you guys think

[MamziB]

@samnordmann Sure, I will update the other PR to force the bcast to have dropped packets

[samnordmann]

Thanks!

[MamziB]

@janjust can you please let me know if you have comments on this?

[janjust]

@MamziB not specifically on this PR, but I'm still curious why the other PR, the test is not failing, I haven't investigated it yet.

[MamziB]

@janjust thanks Tommy. Since I am preparing a PR which is gonna be based on this PR, can we merge this PR so that we can proceed?

[MamziB]

@Sergei-Lebedev i rebased it