DAOS-14484 cart: Implement per-context inflight queue #13202

frostedcmos · 2023-10-19T06:40:40Z

Implement per-context inflight queue

Signed-off-by: Alexander Oganezov [email protected]

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

- Implement RPC inflight quota Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

github-actions · 2023-10-19T06:41:38Z

Bug-tracker data:
Ticket title is 'Refactor RPC-in-flight limit functionality'
Status is 'Blocked'
https://daosio.atlassian.net/browse/DAOS-14484

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-10-19T07:14:14Z

Test stage Unit Test on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13202/1/testReport/

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-10-19T10:14:36Z

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/2/execution/node/1121/log

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

soumagne · 2023-10-19T19:40:45Z

src/include/cart/api.h

+ *                             failure.
+ */
+int crt_context_quotas_finalize(crt_context_t crt_ctx);
+


why are init / finalize public APIs ?

I am still going back and forth whether we want to have a runtime control to disable (and subsequently re-enable) quotas. I started with that idea, which is why these are public, but for now auto-enabling it on every context

src/include/cart/types.h

src/cart/crt_context.c

daosbuild1 · 2023-10-19T23:06:48Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/3/execution/node/1266/log

daosbuild1 · 2023-10-20T16:12:15Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/3/execution/node/1404/log

daosbuild1 · 2023-10-20T17:11:19Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/3/execution/node/1358/log

- add comment to not implemented quotas - set default to 32 inflight Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-10-22T12:08:27Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/4/execution/node/1266/log

daosbuild1 · 2023-10-22T19:10:12Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/4/execution/node/1404/log

its quota reservation for any rpc in the list. Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-10-23T23:08:06Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/5/execution/node/1266/log

daosbuild1 · 2023-10-24T02:23:12Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/5/execution/node/1404/log

when quota limit is reached. Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-10-24T22:38:39Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/6/execution/node/1399/log

daosbuild1

LGTM. No errors found by checkpatch.

soumagne · 2023-12-11T23:02:31Z

src/cart/README.env

@@ -139,6 +139,12 @@ This file lists the environment variables used in CaRT.
   It its value exceed 256, then will use 256 for flow control.
   Set it to zero means disable the flow control in cart.

+ . D_QUOTA_RPCS


maybe we should just call it D_RPC_MAX_IN_FLIGHT even if it's a little longer ? :) as I feel D_QUOTA_RPCS might be too generic and introduce a different nomenclature with D_QUOTA (although I know that's what you're trying to do here).

Sure, I am open to naming, but i dont like D_RPC_MAX_IN_FLIGHT as its long and harder to remember.

I am okay with D_QUOTA_RPCS.

soumagne · 2023-12-11T23:14:22Z

src/include/cart/api.h

+ *                             failure.
+ */
+int crt_context_quota_limit_get(crt_context_t crt_ctx, crt_quota_type_t quota, int *value);
+
 /**


Thinking more about it I'm not certain we should introduce those new APIs unless we already have a specific use for them. Right now I don't think we really have one yet ?

Yeah I am mixed on this. I was thinking that one use-case we have is self-test (or future perf tools?) to be able to adjust quotas on a fly, based cmd line args and not have to set env.

soumagne · 2023-12-11T23:18:45Z

src/cart/crt_init.c

@@ -257,6 +258,8 @@ prov_data_init(struct crt_prov_gdata *prov_data, crt_provider_t provider,
 	return DER_SUCCESS;
 }

+#define CRT_QUOTA_RPCS_DEFAULT 64


I don't think that default should be set in the middle of nowhere but maybe in some place where other defaults are set ?

It's not in a middle of nowhere, but sure:) It was declared outside of the function that used it. i ll find a better place:)

soumagne · 2023-12-11T23:20:29Z

src/include/cart/types.h

+	/** Total count of supported quotas */
+	CRT_QUOTA_COUNT,
+} crt_quota_type_t;
+
 /** @}


same comment as above, maybe we should not have all this quota API and keep it simple for now.

yeah if we decide we dont need public apis, this will move to internal header

soumagne · 2023-12-11T23:26:02Z

src/cart/crt_context.c

@@ -11,6 +11,8 @@
 #include "crt_internal.h"

 static void crt_epi_destroy(struct crt_ep_inflight *epi);
+static int context_quotas_init(crt_context_t crt_ctx);
+static int context_quotas_finalize(crt_context_t crt_ctx);



Same comment here, unless we have more, maybe that's not necessary to have quotas init and finalize (is finalize really needed also?)

We have quota mutex to destroy in finalize. Possibly for other quotas we might want to clean any lists, but not needed for rpc list as untrack logic of each rpc will take care of it. But we might need for allocation queues if we ever add those.

soumagne · 2023-12-11T23:32:51Z

src/cart/crt_context.c

@@ -1264,6 +1286,10 @@ crt_context_req_track(struct crt_rpc_priv *rpc_priv)
 	/* reference taken by d_hash_rec_find or "epi->epi_ref = 1" above */
 	D_MUTEX_LOCK(&crt_ctx->cc_mutex);
 	d_hash_rec_decref(&crt_ctx->cc_epi_table, &epi->epi_link);
