prov/shm, prov/efa: Allow more than 256 shm peers by jiaxiyan · Pull Request #13180 · ofiwg/libfabric

jiaxiyan · 2026-11-09T20:22:38Z

shm supports at most 256 peers per AV, fixed at compile time. On instances with high core counts, a job with more than 256 processes per host fails: the 257th local peer fails fi_av_insert, and MPI aborts in MPI_Init:

efa_rdm_av_entry_insert_shm_av(): Max number of shm AV entry (256) has been reached.
efa_rdm_av_entry_alloc_explicit(): Failed to insert fi_addr 256 into shm provider's AV: Cannot allocate memory

This series keeps 256 as the default and lets the limit be raised at runtime, without changing memory use at the default.

prov/shm: Size the SAR pool independently of SMR_MAX_PEERS. The SAR pool stays at 256 buffers regardless of the peer limit, and the per-peer SAR quota is clamped to at least 1 so it can't drop to 0 when peers outnumber SAR buffers.
prov/shm: Let the AV count raise the peer limit. An AV opened with fi_av_attr.count above 256 raises its limit to that count, up to 4096. Smaller counts keep the default. The peer map and each endpoint's peer data are sized from the limit.
prov/efa: Allow raising FI_EFA_SHM_AV_SIZE above 256. FI_EFA_SHM_AV_SIZE (default 256) can now be set up to 4096.

shijin-aws · 2026-11-10T01:03:20Z

 					       av_entry);
        		smr_ep = container_of(util_ep, struct smr_ep, util_ep);
 			smr_ep->region->max_sar_buf_per_peer =
