core + prov/efa: Request page alignment for buffer pools #7431

nzmsv · 2022-02-08T02:52:50Z

This lets us safely use MADV_DONTFORK without worrying about the buffer sharing a page with an unrelated allocation.

We also have to make the allocation size aligned to avoid the same page sharing issue at the end of the buffer.

shijin-aws · 2022-02-12T01:40:13Z

Could you modify the commit/PR title to be prov/efa: ... to be consistent with other PRs/commits. This will help us filter commits. Thanks.

prov/efa/src/rxr/rxr.h

wzamazon

Please use ofi_get_page_size to get page size.

prov/efa/src/rxr/rxr.h

prov/util/src/util_buf.c

prov/efa/src/rxr/rxr_ep.c

Add OFI_BUFPOOL_PAGE_ALIGNED flag that requests page alignment for the entire buffer pool rather than individual entries. This is required to safely use MADV_DONTFORK when registering the buffers with ibv_reg_mr. Signed-off-by: Greg Inozemtsev <ginozemt@amazon.com>

wzamazon

Please split the PR into 2:

1st one make change to prov/utils to introduce the flag OFI_BUFPOOL_PAGE_ALIGNED

2nd one change the efa provider to use the the flag.

wzamazon · 2022-03-16T18:36:48Z

prov/efa/src/rxr/rxr_ep.c

