|
The following patch is a rework of the refcount and cleanup
fields in mod_mem_cache.c. I replaced the cleanup field with a bit set
in refcount. This is done to prevent race conditions when refcount
is accessed on two different threads/CPUS. The complete description
was done on that mail thread earlier.
The race condition is fixed, no performance hit,
and no memory leak, from what I can tell.
NOT all the cases have been tested. I will do that on Monday, in the
mean time I will appreciate if the patch could be reviewed by others.
Thank you very much,
Jean-Jacques
Index: modules/experimental/mod_cache.h
=================================================================== RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_cache.h,v retrieving revision 1.36.2.8 diff -u -r1.36.2.8 mod_cache.h --- modules/experimental/mod_cache.h 26 Aug 2004 18:35:13 -0000 1.36.2.8 +++ modules/experimental/mod_cache.h 10 Sep 2004 22:22:09 -0000 @@ -173,7 +173,6 @@ apr_size_t count; /* Number of body bytes written to the cache so far */ int complete; apr_atomic_t refcount; - apr_size_t cleanup; }; typedef struct cache_handle cache_handle_t; Index: modules/experimental/mod_mem_cache.c =================================================================== RCS file: /home/cvs/httpd-2.0/modules/experimental/mod_mem_cache.c,v retrieving revision 1.88.2.9 diff -u -r1.88.2.9 mod_mem_cache.c --- modules/experimental/mod_mem_cache.c 26 Aug 2004 16:59:44 -0000 1.88.2.9 +++ modules/experimental/mod_mem_cache.c 10 Sep 2004 22:52:13 -0000 @@ -88,6 +88,8 @@ #define DEFAULT_MAX_STREAMING_BUFFER_SIZE 100000 #define CACHEFILE_LEN 20 +#define OBJECT_CLEANUP_BIT 0x00F00000 + /* 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); @@ -152,17 +154,15 @@ { cache_object_t *obj = (cache_object_t *)a; - /* Cleanup the cache object. Object should be removed from the cache - * now. Increment the refcount before setting cleanup to avoid a race - * condition. A similar pattern is used in remove_url() - */ - apr_atomic_inc(&obj->refcount); - - obj->cleanup = 1; - - if (!apr_atomic_dec(&obj->refcount)) { + apr_atomic_t tmp_refcount = obj->refcount; + /* Cleanup the cache object. Object should be removed from the cache now. */ + if (!apr_atomic_read(&obj->refcount)) { cleanup_cache_object(obj); } + else if(tmp_refcount != apr_atomic_cas(&obj->refcount, tmp_refcount | OBJECT_CLEANUP_BIT, tmp_refcount)) { + memcache_cache_free(obj); + } + return; } /* * functions return a 'negative' score since priority queues @@ -276,11 +276,11 @@ } /* 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 obj->cleanup) + * removed the object from the cache (and set the OBJECT_CLEANUP_BIT) */ - if (!obj->cleanup) { + if (!(apr_atomic_read(&obj->refcount) & OBJECT_CLEANUP_BIT)) { cache_remove(sconf->cache_cache, obj); - obj->cleanup = 1; + apr_atomic_set(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT); } if (sconf->lock) { apr_thread_mutex_unlock(sconf->lock); @@ -288,10 +288,8 @@ } /* Cleanup the cache object */ - if (!apr_atomic_dec(&obj->refcount)) { - if (obj->cleanup) { - cleanup_cache_object(obj); - } + if(apr_atomic_dec(&obj->refcount) == OBJECT_CLEANUP_BIT) { + cleanup_cache_object(obj); } return APR_SUCCESS; } @@ -314,11 +312,7 @@ while (obj) { /* Iterate over the cache and clean up each entry */ /* Free the object if the recount == 0 */ - apr_atomic_inc(&obj->refcount); - obj->cleanup = 1; - if (!apr_atomic_dec(&obj->refcount)) { - cleanup_cache_object(obj); - } + memcache_cache_free(obj); obj = cache_pop(co->cache_cache); } @@ -415,7 +409,6 @@ apr_atomic_set(&obj->refcount, 1); mobj->total_refs = 1; obj->complete = 0; - obj->cleanup = 0; obj->vobj = mobj; /* Safe cast: We tested < sconf->max_cache_object_size above */ mobj->m_len = (apr_size_t)len; @@ -536,9 +529,9 @@ * an object marked for cleanup is by design not in the * hash table. */ - if (!obj->cleanup) { + if (!(obj->refcount & OBJECT_CLEANUP_BIT)) { cache_remove(sconf->cache_cache, obj); - obj->cleanup = 1; + apr_atomic_set(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT); ap_log_error(APLOG_MARK, APLOG_INFO, 0, NULL, "gcing a cache entry"); } @@ -629,20 +622,13 @@ mem_cache_object_t *mobj; cache_remove(sconf->cache_cache, obj); mobj = (mem_cache_object_t *) obj->vobj; - - /* Refcount increment in this case MUST be made under - * protection of the lock - */ - apr_atomic_inc(&obj->refcount); - if (obj) { - obj->cleanup = 1; - } + apr_atomic_set(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT); } if (sconf->lock) { apr_thread_mutex_unlock(sconf->lock); } if (obj) { - if (!apr_atomic_dec(&obj->refcount)) { + if(apr_atomic_read(&obj->refcount) == OBJECT_CLEANUP_BIT) { cleanup_cache_object(obj); } } @@ -908,8 +894,8 @@ if (sconf->lock) { apr_thread_mutex_lock(sconf->lock); } - if (obj->cleanup) { - /* If obj->cleanup is set, the object has been prematurly + if ((apr_atomic_read(&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. @@ -918,12 +904,12 @@ (cache_object_t *) cache_find(sconf->cache_cache, obj->key); if (tmp_obj) { cache_remove(sconf->cache_cache, tmp_obj); - tmp_obj->cleanup = 1; - if (!tmp_obj->refcount) { + apr_atomic_set(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT); + if (!apr_atomic_read(&obj->refcount) & OBJECT_CLEANUP_BIT) { cleanup_cache_object(tmp_obj); } } - obj->cleanup = 0; + apr_atomic_set(&obj->refcount, obj->refcount & ~OBJECT_CLEANUP_BIT); } else { cache_remove(sconf->cache_cache, obj); |
mod_mem_cache_race4.patch
Description: Binary data
