[Asterisk-code-review] app_queue: Fix deadlock between update and show queues (asterisk[13])
George Joseph
asteriskteam at digium.com
Wed Nov 11 08:55:22 CST 2020
George Joseph has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/15171 )
Change subject: app_queue: Fix deadlock between update and show queues
......................................................................
app_queue: Fix deadlock between update and show queues
Operations that update queues when shared_lastcall is set lock the
queue in question, then have to lock the queues container to find the
other queues with the same member. On the other hand, __queues_show
(which is called by both the CLI and AMI) does the reverse. It locks
the queues container, then iterates over the queues locking each in
turn to display them. This creates a deadlock.
* Moved queue print logic from __queues_show to a separate function
that can be called for a single queue.
* Updated __queues_show so it doesn't need to lock or traverse
the queues container to show a single queue.
* Updated __queues_show to snap a copy of the queues container and iterate
over that instead of locking the queues container and iterating over
it while locked. This prevents us from having to hold both the
container lock and the queue locks at the same time. This also
allows us to sort the queue entries.
ASTERISK-29155
Change-Id: I78d4dc36728c2d7bc187b97d82673fc77f2bcf41
---
M apps/app_queue.c
1 file changed, 122 insertions(+), 94 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/71/15171/1
diff --git a/apps/app_queue.c b/apps/app_queue.c
index 4db468f..4ab30d8 100644
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -9211,6 +9211,90 @@
}
}
+/*! \brief Print a single queue to AMI or the CLI */
+static void print_queue(struct mansession *s, int fd, struct call_queue *q)
+{
+ float sl;
+ struct ao2_iterator mem_iter;
+ struct ast_str *out = ast_str_alloca(512);
+ time_t now = time(NULL);
+
+ ast_str_set(&out, 0, "%s has %d calls (max ", q->name, q->count);
+ if (q->maxlen) {
+ ast_str_append(&out, 0, "%d", q->maxlen);
+ } else {
+ ast_str_append(&out, 0, "unlimited");
+ }
+ sl = 0;
+ if (q->callscompleted > 0) {
+ sl = 100 * ((float) q->callscompletedinsl / (float) q->callscompleted);
+ }
+ ast_str_append(&out, 0, ") in '%s' strategy (%ds holdtime, %ds talktime), W:%d, C:%d, A:%d, SL:%2.1f%% within %ds",
+ int2strat(q->strategy), q->holdtime, q->talktime, q->weight,
+ q->callscompleted, q->callsabandoned,sl,q->servicelevel);
+ do_print(s, fd, ast_str_buffer(out));
+ if (!ao2_container_count(q->members)) {
+ do_print(s, fd, " No Members");
+ } else {
+ struct member *mem;
+
+ do_print(s, fd, " Members: ");
+ mem_iter = ao2_iterator_init(q->members, 0);
+ while ((mem = ao2_iterator_next(&mem_iter))) {
+ ast_str_set(&out, 0, " %s", mem->membername);
+ if (strcasecmp(mem->membername, mem->interface)) {
+ ast_str_append(&out, 0, " (%s", mem->interface);
+ if (!ast_strlen_zero(mem->state_interface)
+ && strcmp(mem->state_interface, mem->interface)) {
+ ast_str_append(&out, 0, " from %s", mem->state_interface);
+ }
+ ast_str_append(&out, 0, ")");
+ }
+ if (mem->penalty) {
+ ast_str_append(&out, 0, " with penalty %d", mem->penalty);
+ }
+
+ ast_str_append(&out, 0, " (ringinuse %s)", mem->ringinuse ? "enabled" : "disabled");
+
+ ast_str_append(&out, 0, "%s%s%s%s%s%s%s%s%s%s%s%s (%s%s%s)",
+ mem->dynamic ? ast_term_color(COLOR_CYAN, COLOR_BLACK) : "", mem->dynamic ? " (dynamic)" : "", ast_term_reset(),
+ mem->realtime ? ast_term_color(COLOR_MAGENTA, COLOR_BLACK) : "", mem->realtime ? " (realtime)" : "", ast_term_reset(),
+ mem->paused ? ast_term_color(COLOR_BROWN, COLOR_BLACK) : "", mem->paused ? " (paused)" : "", ast_term_reset(),
+ mem->starttime ? ast_term_color(COLOR_BROWN, COLOR_BLACK) : "", mem->starttime ? " (in call)" : "", ast_term_reset(),
+ ast_term_color(
+ mem->status == AST_DEVICE_UNAVAILABLE || mem->status == AST_DEVICE_UNKNOWN ?
+ COLOR_RED : COLOR_GREEN, COLOR_BLACK),
+ ast_devstate2str(mem->status), ast_term_reset());
+ if (mem->calls) {
+ ast_str_append(&out, 0, " has taken %d calls (last was %ld secs ago)",
+ mem->calls, (long) (time(NULL) - mem->lastcall));
+ } else {
+ ast_str_append(&out, 0, " has taken no calls yet");
+ }
+ do_print(s, fd, ast_str_buffer(out));
+ ao2_ref(mem, -1);
+ }
+ ao2_iterator_destroy(&mem_iter);
+ }
+ if (!q->head) {
+ do_print(s, fd, " No Callers");
+ } else {
+ struct queue_ent *qe;
+ int pos = 1;
+
+ do_print(s, fd, " Callers: ");
+ for (qe = q->head; qe; qe = qe->next) {
+ ast_str_set(&out, 0, " %d. %s (wait: %ld:%2.2ld, prio: %d)",
+ pos++, ast_channel_name(qe->chan), (long) (now - qe->start) / 60,
+ (long) (now - qe->start) % 60, qe->prio);
+ do_print(s, fd, ast_str_buffer(out));
+ }
+ }
+ do_print(s, fd, ""); /* blank line between entries */
+}
+
+AO2_STRING_FIELD_SORT_FN(call_queue, name);
+
/*!
* \brief Show queue(s) status and statistics
*
@@ -9221,10 +9305,10 @@
{
struct call_queue *q;
struct ast_str *out = ast_str_alloca(512);
- int found = 0;
- time_t now = time(NULL);
+ struct ao2_container *sorted_queues;
+
struct ao2_iterator queue_iter;
- struct ao2_iterator mem_iter;
+ int found = 0;
if (argc != 2 && argc != 3) {
return CLI_SHOWUSAGE;
@@ -9232,9 +9316,18 @@
if (argc == 3) { /* specific queue */
if ((q = find_load_queue_rt_friendly(argv[2]))) {
- queue_t_unref(q, "Done with temporary pointer");
+ ao2_lock(q);
+ print_queue(s, fd, q);
+ ao2_unlock(q);
+ queue_unref(q);
+ } else {
+ ast_str_set(&out, 0, "No such queue: %s.", argv[2]);
+ do_print(s, fd, ast_str_buffer(out));
}
- } else if (ast_check_realtime("queues")) {
+ return CLI_SUCCESS;
+ }
+
+ if (ast_check_realtime("queues")) {
/* This block is to find any queues which are defined in realtime but
* which have not yet been added to the in-core container
*/
@@ -9251,19 +9344,34 @@
}
}
- ao2_lock(queues);
- queue_iter = ao2_iterator_init(queues, AO2_ITERATOR_DONTLOCK);
- while ((q = ao2_t_iterator_next(&queue_iter, "Iterate through queues"))) {
- float sl;
- struct call_queue *realtime_queue = NULL;
+ /*
+ * Snapping a copy of the container prevents having to lock both the queues container
+ * and the queue itself at the same time. It also allows us to sort the entries.
+ */
+ sorted_queues = ao2_container_alloc_rbtree(AO2_ALLOC_OPT_LOCK_NOLOCK, 0, call_queue_sort_fn, NULL);
+ if (!sorted_queues) {
+ return CLI_SUCCESS;
+ }
+ if (ao2_container_dup(sorted_queues, queues, 0)) {
+ ao2_ref(sorted_queues, -1);
+ return CLI_SUCCESS;
+ }
+ /*
+ * No need to lock the container since it's temporary and static.
+ * We also unlink the entries as we use them so the container is
+ * empty when the iterator finishes. We can then just unref the container.
+ */
+ queue_iter = ao2_iterator_init(sorted_queues, AO2_ITERATOR_DONTLOCK | AO2_ITERATOR_UNLINK);
+ while ((q = ao2_t_iterator_next(&queue_iter, "Iterate through queues"))) {
+ struct call_queue *realtime_queue = NULL;
ao2_lock(q);
/* This check is to make sure we don't print information for realtime
* queues which have been deleted from realtime but which have not yet
* been deleted from the in-core container. Only do this if we're not
* looking for a specific queue.
*/
- if (argc < 3 && q->realtime) {
+ if (q->realtime) {
realtime_queue = find_load_queue_rt_friendly(q->name);
if (!realtime_queue) {
ao2_unlock(q);
@@ -9273,96 +9381,16 @@
queue_t_unref(realtime_queue, "Queue is already in memory");
}
- if (argc == 3 && strcasecmp(q->name, argv[2])) {
- ao2_unlock(q);
- queue_t_unref(q, "Done with iterator");
- continue;
- }
found = 1;
+ print_queue(s, fd, q);
- ast_str_set(&out, 0, "%s has %d calls (max ", q->name, q->count);
- if (q->maxlen) {
- ast_str_append(&out, 0, "%d", q->maxlen);
- } else {
- ast_str_append(&out, 0, "unlimited");
- }
- sl = 0;
- if (q->callscompleted > 0) {
- sl = 100 * ((float) q->callscompletedinsl / (float) q->callscompleted);
- }
- ast_str_append(&out, 0, ") in '%s' strategy (%ds holdtime, %ds talktime), W:%d, C:%d, A:%d, SL:%2.1f%% within %ds",
- int2strat(q->strategy), q->holdtime, q->talktime, q->weight,
- q->callscompleted, q->callsabandoned,sl,q->servicelevel);
- do_print(s, fd, ast_str_buffer(out));
- if (!ao2_container_count(q->members)) {
- do_print(s, fd, " No Members");
- } else {
- struct member *mem;
-
- do_print(s, fd, " Members: ");
- mem_iter = ao2_iterator_init(q->members, 0);
- while ((mem = ao2_iterator_next(&mem_iter))) {
- ast_str_set(&out, 0, " %s", mem->membername);
- if (strcasecmp(mem->membername, mem->interface)) {
- ast_str_append(&out, 0, " (%s", mem->interface);
- if (!ast_strlen_zero(mem->state_interface)
- && strcmp(mem->state_interface, mem->interface)) {
- ast_str_append(&out, 0, " from %s", mem->state_interface);
- }
- ast_str_append(&out, 0, ")");
- }
- if (mem->penalty) {
- ast_str_append(&out, 0, " with penalty %d", mem->penalty);
- }
-
- ast_str_append(&out, 0, " (ringinuse %s)", mem->ringinuse ? "enabled" : "disabled");
-
- ast_str_append(&out, 0, "%s%s%s%s%s%s%s%s%s%s%s%s (%s%s%s)",
- mem->dynamic ? ast_term_color(COLOR_CYAN, COLOR_BLACK) : "", mem->dynamic ? " (dynamic)" : "", ast_term_reset(),
- mem->realtime ? ast_term_color(COLOR_MAGENTA, COLOR_BLACK) : "", mem->realtime ? " (realtime)" : "", ast_term_reset(),
- mem->paused ? ast_term_color(COLOR_BROWN, COLOR_BLACK) : "", mem->paused ? " (paused)" : "", ast_term_reset(),
- mem->starttime ? ast_term_color(COLOR_BROWN, COLOR_BLACK) : "", mem->starttime ? " (in call)" : "", ast_term_reset(),
- ast_term_color(
- mem->status == AST_DEVICE_UNAVAILABLE || mem->status == AST_DEVICE_UNKNOWN ?
- COLOR_RED : COLOR_GREEN, COLOR_BLACK),
- ast_devstate2str(mem->status), ast_term_reset());
- if (mem->calls) {
- ast_str_append(&out, 0, " has taken %d calls (last was %ld secs ago)",
- mem->calls, (long) (time(NULL) - mem->lastcall));
- } else {
- ast_str_append(&out, 0, " has taken no calls yet");
- }
- do_print(s, fd, ast_str_buffer(out));
- ao2_ref(mem, -1);
- }
- ao2_iterator_destroy(&mem_iter);
- }
- if (!q->head) {
- do_print(s, fd, " No Callers");
- } else {
- struct queue_ent *qe;
- int pos = 1;
-
- do_print(s, fd, " Callers: ");
- for (qe = q->head; qe; qe = qe->next) {
- ast_str_set(&out, 0, " %d. %s (wait: %ld:%2.2ld, prio: %d)",
- pos++, ast_channel_name(qe->chan), (long) (now - qe->start) / 60,
- (long) (now - qe->start) % 60, qe->prio);
- do_print(s, fd, ast_str_buffer(out));
- }
- }
- do_print(s, fd, ""); /* blank line between entries */
ao2_unlock(q);
queue_t_unref(q, "Done with iterator"); /* Unref the iterator's reference */
}
ao2_iterator_destroy(&queue_iter);
- ao2_unlock(queues);
+ ao2_ref(sorted_queues, -1);
if (!found) {
- if (argc == 3) {
- ast_str_set(&out, 0, "No such queue: %s.", argv[2]);
- } else {
- ast_str_set(&out, 0, "No queues.");
- }
+ ast_str_set(&out, 0, "No queues.");
do_print(s, fd, ast_str_buffer(out));
}
return CLI_SUCCESS;
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15171
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I78d4dc36728c2d7bc187b97d82673fc77f2bcf41
Gerrit-Change-Number: 15171
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20201111/bafde624/attachment-0001.html>
More information about the asterisk-code-review
mailing list