+
+	if (quota_rc == -DER_QUOTA_LIMIT)
+		d_list_add_tail(&rpc_priv->crp_waitq_link, &crt_ctx->cc_quotas.rpc_waitq);


why is this not within the block at line 1254 ?

why is this not within the block at line 1254 ?

So that it would not be done with epi->epi_mutex lock, but instead needs context lock.

We can reorganize things nicer once we can get rid of EP credits-related code, which should simplify this and few other calls greatly.

soumagne · 2023-12-11T23:35:41Z

src/cart/crt_context.c

+	int rc;
+
+	if (rpc == NULL)
+		return;


shouldn't that be an assert ?

can just remove it now or change to assert yes. its a left-over from a previous behavior

soumagne · 2023-12-11T23:40:31Z

src/cart/crt_context.c

+	if (tmp_rpc != NULL)
+		dispatch_rpc(tmp_rpc);
+	else
+		crt_context_put_quota_resource(rpc_priv->crp_pub.cr_ctx, CRT_QUOTA_RPCS);


I'm not sure I understand why crt_context_put_quota_resource needs to be invoked all the time ? which acquires/releases another lock ?

once the rpc is done, you either process the next rpc (reusing the existing quota) or you put the quota back if there is nothing else queued

daosbuild1 · 2023-12-12T03:42:44Z

Test stage Functional Hardware Medium Verbs Provider completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13202/12/testReport/

daosbuild1 · 2023-12-12T08:10:35Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13202/12/execution/node/1552/log

- move default to a diff file Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

- Add per-context quotas - Implement RPC inflight quota Signed-off-by: Alexander A Oganezov <[email protected]>

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

jolivier23 · 2023-12-20T18:49:49Z

src/cart/crt_context.c

+	if (ctx->cc_quotas.current[quota] < ctx->cc_quotas.limit[quota])
+		ctx->cc_quotas.current[quota]++;
+	else {
+		D_WARN("Quota limit reached for quota_type=%d\n", quota);


This is overly chatty and should be a debug

NB: "overly chatty" == gigabytes of client logs under load. ;)

- crt_req_set/get quota resource shortened and static inline now - changed warning to debug message when exceeding quotas Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

jgmoore-or · 2023-12-29T18:22:05Z

src/cart/README.env

@@ -139,6 +139,12 @@ This file lists the environment variables used in CaRT.
   It its value exceed 256, then will use 256 for flow control.
   Set it to zero means disable the flow control in cart.

+ . D_QUOTA_RPCS


I am okay with D_QUOTA_RPCS.

Backport of in-flight upstream PR #13202 - Add per-context quotas - Implement RPC inflight quota Required-githooks: true Change-Id: I3fdf77082d66d8009ee7099cc838fcff5da72d4b Signed-off-by: Alexander A Oganezov <[email protected]> Signed-off-by: Jeff Olivier <[email protected]>

- D_QUOTA_RPCS envariable added. When set, limits the number of RPCs on a wire being sent out by the process. - RPCs that exceed quota limit (if set), will now be queued by the sender - Quota support code added to handle and track resources Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

- D_QUOTA_RPCS envariable added. When set, limits the number of RPCs on a wire being sent out by the process. - RPCs that exceed quota limit (if set), will now be queued by the sender - Quota support code added to handle and track resources Signed-off-by: Alexander A Oganezov <[email protected]>

- D_QUOTA_RPCS envariable added. When set, limits the number of RPCs on a wire being sent out by the process. - RPCs that exceed quota limit (if set), will now be queued by the sender - Quota support code added to handle and track resources Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

