UCT/MM: Fix the FIFO room calculation for tail > head

[arun-chandran-edarath]

What?

Fix for #10761

[yosefe]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 5 pipeline(s).

[yosefe]

/azp run perf

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[arun-chandran-edarath]

@yosefe

seems ignoring the ARM bit and casting the difference to integer and then doing the comparison is performing better. New macro is given below

#define FIX2_UCT_MM_EP_IS_ABLE_TO_SEND(_head, _tail, _fifo_size)
ucs_likely(((int64_t)(((_head) & ~UCT_MM_IFACE_FIFO_HEAD_EVENT_ARMED) - (_tail))) < (int64_t)(_fifo_size))

The attached test.c.txt compares the performance of the macro as well as the correctness please check.

I will upload a new diff.

[
test.c.txt
](url)

[jakemoroni]

Want me to give this a try on my setup?

[arun-chandran-edarath]

Want me to give this a try on my setup?
@jakemoroni
Yes, please! I would appreciate it.

[jakemoroni]

This seems to fix the issue for me 👍

It is still unclear to me how the tail can end up greater than the head without any wraparound occurring, but it does work. Thanks!

cc @hjelmn

[arun-chandran-edarath]

This seems to fix the issue for me 👍

It is still unclear to me how the tail can end up greater than the head without any wraparound occurring, but it does work. Thanks!

cc @hjelmn

@jakemoroni Thank you so much for the verification.

I have explained in this reply #10761 (comment) about, how the sender's view of receiver_tail can overtake the sender's view of receiver_head (tail from receiver's view becomes bigger than head only in case of wraparound. head and tail are variables in the receiver and shared to every other rank(sender) via shared memory). Please have a look at the provided code comments.

[hjelmn]

Can you squish this down the minimal number of commits? I can take a look and review once that is complete.

I ask because this seems like it fixes another commit on this PR: "TEST/MM: Fix compile error"

[arun-chandran-edarath]

I have squashed the commits, please take a look.

[hjelmn]

LGTM. Scanning through the code I see UCT_MM_IFACE_FIFO_HEAD_EVENT_ARMED is only ever used with the head so the original math was definitely wrong. We also tested the fix on one of our systems and it fixes the issue.

[hjelmn]

@yosefe Please merge.

[tvegas1]

we can specify in comment that this code cannot work in case of wrap around (as it originally was). Or maybe that only case where head < tail is when head is stale and in this case we intend to return 0?

[tvegas1]

To summarize my understanding for later referral.

With two writers A and B, A is slow, B is very fast:

1. A reads head from ctl area, checks if it is able to send.
2. In the meantime, B performs same check and acquires head (CAS, A has now a stale head), writes.
3. The reader even has time to consume and update tail in ctl area.
4. Step 2. and 3. can repeat.
5. Now A is still in the checks, updates cached_tail and compares it again with its stale head , and finds head < cached_tail.

So head can indeed be observed as being below cached_tail, and the checking code must return 0 in that case. After #10485, this was no longer the case.

[yosefe]

minor: add newline at end of file