You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by st...@apache.org on 2004/09/17 16:58:06 UTC
cvs commit: httpd-2.0/modules/experimental mod_mem_cache.c
stoddard 2004/09/17 07:58:05
Modified: modules/experimental mod_mem_cache.c
Log:
eliminate cleanup bit in favor of managing the object exclusively with the refcount field
Revision Changes Path
1.116 +112 -64 httpd-2.0/modules/experimental/mod_mem_cache.c
Index: mod_mem_cache.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_mem_cache.c,v
retrieving revision 1.115
retrieving revision 1.116
diff -u -r1.115 -r1.116
--- mod_mem_cache.c 15 Sep 2004 15:09:09 -0000 1.115
+++ mod_mem_cache.c 17 Sep 2004 14:58:05 -0000 1.116
@@ -13,6 +13,27 @@
* limitations under the License.
*/
+/*
+ * Rules for managing obj->refcount:
+ * refcount should be incremented when an object is placed in the cache. Insertion
+ * of an object into the cache and the refcount increment should happen under
+ * protection of the sconf->lock.
+ *
+ * refcount should be decremented when the object is removed from the cache.
+ * Object should be removed from the cache and the refcount decremented while
+ * under protection of the sconf->lock.
+ *
+ * refcount should be incremented when an object is retrieved from the cache
+ * by a worker thread. The retrieval/find operation and refcount increment
+ * should occur under protection of the sconf->lock
+ *
+ * refcount can be atomically decremented w/o protection of the sconf->lock
+ * by worker threads.
+ *
+ * Any object whose refcount drops to 0 should be freed/cleaned up. A refcount
+ * of 0 means the object is not in the cache and no worker threads are accessing
+ * it.
+ */
#define CORE_PRIVATE
#include "mod_cache.h"
#include "cache_pqueue.h"
@@ -88,8 +109,6 @@
#define DEFAULT_MAX_STREAMING_BUFFER_SIZE 100000
#define CACHEFILE_LEN 20
-#define OBJECT_CLEANUP_BIT 0x00800000 /* flag to cleanup cache object */
-
/* Forward declarations */
static int remove_entity(cache_handle_t *h);
static apr_status_t store_headers(cache_handle_t *h, request_rec *r, cache_info *i);
@@ -144,25 +163,23 @@
return obj->key;
}
/**
- * function used to free an entry, used as a callback and as a local function.
- * There is way too much magic in this code. This callback
- * has to be called under protection of the sconf->lock, which means that we
- * do not (and should not) attempt to obtain the lock recursively.
+ * memcache_cache_free()
+ * memcache_cache_free is a callback that is only invoked by a thread
+ * running in cache_insert(). cache_insert() runs under protection
+ * of sconf->lock. By the time this function has been entered, the cache_object
+ * has been ejected from the cache. decrement the refcount and if the refcount drops
+ * to 0, cleanup the cache object.
*/
static void memcache_cache_free(void*a)
{
cache_object_t *obj = (cache_object_t *)a;
- apr_uint32_t tmp_refcount = obj->refcount;
- /* Cleanup the cache object. Object should be removed from the cache now. */
- if (!apr_atomic_read32(&obj->refcount)) {
+ /* Decrement the refcount to account for the object being ejected
+ * from the cache. If the refcount is 0, free the object.
+ */
+ if (!apr_atomic_dec32(&obj->refcount)) {
cleanup_cache_object(obj);
}
- /* checking if there was a collision with another thread */
- else if(tmp_refcount != apr_atomic_cas32(&obj->refcount, tmp_refcount | OBJECT_CLEANUP_BIT, tmp_refcount)) {
- memcache_cache_free(obj);
- }
- return;
}
/*
* functions return a 'negative' score since priority queues
@@ -269,26 +286,27 @@
/* If obj->complete is not set, the cache update failed and the
* object needs to be removed from the cache then cleaned up.
+ * The garbage collector may have ejected the object from the
+ * cache already, so make sure it is really still in the cache
+ * before attempting to remove it.
*/
if (!obj->complete) {
+ cache_object_t *tobj = NULL;
if (sconf->lock) {
apr_thread_mutex_lock(sconf->lock);
}
- /* Remember, objects marked for cleanup are, by design, already
- * removed from the cache. remove_url() could have already
- * removed the object from the cache (and set the OBJECT_CLEANUP_BIT)
- */
- if (!(apr_atomic_read32(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
+ tobj = cache_find(sconf->cache_cache, obj->key);
+ if (tobj == obj) {
cache_remove(sconf->cache_cache, obj);
- apr_atomic_set32(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT);
+ apr_atomic_dec32(&obj->refcount);
}
if (sconf->lock) {
apr_thread_mutex_unlock(sconf->lock);
}
- }
+ }
- /* Cleanup the cache object */
- if(apr_atomic_dec32(&obj->refcount) == OBJECT_CLEANUP_BIT) {
+ /* If the refcount drops to 0, cleanup the cache object */
+ if (!apr_atomic_dec32(&obj->refcount)) {
cleanup_cache_object(obj);
}
return APR_SUCCESS;
@@ -310,9 +328,10 @@
}
obj = cache_pop(co->cache_cache);
while (obj) {
- /* Iterate over the cache and clean up each entry */
- /* Free the object if the recount == 0 */
- memcache_cache_free(obj);
+ /* Iterate over the cache and clean up each unreferenced entry */
+ if (!apr_atomic_dec32(&obj->refcount)) {
+ cleanup_cache_object(obj);
+ }
obj = cache_pop(co->cache_cache);
}
@@ -418,7 +437,7 @@
* Note: Perhaps we should wait to put the object in the
* hash table when the object is complete? I add the object here to
* avoid multiple threads attempting to cache the same content only
- * to discover at the very end that only one of them will suceed.
+ * to discover at the very end that only one of them will succeed.
* Furthermore, adding the cache object to the table at the end could
* open up a subtle but easy to exploit DoS hole: someone could request
* a very large file with multiple requests. Better to detect this here
@@ -432,7 +451,12 @@
tmp_obj = (cache_object_t *) cache_find(sconf->cache_cache, key);
if (!tmp_obj) {
- cache_insert(sconf->cache_cache, obj);
+ cache_insert(sconf->cache_cache, obj);
+ /* Add a refcount to account for the reference by the
+ * hashtable in the cache. Refcount should be 2 now, one
+ * for this thread, and one for the cache.
+ */
+ apr_atomic_inc32(&obj->refcount);
}
if (sconf->lock) {
apr_thread_mutex_unlock(sconf->lock);
@@ -516,25 +540,33 @@
return OK;
}
+/* remove_entity()
+ * Notes:
+ * refcount should be at least 1 upon entry to this function to account
+ * for this thread's reference to the object. If the refcount is 1, then
+ * object has been removed from the cache by another thread and this thread
+ * is the last thread accessing the object.
+ */
static int remove_entity(cache_handle_t *h)
{
cache_object_t *obj = h->cache_obj;
+ cache_object_t *tobj = NULL;
- /* Remove the cache object from the cache under protection */
if (sconf->lock) {
apr_thread_mutex_lock(sconf->lock);
}
- /* If the object is not already marked for cleanup, remove
- * it from the cache and mark it for cleanup. Remember,
- * an object marked for cleanup is by design not in the
- * hash table.
+
+ /* If the entity is still in the cache, remove it and decrement the
+ * refcount. If the entity is not in the cache, do nothing. In both cases
+ * decrement_refcount called by the last thread referencing the object will
+ * trigger the cleanup.
*/
- if (!(apr_atomic_read32(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
+ tobj = cache_find(sconf->cache_cache, obj->key);
+ if (tobj == obj) {
cache_remove(sconf->cache_cache, obj);
- apr_atomic_set32(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT);
- ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, "gcing a cache entry");
+ apr_atomic_dec32(&obj->refcount);
}
-
+
if (sconf->lock) {
apr_thread_mutex_unlock(sconf->lock);
}
@@ -600,31 +632,32 @@
return APR_SUCCESS;
}
/* Define request processing hook handlers */
+/* remove_url()
+ * Notes:
+ */
static int remove_url(const char *key)
{
cache_object_t *obj;
+ int cleanup = 0;
- /* Order of the operations is important to avoid race conditions.
- * First, remove the object from the cache. Remember, all additions
- * deletions from the cache are protected by sconf->lock.
- * Increment the ref count on the object to indicate our thread
- * is accessing the object. Then set the cleanup flag on the
- * object. Remember, the cleanup flag is NEVER set on an
- * object in the hash table. If an object has the cleanup
- * flag set, it is guaranteed to NOT be in the hash table.
- */
if (sconf->lock) {
apr_thread_mutex_lock(sconf->lock);
}
obj = cache_find(sconf->cache_cache, key);
if (obj) {
- cache_remove(sconf->cache_cache, obj);
- memcache_cache_free(obj);
+ cache_remove(sconf->cache_cache, obj);
+ /* For performance, cleanup cache object after releasing the lock */
+ cleanup = !apr_atomic_dec32(&obj->refcount);
}
if (sconf->lock) {
apr_thread_mutex_unlock(sconf->lock);
}
+
+ if (cleanup) {
+ cleanup_cache_object(obj);
+ }
+
return OK;
}
@@ -787,6 +820,7 @@
{
apr_status_t rv;
cache_object_t *obj = h->cache_obj;
+ cache_object_t *tobj = NULL;
mem_cache_object_t *mobj = (mem_cache_object_t*) obj->vobj;
apr_read_type_e eblock = APR_BLOCK_READ;
apr_bucket *e;
@@ -887,25 +921,39 @@
if (sconf->lock) {
apr_thread_mutex_lock(sconf->lock);
}
- if ((apr_atomic_read32(&obj->refcount) & OBJECT_CLEANUP_BIT)) {
- /* If OBJECT_CLEANUP_BIT is set, the object has been prematurly
- * ejected from the cache by the garbage collector. Add the
- * object back to the cache. If an object with the same key is
- * found in the cache, eject it in favor of the completed obj.
+ /* Has the object been ejected from the cache?
+ */
+ tobj = (cache_object_t *) cache_find(sconf->cache_cache, obj->key);
+ if (tobj == obj) {
+ /* Object is still in the cache, remove it, update the len field then
+ * replace it under protection of sconf->lock.
*/
- cache_object_t *tmp_obj =
- (cache_object_t *) cache_find(sconf->cache_cache, obj->key);
- if (tmp_obj) {
- cache_remove(sconf->cache_cache, tmp_obj);
- memcache_cache_free(tmp_obj);
- }
- apr_atomic_set32(&obj->refcount, obj->refcount & ~OBJECT_CLEANUP_BIT);
- }
- else {
cache_remove(sconf->cache_cache, obj);
+ /* For illustration, cache no longer has reference to the object
+ * so decrement the refcount
+ * apr_atomic_dec32(&obj->refcount);
+ */
+ mobj->m_len = obj->count;
+
+ cache_insert(sconf->cache_cache, obj);
+ /* For illustration, cache now has reference to the object, so
+ * increment the refcount
+ * apr_atomic_inc32(&obj->refcount);
+ */
}
- mobj->m_len = obj->count;
- cache_insert(sconf->cache_cache, obj);
+ else if (tobj) {
+ /* Different object with the same key found in the cache. Doing nothing
+ * here will cause the object refcount to drop to 0 in decrement_refcount
+ * and the object will be cleaned up.
+ */
+
+ } else {
+ /* Object has been ejected from the cache, add it back to the cache */
+ mobj->m_len = obj->count;
+ cache_insert(sconf->cache_cache, obj);
+ apr_atomic_inc32(&obj->refcount);
+ }
+
if (sconf->lock) {
apr_thread_mutex_unlock(sconf->lock);
}