UCT/IB/DC: dynamic allocation of dci pools by configuration #9665

roiedanino · 2024-02-06T15:46:37Z

What

Instead of creating all dci pools (and dcis) on iface init, dci pools will be created dynamically on demand by configuration

Why ?

As part of the active messages priority feature - creating different pools with different SLs is required, enabling dedicated dci pools for different lanes with different priorities, as we can't change SL of a dci (qp) after creation / connection.

How ?

For each new dc ep created - a matching dci pool will be created unless a dci pool with the same exact configuration already exists in the hash table, in which the configuration is the key and the value is pool_index, so eps which share configuration will also share a dci pool.

gleon99 · 2024-02-12T07:07:58Z

src/uct/ib/dc/dc_mlx5.c

@@ -162,6 +162,9 @@ uct_dc_mlx5_ep_create_connected(const uct_ep_params_t *params, uct_ep_h* ep_p)
 {
 uct_dc_mlx5_iface_t *iface = ucs_derived_of(params->iface,
 uct_dc_mlx5_iface_t);
+ uint8_t sl = iface->super.super.super.config.sl;


gleon99 · 2024-02-12T07:31:59Z

src/uct/ib/dc/dc_mlx5_ep.h

+ uint8_t *dci_index_p);
+
+static UCS_F_ALWAYS_INLINE void
+uct_dc_mlx5_init_dci_config_key(uct_dc_mlx5_dci_config_t *dci_config,


Minor: func args & init order better match struct members decl order.

gleon99 · 2024-02-12T07:40:15Z

src/uct/ib/dc/dc_mlx5.c

- uint8_t dci_index,
- uint8_t path_index,
- int full_handshake)
+ucs_status_t uct_dc_mlx5_iface_create_dci(uct_dc_mlx5_iface_t *iface,


Why removed static? Seems it is not used outside

gleon99 · 2024-02-12T07:42:16Z

src/uct/ib/dc/dc_mlx5.c

@@ -761,7 +767,7 @@ static void uct_dc_mlx5_iface_dcis_destroy(uct_dc_mlx5_iface_t *iface,
 ucs_assert(num_dci_pools <= iface->tx.num_dci_pools);
 ucs_assert(num_dcis <= uct_dc_mlx5_iface_total_ndci(iface));

- for (dci_index = 0; dci_index < num_dcis; dci_index++) {
+ for (dci_index = 0; dci_index < iface->tx.dci_counter; dci_index++) {


Seems num_dcis is not used in this func anymore, except the assert. But the assert by itself has no meaning if the var is not used elsewhere..

gleon99 · 2024-02-12T07:42:50Z

src/uct/ib/dc/dc_mlx5.c

+ const uct_dc_mlx5_dci_config_t *config,
+ uint8_t pool_size, uint8_t *pool_index_p)
+{
+ uint8_t full_handshake = iface->flags &


const where possible

gleon99 · 2024-02-12T09:40:09Z

src/uct/ib/dc/dc_mlx5.c

+}
+
+/**
+ * Checks wether dci pool config present in the dc_config_hash and then either returning


Checks whether dci pool config is present in dc_config_hash and returns the matching pool index or creates a new one

Move docs to .h

gleon99 · 2024-02-12T09:45:20Z

src/uct/ib/dc/dc_mlx5.c

+ status = uct_dc_mlx5_iface_create_dci_pool(iface, dci_config, pool_size,
+ pool_index_p);
+ if (status != UCS_OK) {
+ return status;


gleon99 · 2024-02-12T09:54:38Z

src/uct/ib/dc/dc_mlx5.c


 iface->flags |= UCT_DC_MLX5_IFACE_FLAG_KEEPALIVE;
- iface->keepalive_dci = dci_index;
+ iface->keepalive_dci = iface->tx.dci_counter - 1;


Can this place be reached with dci_counter 0?

No, in the current state of the PR a pool is being created with a single dci so dc_counter is at least 1.

gleon99 · 2024-02-12T09:57:57Z

src/uct/ib/dc/dc_mlx5.c

+ dci_pool->release_stack_top = -1;
+
+ for (i = 0; i < pool_size; ++i) {
+ status = uct_dc_mlx5_iface_create_dci(


We discussed to do "lazy" creation..

Yes, it's in progress I will add WIP-DNM label

gleon99 · 2024-02-12T11:48:43Z

@roiedanino maybe add some tests?

…tion

src/uct/ib/dc/dc_mlx5.h

…config key

Conflicts: src/uct/ib/dc/dc_mlx5.c src/uct/ib/dc/dc_mlx5.h src/uct/ib/dc/dc_mlx5_ep.c

yosefe · 2024-05-05T10:20:15Z

src/uct/ib/dc/dc_mlx5.c

@@ -149,15 +149,17 @@ ucs_config_field_t uct_dc_mlx5_iface_config_table[] = {
 static ucs_status_t
 uct_dc_mlx5_ep_create_connected(const uct_ep_params_t *params, uct_ep_h* ep_p)
 {
- uct_dc_mlx5_iface_t *iface = ucs_derived_of(params->iface,
- uct_dc_mlx5_iface_t);
+ uct_dc_mlx5_iface_t *iface = ucs_derived_of(params->iface,


yosefe · 2024-05-05T10:26:06Z

src/uct/ib/dc/dc_mlx5.c

+ uct_ib_mlx5_qp_mmio_cleanup(&dci->txwq.super, dci->txwq.reg);
+
+ uct_dc_mlx5_dci_set_invalid(dci);
+ dci = NULL;


why needed?

To avoid valgrind issues, maybe it's not needed

yosefe · 2024-05-05T10:28:58Z

src/uct/ib/dc/dc_mlx5.c

- dci_pool->stack[i] = dci_index;
- ++dci_index;
- }
+ ucs_assert_always(pool_index < UCT_DC_MLX5_IFACE_MAX_DCI_POOLS);


do we still need dci_counter? can we use array length instead?

We need to know what will be the next dci_index somehow

yosefe · 2024-05-05T10:38:14Z

src/uct/ib/dc/dc_mlx5.c

+ (config->key.flags & UCT_DC_MLX5_DCI_CONFIG_MAX_RD_ATOMIC_IS_64) ?
+ 64 :
+ 16,


to simplify, maybe max_rd_atomic can be just a uint8_t value in the key (not a flag) ?
then no need for this wrapper function

yosefe · 2024-05-05T10:38:24Z

src/uct/ib/dc/dc_mlx5.c

+ uint8_t pool_size = //(config->key.flags & UCT_DC_MLX5_DCI_CONFIG_KEEPALIVE) ?
+ // UCT_DC_MLX5_KEEPALIVE_NUM_DCIS :


yosefe · 2024-05-05T11:37:19Z

src/uct/ib/dc/dc_mlx5_ep.h

+ return status;
+ }
+
+ stack_ptr = ucs_array_append(&pool->stack, return UCS_ERR_NO_MEMORY);


stack_ptr var is reundant

yosefe · 2024-05-05T11:37:28Z

src/uct/ib/dc/dc_mlx5_ep.h

+
+ stack_ptr = ucs_array_append(&pool->stack, return UCS_ERR_NO_MEMORY);
+ *stack_ptr = dci_index;
+


remove space line

yosefe · 2024-05-05T11:38:58Z

src/uct/ib/dc/dc_mlx5_ep.h

- ep->dci_channel_index = iface->tx.dcis[ep->dci].next_channel_index++ %
- iface->tx.num_dci_channels;
+ ep->dci_channel_index =
+ ucs_array_elem(&iface->tx.dcis, ep->dci).next_channel_index++ %


helper var for dci object

yosefe · 2024-05-05T11:39:15Z

src/uct/ib/dc/dc_mlx5_ep.h

@@ -331,9 +392,22 @@ uct_dc_mlx5_ep_basic_init(uct_dc_mlx5_iface_t *iface, uct_dc_mlx5_ep_t *ep)
 static UCS_F_ALWAYS_INLINE int
 uct_dc_mlx5_iface_dci_can_alloc(uct_dc_mlx5_iface_t *iface, uint8_t pool_index)
 {
+ ucs_assert_always(pool_index < iface->tx.num_dci_pools);


adds overhead to fast path

yosefe · 2024-05-05T11:40:04Z

src/uct/ib/dc/dc_mlx5_ep.h


 uct_dc_mlx5_iface_dci_schedule_release(iface, dci_index);

 return 1;
 }

+static inline ucs_status_t uct_dc_mlx5_iface_dci_alloc(uct_dc_mlx5_iface_t *iface, uct_dc_mlx5_ep_t *ep)


why needed to move this function?

why needed to add return value to it?

… cancel

…uled

Conflicts: src/uct/ib/dc/dc_mlx5.c

roiedanino added the WIP-DNM Work in progress / Do not review label Feb 6, 2024

roiedanino self-assigned this Feb 6, 2024

UCT/IB/DC: dynamic creation of dci pools by configuration

d5707b5

roiedanino force-pushed the dc/dynamic-allocation-of-dci branch from f09c73c to d5707b5 Compare February 7, 2024 16:08

UCT/IB/DC: code style - clang format

06e3bd1

roiedanino requested review from Artemy-Mellanox and yosefe February 7, 2024 16:16

roiedanino added Ready for Review and removed WIP-DNM Work in progress / Do not review labels Feb 7, 2024

UCT/IB/DC: fixed iface_flush_dcis to use correct number of dcis

9d98e1c

gleon99 reviewed Feb 12, 2024

View reviewed changes

roiedanino added WIP-DNM Work in progress / Do not review and removed Ready for Review labels Feb 12, 2024

roiedanino added 14 commits February 12, 2024 18:22

UCT/IB/DC: create dci pools on dci_allocation instead of on pool crea…

0504114

…tion

UCT/IB/DC: added assert for pool_index < MAX_DCI_POOLS

198f7a0

UCT/IB/DC: check uct_dc_mlx5_dci_pool_get_or_create return value

29a3bb9

UCT/IB/DC: fixup - missing status declaration

0a8f369

UCT/IB/DC: removed unreachable exit label

88137e0

UCT/IB/DC: removed unused variable status

e3ba33c

UCT/IB/DC: fixed dynamic creation of dcis for random policy

90d144a

UCT/IB/DC: removed asserts - cannot assume that anymore

50260bf

UCT/IB/DC: added field for dcis as not all dcis will be initialized

c4e7352

UCT/IB/DC: destructing all initialized dcis

fc5df1d

UCT/IB/DC: reverted blank spaces

9beddcb

UCT/IB/DC: check status after dci_pool get or create

630aece

UCT/IB/DC: failures when using SL > 7

43f9feb

UCT/IB/DC: cleanup and code style

2809d59

roiedanino commented Mar 6, 2024

View reviewed changes

src/uct/ib/dc/dc_mlx5.h Outdated Show resolved Hide resolved

UCT/IB/DC: releasing fc properly

e104c23

roiedanino added 4 commits April 17, 2024 16:05

UCT/IB/DC: removed parameters that are still derived from iface from …

0822950

…config key

UCT/IB/DC: started converting pool->stack to ucs_array

5de4578

UCT/IB/DC: still dealing with segfaults in tests

edea9a4

UCT/IB/DC: transforming dcis into ucs_array

3a947c6

roiedanino added the WIP-DNM Work in progress / Do not review label Apr 21, 2024

roiedanino added 3 commits April 24, 2024 11:25

UCT/IB/DC: issues with ucs_array (Dcis)

1e6b303

UCT/IB/DC: changed dcis index to unit16_t

34e9425

Merge branch 'master' into dc/dynamic-allocation-of-dci

edaaea4

Conflicts: src/uct/ib/dc/dc_mlx5.c src/uct/ib/dc/dc_mlx5.h src/uct/ib/dc/dc_mlx5_ep.c

roiedanino requested a review from yosefe April 25, 2024 09:50

roiedanino added Ready for Review and removed WIP-DNM Work in progress / Do not review labels Apr 25, 2024

yosefe reviewed May 5, 2024

View reviewed changes

roiedanino added 11 commits May 8, 2024 11:18

UCT/IB/DC: removed dci_counter, dcis array is dynamically expanding

95b21b8

TOOLS/INFO: added a print for uct_dc_mlx5_dci_pool_t size

1f4b16b

UCT/IB/DC: aligned iface struct fields

c13931f

UCT/IB/DC: code style

c2734d2

UCT/IB/DC: don't create a default pool in dcis_create, issue in flush…

8d41d9c

… cancel

UCT/IB/DC: checked the fake dci created for bb_max is not being sched…

78f2c62

…uled

UCT/IB/DC: fixed fc_ep pool index (bit mask was wrong)

a5e8b5d

UCT/IB/DC: fixing can_alloc logic

4969097

UCT/IB/DC: bug fixes (failing tests)

07a3ccd

UCT/IB/DC: minor fixes - removed spaces and comments

7ced626

TEST/GTEST/UCT/IB: removed spaces

0ab7075

roiedanino requested a review from yosefe June 3, 2024 12:51

roiedanino added 6 commits June 3, 2024 16:00

Merge branch 'master' into dc/dynamic-allocation-of-dci

440fda4

Conflicts: src/uct/ib/dc/dc_mlx5.c

UCT/IB/DC: wrapped dci initializers with {{ }} (gcc bug 53119)

3da41b0

UCT/IB/DC: issue in fc_pure_grant_send

c0d7cc5

UCT/IB/DC: added missing check in dci_can_alloc

85033b1

TEST/GTEST/UCT/IB: reverted a bad change in tests

54cae42

UCT/IB/DC: ep->flags should be initialized to 0

2af108f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/IB/DC: dynamic allocation of dci pools by configuration #9665

UCT/IB/DC: dynamic allocation of dci pools by configuration #9665

roiedanino commented Feb 6, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

gleon99 Feb 12, 2024

roiedanino Feb 12, 2024

gleon99 Feb 12, 2024

roiedanino Feb 12, 2024

gleon99 commented Feb 12, 2024

yosefe May 5, 2024

yosefe May 5, 2024

roiedanino May 5, 2024 •

edited

yosefe May 5, 2024

roiedanino May 5, 2024

yosefe May 5, 2024

yosefe May 5, 2024

yosefe May 5, 2024

yosefe May 5, 2024

yosefe May 5, 2024

yosefe May 5, 2024

yosefe May 5, 2024

		uint8_t pool_size = //(config->key.flags & UCT_DC_MLX5_DCI_CONFIG_KEEPALIVE) ?
		// UCT_DC_MLX5_KEEPALIVE_NUM_DCIS :


		stack_ptr = ucs_array_append(&pool->stack, return UCS_ERR_NO_MEMORY);
		*stack_ptr = dci_index;

UCT/IB/DC: dynamic allocation of dci pools by configuration #9665

Are you sure you want to change the base?

UCT/IB/DC: dynamic allocation of dci pools by configuration #9665

Conversation

roiedanino commented Feb 6, 2024

What

Why ?

How ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gleon99 commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roiedanino May 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roiedanino May 5, 2024 •

edited