Review feedback
diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
index 56dd87c..57da5c5 100644
--- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c
+++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c
@@ -2189,7 +2189,7 @@
GRPC_ERROR_INT_HTTP2_ERROR,
GRPC_CHTTP2_ENHANCE_YOUR_CALM));
if (n > 1) {
- /* Since we cancel one stream per destructive reclaimation, if
+ /* Since we cancel one stream per destructive reclamation, if
there are more streams left, we can immediately post a new
reclaimer in case the resource quota needs to free more
memory */
diff --git a/src/core/lib/iomgr/resource_quota.c b/src/core/lib/iomgr/resource_quota.c
index 4f0d16c..bfc9058 100644
--- a/src/core/lib/iomgr/resource_quota.c
+++ b/src/core/lib/iomgr/resource_quota.c
@@ -89,6 +89,7 @@
resource_user->links[list].prev = (*root)->links[list].prev;
resource_user->links[list].next->links[list].prev =
resource_user->links[list].prev->links[list].next = resource_user;
+ *root = resource_user;
}
}
@@ -105,7 +106,6 @@
resource_user->links[list].prev = *root;
resource_user->links[list].next->links[list].prev =
resource_user->links[list].prev->links[list].next = resource_user;
- *root = resource_user;
}
}
@@ -114,7 +114,7 @@
return resource_quota->roots[list] == NULL;
}
-static grpc_resource_user *rulist_pop_tail(grpc_resource_quota *resource_quota,
+static grpc_resource_user *rulist_pop_head(grpc_resource_quota *resource_quota,
grpc_rulist list) {
grpc_resource_user **root = &resource_quota->roots[list];
grpc_resource_user *resource_user = *root;
@@ -186,7 +186,7 @@
static bool rq_alloc(grpc_exec_ctx *exec_ctx,
grpc_resource_quota *resource_quota) {
grpc_resource_user *resource_user;
- while ((resource_user = rulist_pop_tail(resource_quota,
+ while ((resource_user = rulist_pop_head(resource_quota,
GRPC_RULIST_AWAITING_ALLOCATION))) {
gpr_mu_lock(&resource_user->mu);
if (resource_user->free_pool < 0 &&
@@ -209,7 +209,7 @@
grpc_exec_ctx_enqueue_list(exec_ctx, &resource_user->on_allocated, NULL);
gpr_mu_unlock(&resource_user->mu);
} else {
- rulist_add_tail(resource_user, GRPC_RULIST_AWAITING_ALLOCATION);
+ rulist_add_head(resource_user, GRPC_RULIST_AWAITING_ALLOCATION);
gpr_mu_unlock(&resource_user->mu);
return false;
}
@@ -221,7 +221,7 @@
static bool rq_reclaim_from_per_user_free_pool(
grpc_exec_ctx *exec_ctx, grpc_resource_quota *resource_quota) {
grpc_resource_user *resource_user;
- while ((resource_user = rulist_pop_tail(resource_quota,
+ while ((resource_user = rulist_pop_head(resource_quota,
GRPC_RULIST_NON_EMPTY_FREE_POOL))) {
gpr_mu_lock(&resource_user->mu);
if (resource_user->free_pool > 0) {
@@ -249,7 +249,7 @@
if (resource_quota->reclaiming) return true;
grpc_rulist list = destructive ? GRPC_RULIST_RECLAIMER_DESTRUCTIVE
: GRPC_RULIST_RECLAIMER_BENIGN;
- grpc_resource_user *resource_user = rulist_pop_tail(resource_quota, list);
+ grpc_resource_user *resource_user = rulist_pop_head(resource_quota, list);
if (resource_user == NULL) return false;
if (grpc_resource_quota_trace) {
gpr_log(GPR_DEBUG, "RQ %s %s: initiate %s reclamation",
@@ -325,7 +325,7 @@
GRPC_RULIST_AWAITING_ALLOCATION)) {
rq_step_sched(exec_ctx, resource_user->resource_quota);
}
- rulist_add_head(resource_user, GRPC_RULIST_AWAITING_ALLOCATION);
+ rulist_add_tail(resource_user, GRPC_RULIST_AWAITING_ALLOCATION);
}
static void ru_add_to_free_pool(grpc_exec_ctx *exec_ctx, void *ru,
@@ -337,7 +337,7 @@
GRPC_RULIST_NON_EMPTY_FREE_POOL)) {
rq_step_sched(exec_ctx, resource_user->resource_quota);
}
- rulist_add_head(resource_user, GRPC_RULIST_NON_EMPTY_FREE_POOL);
+ rulist_add_tail(resource_user, GRPC_RULIST_NON_EMPTY_FREE_POOL);
}
static void ru_post_benign_reclaimer(grpc_exec_ctx *exec_ctx, void *ru,
@@ -351,7 +351,7 @@
GRPC_RULIST_RECLAIMER_BENIGN)) {
rq_step_sched(exec_ctx, resource_user->resource_quota);
}
- rulist_add_head(resource_user, GRPC_RULIST_RECLAIMER_BENIGN);
+ rulist_add_tail(resource_user, GRPC_RULIST_RECLAIMER_BENIGN);
}
static void ru_post_destructive_reclaimer(grpc_exec_ctx *exec_ctx, void *ru,
@@ -367,7 +367,7 @@
GRPC_RULIST_RECLAIMER_DESTRUCTIVE)) {
rq_step_sched(exec_ctx, resource_user->resource_quota);
}
- rulist_add_head(resource_user, GRPC_RULIST_RECLAIMER_DESTRUCTIVE);
+ rulist_add_tail(resource_user, GRPC_RULIST_RECLAIMER_DESTRUCTIVE);
}
static void ru_destroy(grpc_exec_ctx *exec_ctx, void *ru, grpc_error *error) {
@@ -563,9 +563,6 @@
for (int i = 0; i < GRPC_RULIST_COUNT; i++) {
resource_user->links[i].next = resource_user->links[i].prev = NULL;
}
-#ifndef NDEBUG
- resource_user->asan_canary = gpr_malloc(1);
-#endif
if (name != NULL) {
resource_user->name = gpr_strdup(name);
} else {
@@ -592,9 +589,6 @@
void grpc_resource_user_destroy(grpc_exec_ctx *exec_ctx,
grpc_resource_user *resource_user) {
-#ifndef NDEBUG
- gpr_free(resource_user->asan_canary);
-#endif
grpc_resource_quota_internal_unref(exec_ctx, resource_user->resource_quota);
gpr_mu_destroy(&resource_user->mu);
gpr_free(resource_user->name);
diff --git a/src/core/lib/iomgr/resource_quota.h b/src/core/lib/iomgr/resource_quota.h
index c15cb68..6dfac55 100644
--- a/src/core/lib/iomgr/resource_quota.h
+++ b/src/core/lib/iomgr/resource_quota.h
@@ -49,7 +49,8 @@
resource constrained, grpc_resource_user instances are asked (in turn) to
free up whatever they can so that the system as a whole can make progress.
- There are three kinds of reclamation that take place:
+ There are three kinds of reclamation that take place, in order of increasing
+ invasiveness:
- an internal reclamation, where cached resource at the resource user level
is returned to the quota
- a benign reclamation phase, whereby resources that are in use but are not
@@ -58,9 +59,14 @@
make progress may be enacted so that at least one part of the system can
complete.
- These reclamations are tried in priority order, and only one reclamation
- is outstanding for a quota at any given time (meaning that if a destructive
- reclamation makes progress, we may follow up with a benign reclamation).
+ Only one reclamation will be outstanding for a given quota at a given time.
+ On each reclamation attempt, the kinds of reclamation are tried in order of
+ increasing invasiveness, stopping at the first one that succeeds. Thus, on a
+ given reclamation attempt, if internal and benign reclamation both fail, it
+ will wind up doing a destructive reclamation. However, the next reclamation
+ attempt may then be able to get what it needs via internal or benign
+ reclamation, due to resources that may have been freed up by the destructive
+ reclamation in the previous attempt.
Future work will be to expose the current resource pressure so that back
pressure can be applied to avoid reclamation phases starting.
@@ -112,11 +118,6 @@
lock */
grpc_closure add_to_free_pool_closure;
-#ifndef NDEBUG
- /* Canary object to detect leaked resource users with ASAN */
- void *asan_canary;
-#endif
-
gpr_mu mu;
/* Total allocated memory outstanding by this resource user in bytes;
always positive */
diff --git a/test/core/end2end/tests/resource_quota_server.c b/test/core/end2end/tests/resource_quota_server.c
index 6170444..f594db3 100644
--- a/test/core/end2end/tests/resource_quota_server.c
+++ b/test/core/end2end/tests/resource_quota_server.c
@@ -339,9 +339,8 @@
"Done. %d total calls: %d cancelled at server, %d cancelled at client.",
NUM_CALLS, cancelled_calls_on_server, cancelled_calls_on_client);
- /* The server call may be cancelled after it's received it's status, but
- * before the client does: this means that we should see strictly more
- * failures on the client than on the server */
+ /* The call may be cancelled after the server has sent its status but before
+ * the client has received it. */
GPR_ASSERT(cancelled_calls_on_client >= cancelled_calls_on_server);
/* However, we shouldn't see radically more... 0.9 is a guessed bound on what
* we'd want that ratio to be... to at least trigger some investigation should
diff --git a/test/core/iomgr/resource_quota_test.c b/test/core/iomgr/resource_quota_test.c
index 5e2f9f8..34dee1a 100644
--- a/test/core/iomgr/resource_quota_test.c
+++ b/test/core/iomgr/resource_quota_test.c
@@ -553,9 +553,13 @@
static void
test_resource_user_stays_allocated_and_reclaimers_unrun_until_memory_released(
void) {
- gpr_log(GPR_INFO, "** test_pools_merged_on_resource_user_deletion **");
- grpc_resource_quota *q =
- grpc_resource_quota_create("test_pools_merged_on_resource_user_deletion");
+ gpr_log(GPR_INFO,
+ "** "
+ "test_resource_user_stays_allocated_and_reclaimers_unrun_until_"
+ "memory_released **");
+ grpc_resource_quota *q = grpc_resource_quota_create(
+ "test_resource_user_stays_allocated_and_reclaimers_unrun_until_memory_"
+ "released");
grpc_resource_quota_resize(q, 1024);
for (int i = 0; i < 10; i++) {
grpc_resource_user usr;