Add the acoll component #12484
Add the acoll component #12484
Conversation
|
How this compares with #10470 ? |
We can discuss it at the meeting. Part of the goal of filing the pr was to give people the ability to have a look at it ahead of the meeting if they want/can. |
ompi/mca/coll/acoll/Makefile.am
Outdated
There was a problem hiding this comment.
Do you have plan to add alltoall(v) to acoll?
There was a problem hiding this comment.
Yes, we are planning to add alltoall to acoll next.
| * chosen, further decides if [ring|lin] allgather is to be used. | ||
| * | ||
| */ | ||
| static inline void coll_allgather_decision_fixed(int size, size_t total_dsize, int sg_size, |
There was a problem hiding this comment.
can you shed some lights on how to choose which methods for other intel/amd achitectures? you might also want some utility to let the user to adjust the decisions according for other systems.
There was a problem hiding this comment.
Our testing has been mostly focused on Zen architectures, we will soon test on other architectures. We do not have the utility/config option to override decisions, we will plan to add it.
ompi/mca/coll/acoll/README
Outdated
| @@ -0,0 +1,15 @@ | |||
| Copyright (c) 2023-2024 Advanced Micro Devices, Inc. All rights | |||
There was a problem hiding this comment.
have you thought about what needs to be done to extend this for multiple nodes?
There was a problem hiding this comment.
Some of the APIs (like bcast, barrier, allgather) support multi-node case. However, it is not extensively tested for multi-node, we will test them and extend other APIs also to multi-node.
| /* | ||
| * rd_allgather_sub | ||
| * | ||
| * Function: Uses recursive doubling based allgather for the group. |
There was a problem hiding this comment.
have you compared the performance of other methods, besides recursive doubling?
There was a problem hiding this comment.
Yes, acoll/allgather chooses among recursive doubling, ring and linear based on process count and message sizes.
| } | ||
|
|
||
| /* This barrier is needed to prevent random hangs */ | ||
| err = ompi_coll_base_barrier_intra_tree(comm, module); |
There was a problem hiding this comment.
Why the barrier is needed here? This barrier will also add cost to small message allgather.
| if (sbuf != MPI_IN_PLACE) | ||
| memcpy(tmp_rbuf, sbuf, my_count_size * dsize); | ||
| } else { | ||
| ompi_3buff_op_reduce(op, (char *) data->xpmem_saddr[0] + chunk * rank * dsize, |
There was a problem hiding this comment.
is the 3 operator reduce function to maintain the order?
There was a problem hiding this comment.
I think this was a bit faster than copying the chunks first and then reducing later in the following "for" loop.
|
Please rebase to current main to get rid of the |
|
I tested the PR in AWS CI. I'm seeing assertion errors with You can try |
|
@amd-nithyavs could you rebase this PR to see if that clears up the mpi4py CI failure? |
@hppritcha we did have issues after rebase. Have fixed the issues, will update the PR soon. Thanks. |
The updated PR (yet to be pushed) will fix this issue. Thanks. |
The issue is fixed in the updated PR. |
We have updated the PR, it passes the mpi4py tests. |
|
Running AWS CI |
|
@amd-nithyavs I noticed that the PR is currently split into 3 commits. Please squash them before merging. |
|
Passed AWS CI. Note that we don't test with xpmem. |
|
Hello! The Git Commit Checker CI bot found a few problems with this PR: 2f7c5e2: Merge latest of local ompiv5
Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks! |
|
@wenduwan We have rebased to the latest and squashed the commits. |
ompi/mca/coll/acoll/LICENSE.md
Outdated
| @@ -0,0 +1,11 @@ | |||
| Copyright (C) 2024, Advanced Micro Devices, Inc. All rights reserved. | |||
There was a problem hiding this comment.
In general, the copyright notice format in ompi is
Copyright (c) YYYY[-YYYY] Entity etc.
ompi/mca/coll/acoll/configure.m4
Outdated
| AC_DEFUN([MCA_ompi_coll_acoll_CONFIG],[ | ||
| AC_CONFIG_FILES([ompi/mca/coll/acoll/Makefile]) | ||
| # ToDo: Check for a proper way to pass args 1 and 2 |
There was a problem hiding this comment.
Could you please explain what this TODO means? Positional arguments can be accesses in shell style, e.g. $1 $2.
There was a problem hiding this comment.
We can remove TODO. We want acoll to build with or without xpmem, this comment was to check if there is a better way to than " OPAL_CHECK_XPMEM([coll_acoll], [should_build=1], [should_build=1])". Passing $1 $2 wouldn't build acoll if xpmem is not present.
| return temp_ptr; | ||
| } | ||
|
|
||
| static inline void coll_acoll_free(coll_acoll_reserve_mem_t *reserve_mem_ptr, void *ptr) |
There was a problem hiding this comment.
IMO the function name could be more specific.
Also I don't quite understand coll_acoll_reserve_mem_t. It looks to me that each instance tracks a single memory allocation. Since coll_acoll_reserve_mem_t.reserve_mem already points to the allocation, why does the caller have to pass in the ptr again?
There was a problem hiding this comment.
We use coll_acoll_reserve_mem_t to track the use of a pre-allocated memory. However, when the requested size during coll_acoll_malloc() is greater than that of the pre-allocated memory, we allocate new memory, which needs to be then freed during coll_acoll_free(). Hence the need to pass ptr.
| } | ||
| } | ||
|
|
||
| static inline int log_sg_bcast_intra(void *buff, int count, struct ompi_datatype_t *datatype, |
There was a problem hiding this comment.
IIUC the *sg_bcast_intra functions are only used in allgather, so should they be moved to allgather file?
I don't have a strong opinion, but it caught my eye that we are doing actual collectives in utils - that's unusual.
There was a problem hiding this comment.
Agree, will move to allgather.
| if ((false == reserve_mem_ptr->reserve_mem_allocate) | ||
| || (false == reserve_mem_ptr->reserve_mem_in_use)) { | ||
| if (NULL != ptr) { | ||
| free(ptr); |
There was a problem hiding this comment.
I think the NULL check should be moved to the top of the function. But please see my comment about coll_acoll_reserve_mem_t - maybe we don't need ptr at all.
There was a problem hiding this comment.
Please see the reply to the previous comment on coll_acoll_reserve_mem_t. Since ptr will be the same as reserve_mem (the pre-allocated memory) when the memory size needed is less than or equal to that of the pre-allocated memory, the null check should be inside to ensure it is the case corresponding to a new buffer.
| ret = -1; | ||
| goto error_hndl; | ||
| } | ||
| sprintf(rc_name, "acoll_%d_%d_%d", cid, rank, i); |
There was a problem hiding this comment.
nit If we care about the name length then we should consider snprintf instead of sprintf
|
|
||
| data->rcache[i] = mca_rcache_base_module_create("grdma", NULL, &rcache_element); | ||
| if (data->rcache[i] == NULL) { | ||
| printf("Error in rcache create\n"); |
There was a problem hiding this comment.
Reminder to clean up printf in this PR.
ompi/mca/coll/acoll/coll_acoll.h
Outdated
| # include "opal/mca/rcache/base/base.h" | ||
| # include <xpmem.h> |
There was a problem hiding this comment.
nit I don't think we need to indent includes.
|
|
||
| END_C_DECLS | ||
|
|
||
| #define MCA_COLL_ACOLL_MAX_CID 100 |
There was a problem hiding this comment.
See my other comment about CID. I'm not sure if it is intended to be used in collectives.
| } | ||
| #endif | ||
|
|
||
| void mca_coll_acoll_barrier(coll_acoll_data_t *data, int offset, int *group, int gp_size, int rank, |
There was a problem hiding this comment.
This function seems to be at the wrong file.
There was a problem hiding this comment.
Will rename the function, this is not a generic MPI barrier function, it is used internallly for small message allreduce only
acoll is a collective component optimized for AMD "Zen"-based processors. It supports Bcast, Allreduce, Reduce, Barrier, Gather and Allgather APIs. Signed-off-by: Nithya V S <Nithya.VS@amd.com>
|
@wenduwan we have addressed the comments and incorporated some big count related changes as well. |
This PR introduces "acoll", a high-performant collective component that is optimized for communications within a single node of AMD EPYC CPUs. It mainly uses subcommunicators based on l3cache or numa to reduce cross-cache or cross-numa accesses. The supported collectives include Bcast, Allreduce, Gather, Reduce, Barrier, Allgather.
OSU micro-benchmarks were run on 2-socket AMD EPYC 9654 96-Core Processor with 4 NUMA domains per socket, with a total of 192 cores per node, on top of commit bb7ecde.
Average percentage latency reduction over "tuned" across 32, 64, 96, 128, 192 ranks over message sizes of 8 bytes to 8 MB (varied in powers of 2):
Sample graphs:
Allreduce
Bcast
Gather
