COLL/UCC: add persistent collective calls for UCC #13374

mentOS31 · 2025-08-22T05:19:04Z

What
In addition to the existing blocking and non-blocking collective calls,
this PR adds persistent collective call in COLL/UCC.
For example,

mpirun \
  -mca coll_ucc_enable 1 -mca coll_ucc_priority 100 \
  -mca coll_ucc_cts reduce_init \
  osu_reduce_persistent

How
This adds <coll>_init function for each collective operation,
using UCC_COLL_ARGS_FLAG_PERSISTENT flag [1].

Reference
[1] "Unified Collective Communications (UCC) Specification Version 1.0" (2022/02/18)
https://openucx.github.io/ucc/api/v1.0/pdf/ucc.pdf
- UCC_COLL_ARGS_FLAG_PERSISTENT in Section 8.8.4.2 "ucc_coll_args_flags_t"

Signed-off-by: hasegawa.kento <[email protected]>

bosilca

I have few nitpick comments for the entire commit:

'iniz' has little meaning. How about init_common instead ?
'a' as a siffix has no meaning while ' alias' does !
in the original code when the call to the UCC collective _init was made it was multiline with clear separation between send and receive argument. Please maintain that structure, and add the persistent bool flag in front of the ucc_module.

ompi/mca/coll/ucc/coll_ucc_module.c

Sergei-Lebedev · 2025-08-25T05:35:55Z

ompi/mca/coll/ucc/coll_ucc.h