@@ -1399,7 +1399,7 @@ static int rxr_create_pkt_pool(struct rxr_ep *ep, size_t size,
 int rxr_ep_init(struct rxr_ep *ep)
 {
 	size_t entry_sz, sendv_pool_size;
-	int hp_pool_flag;
+	int flags;


rename to pkt_pool_flags is better because it is only apply to packet pools.

wzamazon · 2022-03-16T18:37:37Z

prov/efa/src/rxr/rxr_ep.c

@@ -1409,19 +1409,21 @@ int rxr_ep_init(struct rxr_ep *ep)
 #endif

 	if (efa_fork_status == EFA_FORK_SUPPORT_ON)
-		hp_pool_flag = 0;
+		// Buffer pool needs to be page aligned in order to set MADV_DONTFORK
+		// on the pages making up the pool.


use /* instead of //. Also please elaborate

wzamazon · 2022-03-16T18:40:06Z

prov/efa/src/rxr/rxr_ep.c

@@ -1452,7 +1454,8 @@ int rxr_ep_init(struct rxr_ep *ep)
 		 */
 		ret = rxr_create_pkt_pool(ep, entry_sz,
 					  rxr_env.readcopy_pool_size,
-					  0, &ep->rx_readcopy_pkt_pool);
+					  flags & ~OFI_BUFPOOL_HUGEPAGES,


use pkt_pool_flags to be consistent. I understand it changed behavior to use huge pages when fork support is off, which is ok.

wzamazon · 2022-03-16T21:44:26Z

@shefty

We have identified this PR to fix a segfault when application running with rdma-core's fork support turned on, and so want it to be included in upcoming libfabric releases.

shefty

See comment I'm about to post to the conversation.

shefty · 2022-03-16T21:51:16Z

include/ofi_mem.h

@@ -289,6 +289,7 @@ enum {
 	OFI_BUFPOOL_INDEXED		= 1 << 1,
 	OFI_BUFPOOL_NO_TRACK		= 1 << 2,
 	OFI_BUFPOOL_HUGEPAGES		= 1 << 3,
+	OFI_BUFPOOL_PAGE_ALIGNED	= 1 << 4,


This is not used for page alignment, but to specify that memory allocations need to be a multiple of page sizes. Please separate the changes to the core files from the EFA provider.

shefty · 2022-03-16T21:58:47Z

prov/util/src/util_buf.c

 retry:
 		ret = ofi_memalign((void **) &buf_region->alloc_region,
-				   roundup_power_of_two(pool->attr.alignment),
+				   roundup_power_of_two(alignment),


The above code is confused and mixes data structure alignment with memory allocation sizes. The two are separate.

shefty · 2022-03-16T21:59:34Z

prov/util/src/util_buf.c

@@ -193,6 +204,9 @@ int ofi_bufpool_create_attr(struct ofi_bufpool_attr *attr,
 	if (pool->attr.flags & OFI_BUFPOOL_HUGEPAGES) {
 		pool->alloc_size = ofi_get_aligned_size(pool->alloc_size,
 							hp_size);
+	} else if (pool->attr.flags & OFI_BUFPOOL_PAGE_ALIGNED) {
+		pool->alloc_size = ofi_get_aligned_size(pool->alloc_size,
+							page_sizes[OFI_PAGE_SIZE]);


As mentioned in previous reviews, if the allocation size changes, we need to adjust other pool attributes.

shefty · 2022-03-16T23:01:40Z

The bufpool has these attributes

entry_size /* size of user's object size plus metadata plus alignment */
alignment /* stores alignment needed for each object */
chunk_cnt /* number of entries in each allocation */
alloc_size /* size of the memory allocation */

Note that entry_size can be larger, smaller, or equal to the object's alignment. The chunk_cnt is currently used to calculate alloc_size. What's needed is:

alloc_alignment /* aligned size for each allocation */

Combined, the alloc_size and alloc_alignment can guarantee that allocations are in whole pages, which is the desired feature.

These fields are related to each other. If we change the alloc_size, the chunk_cnt should reflect that change. And introducing alloc_alignment is essentially doing that. alloc_size needs to include alloc_alignment when being set, the same way entry_size includes the object alignment. The attributes should be set in this order:

object size <- size given by user
alignment <- based on object size or specified by user
entry_size <- based on the object size & alignment
alloc_alignment <- based on entry_size or specified by user
chunk_cnt <- specified by user or defaults & adjusted by alloc_alignment & entry_size
alloc_size <- based on entry_size/chunk_cnt & alloc_alignment

The calculation for the chunk_cnt needs to take into consideration any alloc_alignment. For example, assuming no chunk_cnt specified by user, if alloc_alignment is 4k and entry_size is 32, then the chunk_cnt should be 128. The alloc_size would also 4k (aligned allocation) in this case. However, if alloc_alignment is 4k and entry_size is 8k, then chunk_cnt might be 16 with alloc_size = 128k (aligned).

The alloc_alignment likely changes based on whether we're allocating huge pages or not. The code needs to handle this or add an assert that alloc_alignment is unspecified if huge pages are allowed. At the very least alloc_alignment should match object alignment. That is, we should have:

    ret = ofi_memalign(&buf_region->alloc_region, pool->alloc_alignment, pool->alloc_size)

Instead of "roundup_power_of_two(pool->attr.alignment)" filling in where alloc_alignment should be.

nzmsv · 2022-03-17T08:32:53Z

After discussing this with the team internally, we decided to approach this differently. Rather than changing the bufpool alignment we decided to use mmap to satisfy the allocation. I split this PR into two:

core change to add mmap bufpool support: core: Add OFI_BUFPOOL_MMAP flag to bufpool #7547
prov/efa change to make use of the mmap flag: prov/efa: Use mmap to allocate RXR buffer pools nzmsv/libfabric#1

rashikakheria approved these changes Feb 9, 2022

View reviewed changes

wzamazon reviewed Feb 12, 2022

View reviewed changes

prov/efa/src/rxr/rxr.h Outdated Show resolved Hide resolved

wzamazon suggested changes Feb 12, 2022

View reviewed changes

nzmsv force-pushed the ofi_rxr_align branch from 40cb4b5 to 8e163f6 Compare February 16, 2022 18:28

nzmsv requested a review from wzamazon February 16, 2022 18:29

nzmsv force-pushed the ofi_rxr_align branch from 8e163f6 to 8a5eacf Compare February 16, 2022 18:30

shefty reviewed Mar 3, 2022

View reviewed changes

prov/efa/src/rxr/rxr.h Outdated Show resolved Hide resolved

prov/util/src/util_buf.c Outdated Show resolved Hide resolved

shefty changed the title ~~efa: Request page alignment for RXR buffer pool~~ core + prov/efa: Request page alignment for buffer pools Mar 3, 2022

wzamazon added the evaluating label Mar 3, 2022

nzmsv force-pushed the ofi_rxr_align branch from 8a5eacf to e5c08cf Compare March 15, 2022 18:27

nzmsv changed the title ~~core + prov/efa: Request page alignment for buffer pools~~ prov/efa: Request page alignment for buffer pools Mar 15, 2022

wzamazon reviewed Mar 15, 2022

View reviewed changes

prov/efa/src/rxr/rxr_ep.c Outdated Show resolved Hide resolved

wzamazon reviewed Mar 15, 2022

View reviewed changes

prov/efa/src/rxr/rxr_ep.c Outdated Show resolved Hide resolved

nzmsv force-pushed the ofi_rxr_align branch from e5c08cf to bfd147a Compare March 16, 2022 05:08

nzmsv changed the title ~~prov/efa: Request page alignment for buffer pools~~ core + prov/efa: Request page alignment for buffer pools Mar 16, 2022

nzmsv requested a review from wzamazon March 16, 2022 05:28

nzmsv force-pushed the ofi_rxr_align branch from bfd147a to 0564e43 Compare March 16, 2022 05:30

wzamazon suggested changes Mar 16, 2022

View reviewed changes

wzamazon reviewed Mar 16, 2022

View reviewed changes

wzamazon added for-1.15.x for-1.14.x and removed evaluating labels Mar 16, 2022

shefty requested changes Mar 16, 2022

View reviewed changes

This was referenced Mar 17, 2022

core: Add OFI_BUFPOOL_MMAP flag to bufpool #7547

Merged

prov/efa: Use mmap to allocate RXR buffer pools nzmsv/libfabric#1

Closed

nzmsv closed this Mar 17, 2022

nzmsv deleted the ofi_rxr_align branch March 21, 2022 22:22

nzmsv mentioned this pull request Mar 28, 2022

DataLoader crash when using FI_EFA_USE_DEVICE_RDMA=1 aws/aws-ofi-nccl#69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core + prov/efa: Request page alignment for buffer pools #7431

core + prov/efa: Request page alignment for buffer pools #7431

nzmsv commented Feb 8, 2022

shijin-aws commented Feb 12, 2022

wzamazon left a comment

wzamazon left a comment •

edited

Loading

wzamazon Mar 16, 2022

wzamazon Mar 16, 2022

wzamazon Mar 16, 2022

wzamazon commented Mar 16, 2022

shefty left a comment

shefty Mar 16, 2022

shefty Mar 16, 2022

shefty Mar 16, 2022

shefty commented Mar 16, 2022

nzmsv commented Mar 17, 2022

core + prov/efa: Request page alignment for buffer pools #7431

core + prov/efa: Request page alignment for buffer pools #7431

Conversation

nzmsv commented Feb 8, 2022

shijin-aws commented Feb 12, 2022

wzamazon left a comment

Choose a reason for hiding this comment

wzamazon left a comment • edited Loading

Choose a reason for hiding this comment

wzamazon Mar 16, 2022

Choose a reason for hiding this comment

wzamazon Mar 16, 2022

Choose a reason for hiding this comment

wzamazon Mar 16, 2022

Choose a reason for hiding this comment

wzamazon commented Mar 16, 2022

shefty left a comment

Choose a reason for hiding this comment

shefty Mar 16, 2022

Choose a reason for hiding this comment

shefty Mar 16, 2022

Choose a reason for hiding this comment

shefty Mar 16, 2022

Choose a reason for hiding this comment

shefty commented Mar 16, 2022

nzmsv commented Mar 17, 2022

wzamazon left a comment •

edited

Loading