frostedcmos added 3 commits October 10, 2023 21:41

initial changes

766df28

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

- Add per-context quotas

f2b7192

- Implement RPC inflight quota Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Merge branch 'master' into aaoganez/DAOS-14484

7e46893

frostedcmos requested a review from a team as a code owner October 19, 2023 06:40

daosbuild1 reviewed Oct 19, 2023

View reviewed changes

- update test_gurt to include new errors

39db446

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 reviewed Oct 19, 2023

View reviewed changes

- Change the check of ep credits to account for rpcs queued in the waitq

94087bd

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 reviewed Oct 19, 2023

View reviewed changes

soumagne reviewed Oct 19, 2023

View reviewed changes

- rename crt_quota_t -> crt_quota_type_t

0914730

- add comment to not implemented quotas - set default to 32 inflight Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 reviewed Oct 21, 2023

View reviewed changes

- Instead of release/re-request quota upon one rpc completion, keep

0f82132

its quota reservation for any rpc in the list. Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 reviewed Oct 23, 2023

View reviewed changes

frostedcmos added 2 commits October 24, 2023 07:27

Merge branch 'master' into aaoganez/DAOS-14484

559fcb3

modify quota limit for a debug and add warning print

8d121b5

when quota limit is reached. Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 reviewed Oct 24, 2023

View reviewed changes

Merge branch 'master' into aaoganez/DAOS-14484

343c2e5

frostedcmos requested a review from daltonbohning November 13, 2023 17:54

daosbuild1 reviewed Dec 11, 2023

View reviewed changes

jgmoore-or previously approved these changes Dec 11, 2023

View reviewed changes

soumagne reviewed Dec 11, 2023

View reviewed changes

- disable quotas by default on engines

f561648

- move default to a diff file Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

frostedcmos dismissed jgmoore-or’s stale review via f561648 December 12, 2023 16:16

daosbuild1 reviewed Dec 12, 2023

View reviewed changes

jolivier23 added a commit that referenced this pull request Dec 12, 2023

DAOS-14484 cart: Implement per-context inflight queue #13202 (#13488)

8c8cb95

- Add per-context quotas - Implement RPC inflight quota Signed-off-by: Alexander A Oganezov <[email protected]>

frostedcmos added 2 commits December 13, 2023 17:57

Merge branch 'master' into aaoganez/DAOS-14484

d37c9a5

- fix incorrect check

45463df

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 reviewed Dec 13, 2023

View reviewed changes

jolivier23 reviewed Dec 20, 2023

View reviewed changes

frostedcmos added 2 commits December 29, 2023 07:11

Merge branch 'master' into aaoganez/DAOS-14484

fe2543e

- 'current' quota changed to atomic. locks removed

9010e17

- crt_req_set/get quota resource shortened and static inline now - changed warning to debug message when exceeding quotas Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

frostedcmos requested review from jolivier23, soumagne and jgmoore-or December 29, 2023 07:38

- change check to assert for null rpc

584aaf7

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

jgmoore-or approved these changes Dec 29, 2023

View reviewed changes

jolivier23 approved these changes Jan 4, 2024

View reviewed changes

frostedcmos merged commit f7ab01c into master Jan 4, 2024

frostedcmos deleted the aaoganez/DAOS-14484 branch January 4, 2024 17:29

jolivier23 mentioned this pull request Jan 23, 2024

DAOS-14484 cart: Implement per-context inflight queue #13649

Closed

18 tasks

DAOS-14484 cart: Implement per-context inflight queue #13202

DAOS-14484 cart: Implement per-context inflight queue #13202

Conversation

frostedcmos commented Oct 19, 2023

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Oct 19, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 19, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 19, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 19, 2023

daosbuild1 commented Oct 20, 2023

daosbuild1 commented Oct 20, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 22, 2023

daosbuild1 commented Oct 22, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 23, 2023

daosbuild1 commented Oct 24, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 24, 2023

daosbuild1 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frostedcmos Dec 12, 2023 • edited Loading

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

daosbuild1 commented Dec 12, 2023

daosbuild1 commented Dec 12, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a 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

github-actions bot commented Oct 19, 2023 •

edited

Loading

frostedcmos Dec 12, 2023 •

edited

Loading