@@ -61,6 +62,7 @@ struct mca_coll_ucc_component_t {
    ucc_lib_attr_t                  ucc_lib_attr;
    ucc_coll_type_t                 cts_requested;
    ucc_coll_type_t                 nb_cts_requested;
+    ucc_coll_type_t                 pc_cts_requested;


does pc stand for persistent collective? maybe ps_cts_requested is better

I agree that ps_cts_requested is better. I changed pc_cts_requested to ps_cts_requested.

Sergei-Lebedev · 2025-08-25T05:36:26Z

ompi/mca/coll/ucc/coll_ucc.h

@@ -132,6 +134,34 @@ struct mca_coll_ucc_module_t {
    mca_coll_base_module_t*                         previous_scatter_module;
    mca_coll_base_module_iscatter_fn_t              previous_iscatter;
    mca_coll_base_module_t*                         previous_iscatter_module;
+    mca_coll_base_module_allreduce_init_fn_t previous_allreduce_init;


please keep alignment

I've adjusted the alignment

Sergei-Lebedev · 2025-08-25T05:45:30Z

ompi/mca/coll/ucc/coll_ucc_allgatherv.c

@@ -57,6 +57,10 @@ static inline ucc_status_t mca_coll_ucc_allgatherv_init(const void *sbuf, size_t
        }
    };

+    if (true == persistent) {


maybe make it part of line 37 where other flags are set?

I changed persistent flag to be set at line 37

Sergei-Lebedev · 2025-08-25T06:32:44Z

ompi/mca/coll/ucc/coll_ucc_component.c

+
+    if ((bz > 0) && (bp != 0)) {
+        bp[0] = '\0';
+    }


does it make sense to continue this function if bp/bz is null?

I removed it.

Sergei-Lebedev · 2025-08-25T06:33:28Z

ompi/mca/coll/ucc/coll_ucc_component.c

+        if (0 == strcmp(cp_suffix, "_init")) {
+            if ((bz > 0) && (bp != 0)) {
+                if (blen >= bz) {
+                    return 0 /* XXX internal error */;


return -1 to separate error from other possible results

I changed to return -1

Sergei-Lebedev · 2025-08-25T06:34:34Z

ompi/mca/coll/ucc/coll_ucc_module.c

@@ -87,6 +88,34 @@ static void mca_coll_ucc_module_clear(mca_coll_ucc_module_t *ucc_module)
    ucc_module->previous_scatter_module               = NULL;
    ucc_module->previous_iscatter                     = NULL;
    ucc_module->previous_iscatter_module              = NULL;
+    ucc_module->previous_allreduce_init = NULL;


please keep alignment

I've adjusted the alignment

Sergei-Lebedev · 2025-08-25T06:47:30Z

ompi/mca/coll/ucc/coll_ucc_component.c

@@ -157,18 +215,33 @@ static void mca_coll_ucc_init_default_cts(void)
    n_cts                = opal_argv_count(cts);
    cm->cts_requested    = disable ? COLL_UCC_CTS : 0;
    cm->nb_cts_requested = disable ? COLL_UCC_CTS : 0;
+    cm->pc_cts_requested = 0; /* XXX PC currently disabled by default */


not sure how it supposed to work, maybe I missing something. It seems like disable mode doesn't work for persistent colls as for other. If user gives ^allreduce_init it means enable every persistent collective except allreduce, however since cts_requested is 0 initialy it won't change anything

I changed it so that ^allreduce_init works correctly.

Sergei-Lebedev · 2025-08-25T06:50:13Z

ompi/mca/coll/ucc/coll_ucc_module.c

@@ -530,11 +565,32 @@ mca_coll_ucc_module_disable(mca_coll_base_module_t *module,
    UCC_UNINSTALL_COLL_API(comm, ucc_module, reduce);
    UCC_UNINSTALL_COLL_API(comm, ucc_module, ireduce);
    UCC_UNINSTALL_COLL_API(comm, ucc_module, gather);
+    /* UCC_UNINSTALL_COLL_API(comm, ucc_module, igather); */


why is it commented out?

This was originally missing statement, but I've added a comment as I'm unsure if it's a bug.
I uncommented for missing statement.

Sergei-Lebedev · 2025-08-25T06:58:19Z

ompi/mca/coll/ucc/coll_ucc_module.c

+        if (true != coll_req->super.req_persistent) {
+            continue;
+        }


isn't it an error?

Yes, I added error handling

Signed-off-by: hasegawa.kento <[email protected]>

bosilca

One minor consistency issue, but otherwise this is ready for merge.

ompi/mca/coll/ucc/coll_ucc_scatterv.c

Signed-off-by: hasegawa.kento <[email protected]>

hasegawa.kento added 3 commits August 21, 2025 14:33

COLL/UCC: renaming <coll>_init to avoid naming conflicts

2dec57d

Signed-off-by: hasegawa.kento <[email protected]>

COLL/UCC: add persistent collective calls

9305110

Signed-off-by: hasegawa.kento <[email protected]>

COLL/UCC: add <coll>_init to -mca coll_ucc_cts parser

4171fdb

Signed-off-by: hasegawa.kento <[email protected]>

github-actions bot added the Target: main label Aug 22, 2025

janjust requested review from Sergei-Lebedev and bosilca August 22, 2025 13:22

bosilca reviewed Aug 22, 2025

View reviewed changes

ompi/mca/coll/ucc/coll_ucc_module.c Outdated Show resolved Hide resolved

bosilca reviewed Aug 22, 2025

View reviewed changes

ompi/mca/coll/ucc/coll_ucc_module.c Show resolved Hide resolved

Sergei-Lebedev reviewed Aug 25, 2025

View reviewed changes

COLL/UCC: fix coding style and error handling

1b4bf91

Signed-off-by: hasegawa.kento <[email protected]>

mentOS31 requested review from bosilca and Sergei-Lebedev September 2, 2025 01:12

bosilca previously approved these changes Sep 2, 2025

View reviewed changes

ompi/mca/coll/ucc/coll_ucc_scatterv.c Show resolved Hide resolved

COLL/UCC: Standardize flag and mask value assignment

a8b244e

Signed-off-by: hasegawa.kento <[email protected]>

mentOS31 dismissed bosilca’s stale review via a8b244e September 5, 2025 05:40

mentOS31 requested a review from bosilca September 5, 2025 05:57

bosilca approved these changes Sep 5, 2025

View reviewed changes

COLL/UCC: add persistent collective calls for UCC #13374

Are you sure you want to change the base?

COLL/UCC: add persistent collective calls for UCC #13374

Uh oh!

Conversation

mentOS31 commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bosilca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mentOS31 Sep 1, 2025 • 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!

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!

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!

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!

mentOS31 Sep 1, 2025 • 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!

bosilca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mentOS31 commented Aug 22, 2025 •

edited

Loading

mentOS31 Sep 1, 2025 •

edited

Loading

mentOS31 Sep 1, 2025 •

edited

Loading