-				MIN(SMR_BUF_BATCH_MAX,


Are we convinced that the SAR pool size not changed when num peers goes up will NOT cause performance issue? I think Intel treats SAR as a fast path which works with DSA

zachdworkin

Thanks for working on this! The SAR pool decoupling looks right to me but I have a
concern with the overall approach.

Tying capacity to fi_av_attr.count makes a hint into a hard limit, which is not how other providers treat it. It also means each process can end up with a different max_peers, so it has to be threaded through smr_attr and smr_calculate_size_offsets.

My suggestion: use one node-wide capacity instead of deriving it from the AV. It
could be a constant, or computed once at init, e.g. max(256, SMR_PROCS_PER_CORE * online_cores). (Default SMR_PROCS_PER_CORE to 1)
All processes on the node then lay out regions identically and the AV count stays a hint. Store max_peers it in the region header and we will have to bump SMR_VERSION so a mismatch is detectable. If we are bumping the version we should try to merge this change with the other one to avoid bumping the version too many times. (I am working on a new fallback protocol for #13084 and will open a PR soon).

To keep the cost proportional to peers actually used we could also make all-zero
mean free for smr_peer_data. The region is sized with ftruncate, so
untouched pages aren't committed, but the init loop in smr_create writes every
row. Dropping it would let a region pay only for the rows it uses. (I havent measured the performance for doing this)

Regarding the SAR quota issue Shi raised: the batch size is 256 / AV size, set by how many peers are in the AV, not by how many are active. I measured it with CMA off and a single active sender: bandwidth is flat up to 16 AV entries, then drops about 36-44% at 256 entries (batch of 1). A CMA-on control stays flat, so it is the SAR batch, not the AV, that costs the bandwidth. This already exists for AVs of 256 or fewer, but this change extends it.

Another issue with this change is the resources that the increased number of peers will compete for stays the same. None of the other pools are dynamically changing based on number of peers. They are all based on tx and rx sizes at init/create time. Senders may get FI_EAGAIN errors more often when trying to send due to resource constraints because of this. If you need the functionality today we can keep this change while working towards a more broad architectural solution to address this.

Closing notes: There is no provider-independent fabtest for shm AV capacity. av_test is in shm.exclude, and the other shm tests use two processes, so the new behavior would be untested. Could you add a test, or enable the relevant av_test cases for shm, that inserts more than 256 addresses and checks the limit? The EFA gtests don't cover shm itself.

zachdworkin · 2026-11-10T14:33:31Z

You missed this one. smr_av->used < smr_av->smr_map.max_peers

Thanks for reviewing it! I have updated it to use your suggested approach and also added a new fabtest

zachdworkin · 2026-11-10T16:52:36Z

May fix #11671 (partially)

a-szegel · 2026-11-10T23:22:17Z


 			int			pid;
-			int			resv2;
+			int			max_peers;


I don't think we need the entire int to be max_peers. Do we want to make uint16_t max_peers and keep the rest of the int as resv2. We have to be careful here b/c int is not a fixed size type.

The SAR pool size was tied to SMR_MAX_PEERS, so raising the peer limit would grow every region by 32 KiB per peer. Give the pool its own size. The per-peer SAR quota becomes 0 once peers outnumber SAR buffers, which blocks SAR for every peer. Clamp it to at least 1, and don't start a SAR transfer when the pool is empty. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>

shm is limited to 256 peers per AV, which is not enough for instances with high core counts. Compute one node-wide limit at init, max(256, SMR_PROCS_PER_CORE * online cores), so every process on the node lays out its region the same way and the AV count stays a hint. Store the limit in the region header and bump SMR_VERSION so peers with a different layout are rejected. Also reset shm_id on every iteration of smr_av_insert so a failed insert does not remove the previous peer. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>

FI_EFA_SHM_AV_SIZE defaulted to 256, so jobs on instances with high core counts failed at the 257th local peer. Default it to the number of online CPUs, with a minimum of 256, to match shm's peer limit. Setting the variable still overrides the default. shm now rejects AV counts above its limit, so drop EFA's own maximum and warn when the shm AV cannot be opened. Add gtests for the default, the limit, and an AV open above shm's limit. The cmocka harness reset efa_env to a copy taken before provider init, which no longer holds the shm AV size. Take the copy after init. Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>

zachdworkin

Thank you for adding the fixes I asked for. Most of my additional comments are about the new test. I am still concerned about the point Shi brought up about the SAR batch size per peer. I think it is okay for now to get the functionality working for you guys, but like I said in my previous review a bigger re-architecture for how that works with more peers is going to be required for completeness of this feature. If you want to work together on that in the future please let me know!

zachdworkin · 2026-11-10T23:34:51Z

+	return ret;
+}
+
+static int parse_count(const char *arg, int *count)


is this really necessary? Can you use something like atoi?

zachdworkin · 2026-11-10T23:49:56Z

+	if (len < 0 || len >= (int) sizeof(addr))
+		return -FI_ETOOSMALL;
+
+	*fi_addr = FI_ADDR_NOTAVAIL;


I think we should probably set it to something else here because this is what the provider is expected to set the fi_addr to if it fails. This way we can distinguish if the test set it to something vs the provider setting it to something

zachdworkin · 2026-11-11T00:03:34Z

+		goto close;
+	}
+
+	if (ret > 0 || fi_addrs[inserted] != FI_ADDR_NOTAVAIL) {


What is the purpose of this check? I can't really find a case where we get to here without having catastrophically failed earlier (like inserted 2 addresses on the last iteration of the for loop incurring a segfault) or having skipped it and gone to close on line 90.

zachdworkin · 2026-11-11T00:04:08Z

+
+	if (inserted == max_addrs) {
+		ret = 0;
+		goto close;


I think this goto close is making it so the additional remove and insert one logic always gets skipped. Is that intended?

zachdworkin · 2026-11-11T00:04:26Z

+{
+	ft_unit_usage(name, "Unit test for AV capacity");
+	FT_PRINT_OPTS_USAGE("-n <num>",
+			    "minimum number of addresses the AV must accept (default 256)");


Why is the default minimum 256?

Insert addresses into an AV until it is full, check that it accepts at least a given number, rejects the next insert, and accepts an insert again after a remove. Run it for shm with the minimum set to max(256, online CPUs). Signed-off-by: Jessie Yang <jiaxiyan@amazon.com>

jiaxiyan force-pushed the shm-max-peers-1024 branch from 4ac91d9 to a15ad0f Compare November 9, 2026 20:36

jiaxiyan requested review from a team, j-xiong and zachdworkin November 9, 2026 20:39

a-szegel reviewed Nov 9, 2026

View reviewed changes

Comment thread prov/shm/src/smr_av.c Resolved

Comment thread prov/shm/src/smr.h Resolved

jiaxiyan force-pushed the shm-max-peers-1024 branch from a15ad0f to 34228cf Compare November 9, 2026 23:41

jiaxiyan requested a review from shijin-aws November 10, 2026 00:40

a-szegel previously approved these changes Nov 10, 2026

View reviewed changes

shijin-aws reviewed Nov 10, 2026

View reviewed changes

j-xiong previously approved these changes Nov 10, 2026

View reviewed changes

zachdworkin requested changes Nov 10, 2026

View reviewed changes

jiaxiyan dismissed stale reviews from j-xiong and a-szegel via a711556 November 10, 2026 20:29

jiaxiyan force-pushed the shm-max-peers-1024 branch from 34228cf to a711556 Compare November 10, 2026 20:29

jiaxiyan requested review from a-szegel, j-xiong, shijin-aws and zachdworkin November 10, 2026 21:53

jiaxiyan force-pushed the shm-max-peers-1024 branch from a711556 to dfdb165 Compare November 10, 2026 22:17

shijin-aws previously approved these changes Nov 10, 2026

View reviewed changes

a-szegel reviewed Nov 10, 2026

View reviewed changes

jiaxiyan added 3 commits November 10, 2026 15:59

jiaxiyan dismissed shijin-aws’s stale review via c89cc05 November 11, 2026 00:10

jiaxiyan force-pushed the shm-max-peers-1024 branch from dfdb165 to c89cc05 Compare November 11, 2026 00:10

zachdworkin requested changes Nov 11, 2026

View reviewed changes

jiaxiyan force-pushed the shm-max-peers-1024 branch from c89cc05 to 2d2f6b0 Compare November 11, 2026 00:57

Conversation

jiaxiyan commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shijin-aws Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachdworkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachdworkin commented Nov 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachdworkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jiaxiyan commented Nov 9, 2026 •

edited

Loading

shijin-aws Nov 10, 2026 •

edited

Loading