[Asterisk-code-review] ao2 container: Add support for transparent storage of weakpr... (asterisk[master])
Corey Farrell
asteriskteam at digium.com
Sat Aug 27 10:55:17 CDT 2016
Corey Farrell has uploaded a new change for review.
https://gerrit.asterisk.org/3749
Change subject: ao2_container: Add support for transparent storage of weakproxy objects.
......................................................................
ao2_container: Add support for transparent storage of weakproxy objects.
This adds support for declaring that a container will hold weakproxy
objects. This causes returns from ao2_find and ao2_callback to lookup
the real object held by the weakproxy. This saves two calls to ao2_ref
per returned object.
I do not believe this is ready to merged, but I'm ready to ask if it's
worth pursuing. Testing was done with some of the PJSIP registrar tests
due to their use of named locks. Many of these tests use SIGKILL to
stop Asterisk, but this was the case before my patch. I have not found
any test where this change causes Asterisk shutdown to timeout.
Change-Id: I48f2dfb85e97fd0a382a869cd084afda129c95d9
---
M include/asterisk/astobj2.h
M main/astobj2_container.c
M main/named_locks.c
3 files changed, 67 insertions(+), 30 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/49/3749/1
diff --git a/include/asterisk/astobj2.h b/include/asterisk/astobj2.h
index 2caa598..99841a9 100644
--- a/include/asterisk/astobj2.h
+++ b/include/asterisk/astobj2.h
@@ -1201,6 +1201,20 @@
* ao2_sort_fn.
*/
AO2_CONTAINER_ALLOC_OPT_DUPS_REPLACE = (3 << 1),
+ /*!
+ * \brief This container holds ao2_weakproxy objects.
+ *
+ * \details The container itself will always operate on weakproxy objects.
+ * The weakproxy (and not it's object) should be used with \ref ao2_link and
+ * \ref ao2_unlink. The weakproxy will always be passed to container callback
+ * functions (hash, compare and sort). ao2_callback / ao2_find will return
+ * return the object held by the weakproxy instead of the weakproxy itself.
+ *
+ * \note Removal of NULL weakproxy objects is automatic. If a weakproxy does
+ * not point to a real object it will be unlinked immediately. It is even
+ * possible for ao2_callback or ao2_find to unlink an object.
+ */
+ AO2_CONTAINER_ALLOC_OPT_WEAKPROXY_HOLDER = (8 << 0),
};
/*!
diff --git a/main/astobj2_container.c b/main/astobj2_container.c
index c00da9f..a9fb942 100644
--- a/main/astobj2_container.c
+++ b/main/astobj2_container.c
@@ -78,6 +78,12 @@
return 1;
}
+static void node_obj_proxy_cb(void *obj, void *self)
+{
+ ao2_unlink(self, obj);
+ ao2_ref(self, -1);
+}
+
/*!
* \internal
* \brief Link an object into this container. (internal)
@@ -116,7 +122,17 @@
}
res = 0;
- node = self->v_table->new_node(self, obj_new, tag, file, line, func);
+ if (self->options & AO2_CONTAINER_ALLOC_OPT_WEAKPROXY_HOLDER) {
+ /* verify obj_new is active weakproxy */
+ if (ao2_weakproxy_ref_object(obj_new, 0, 0) > 1) {
+ node = self->v_table->new_node(self, obj_new, tag, file, line, func);
+ } else {
+ node = NULL;
+ }
+ } else {
+ node = self->v_table->new_node(self, obj_new, tag, file, line, func);
+ }
+
if (node) {
#if defined(AO2_DEBUG)
if (ao2_container_check(self, OBJ_NOLOCK)) {
@@ -154,6 +170,15 @@
__adjust_lock(self, orig_lock, 0);
} else {
ao2_unlock(self);
+ }
+
+ if (self->options & AO2_CONTAINER_ALLOC_OPT_WEAKPROXY_HOLDER) {
+ ao2_t_ref(self, +1, "hold for node_obj_proxy_cb");
+ if (ao2_weakproxy_subscribe(obj_new, node_obj_proxy_cb, self, 0)) {
+ /* This is a bit of a problem, obj_new will not get cleaned from the list.
+ * Maybe we should ao2_unlink right now? */
+ ao2_t_ref(self, -1, "release hold for node_obj_proxy_cb");
+ }
}
return res;
@@ -349,11 +374,22 @@
* Link the object into the container that will hold the
* results.
*/
- __ao2_link(multi_container, node->obj, flags, tag, file, line, func);
+ if (self->options && AO2_CONTAINER_ALLOC_OPT_WEAKPROXY_HOLDER) {
+ void *obj = ao2_weakproxy_get_object(node->obj, 0);
+
+ if (obj) {
+ __ao2_link(multi_container, obj, flags, tag, file, line, func);
+ ao2_ref(obj, -1);
+ }
+ } else {
+ __ao2_link(multi_container, node->obj, flags, tag, file, line, func);
+ }
} else {
ret = node->obj;
/* Returning a single object. */
- if (!(flags & OBJ_UNLINK)) {
+ if (self->options && AO2_CONTAINER_ALLOC_OPT_WEAKPROXY_HOLDER) {
+ ret = ao2_weakproxy_get_object(ret, 0);
+ } else if (!(flags & OBJ_UNLINK)) {
/*
* Bump the ref count since we are not going to unlink and
* transfer the container's object ref to the returned object.
diff --git a/main/named_locks.c b/main/named_locks.c
index c71f3b5..d1cbe8c 100644
--- a/main/named_locks.c
+++ b/main/named_locks.c
@@ -61,7 +61,7 @@
static int named_locks_cmp(void *obj_left, void *obj_right, int flags)
{
- const struct named_lock_proxy *object_left = obj_left;
+ struct named_lock_proxy *object_left = obj_left;
const struct named_lock_proxy *object_right = obj_right;
const char *right_key = obj_right;
int cmp;
@@ -81,6 +81,11 @@
break;
}
+ /* Don't match an object in destruction. */
+ if (!cmp && ao2_weakproxy_ref_object(object_left, 0, 0) < 1) {
+ cmp = 1;
+ }
+
return cmp ? 0 : CMP_MATCH;
}
@@ -91,8 +96,9 @@
int ast_named_locks_init(void)
{
- named_locks = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX, 0,
- NAMED_LOCKS_BUCKETS, named_locks_hash, NULL, named_locks_cmp);
+ named_locks = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_MUTEX,
+ AO2_CONTAINER_ALLOC_OPT_WEAKPROXY_HOLDER, NAMED_LOCKS_BUCKETS,
+ named_locks_hash, NULL, named_locks_cmp);
if (!named_locks) {
return -1;
}
@@ -100,11 +106,6 @@
ast_register_cleanup(named_locks_shutdown);
return 0;
-}
-
-static void named_lock_proxy_cb(void *weakproxy, void *data)
-{
- ao2_unlink(named_locks, weakproxy);
}
struct ast_named_lock *__ast_named_lock_get(const char *filename, int lineno, const char *func,
@@ -118,23 +119,13 @@
sprintf(concat_key, "%s-%s", keyspace, key); /* Safe */
ao2_lock(named_locks);
- proxy = ao2_find(named_locks, concat_key, OBJ_SEARCH_KEY | OBJ_NOLOCK);
- if (proxy) {
+ lock = ao2_find(named_locks, concat_key, OBJ_SEARCH_KEY | OBJ_NOLOCK);
+ if (lock) {
ao2_unlock(named_locks);
- lock = __ao2_weakproxy_get_object(proxy, 0, __PRETTY_FUNCTION__, filename, lineno, func);
+ /* We have an existing lock. */
+ ast_assert((ao2_options_get(lock) & AO2_ALLOC_OPT_LOCK_MASK) == lock_type);
- if (lock) {
- /* We have an existing lock and it's not being destroyed. */
- ao2_ref(proxy, -1);
- ast_assert((ao2_options_get(lock) & AO2_ALLOC_OPT_LOCK_MASK) == lock_type);
-
- return lock;
- }
-
- /* the old proxy is being destroyed, clean list before creating/adding new one */
- ao2_lock(named_locks);
- ao2_unlink_flags(named_locks, proxy, OBJ_NOLOCK);
- ao2_ref(proxy, -1);
+ return lock;
}
proxy = ao2_t_weakproxy_alloc(sizeof(*proxy) + keylen, NULL, concat_key);
@@ -149,10 +140,6 @@
/* We have exclusive access to proxy and lock, no need for locking here. */
if (ao2_weakproxy_set_object(proxy, lock, OBJ_NOLOCK)) {
- goto failure_cleanup;
- }
-
- if (ao2_weakproxy_subscribe(proxy, named_lock_proxy_cb, NULL, OBJ_NOLOCK)) {
goto failure_cleanup;
}
--
To view, visit https://gerrit.asterisk.org/3749
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I48f2dfb85e97fd0a382a869cd084afda129c95d9
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
More information about the asterisk-code-review
mailing list