You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Brian Pane <bp...@pacbell.net> on 2001/11/16 05:35:47 UTC
[PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Greg Stein wrote:
[...]
>>I gave up trying to do full reference counting semantics for
>>duplicates of apr_mmap_t, because I couldn't find a suitable
>>pool to own the locks that would be required in a threaded
>>implementation.
>>
>
>Not sure what you mean here. I don't quite understand how threading comes
>into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
>AFAIK. And I don't think they should either...
>
When I tried to implement a traditional reference-counting solution
(all apr_mmap_t's duplicated from the same original share a reference
count, and the mmap'ed segment is destroyed only when the refcount
reaches zero), I ran into threading problems with mod_file_cache.
It's possible for two threads to be doing a dup of the same
cached apr_mmap_t at the same time--if they're both delivering the
same statically cached file, and they both end up having to call
mmap_setaside. To avoid the race condition here, we'd need a lock
around the reference count. The lock API requires that we create the
lock from a pool, but there isn't a really suitable pool from which
to allocate the lock, given that its lifetime isn't necessarily tied
to that of a request or connection.
Fortunately, this whole problem went away when I switched from the
reference-counted model to the transfer-of-ownership model. The latter
doesn't require any locking.
>>...
>>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
>>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
>>@@ -62,10 +62,15 @@
>>
>> #if APR_HAS_MMAP
>>
>>-static apr_status_t mmap_cleanup(void *themmap)
>>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
>> {
>>
>
>Why is this APR_DECLARE'd now? And why the function rename?
>
This is an error left over from an earlier design approach that I tried
and abandoned. Thanks for catching that. I've attached a new version
of the patch that fixes it.
--Brian
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Ian Holsman <ia...@cnet.com>.
Thanks Brian.
It's commited.
On Thu, 2001-11-15 at 20:35, Brian Pane wrote:
> Greg Stein wrote:
> [...]
>
> >>I gave up trying to do full reference counting semantics for
> >>duplicates of apr_mmap_t, because I couldn't find a suitable
> >>pool to own the locks that would be required in a threaded
> >>implementation.
> >>
> >
> >Not sure what you mean here. I don't quite understand how threading comes
> >into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
> >AFAIK. And I don't think they should either...
> >
>
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
>
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
>
> >>...
> >>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> >>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> >>@@ -62,10 +62,15 @@
> >>
> >> #if APR_HAS_MMAP
> >>
> >>-static apr_status_t mmap_cleanup(void *themmap)
> >>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> >> {
> >>
> >
> >Why is this APR_DECLARE'd now? And why the function rename?
> >
> This is an error left over from an earlier design approach that I tried
> and abandoned. Thanks for catching that. I've attached a new version
> of the patch that fixes it.
>
> --Brian
>
>
>
> ----
>
> Index: srclib/apr/include/apr_mmap.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_mmap.h,v
> retrieving revision 1.25
> diff -u -r1.25 apr_mmap.h
> --- srclib/apr/include/apr_mmap.h 2001/08/18 15:15:46 1.25
> +++ srclib/apr/include/apr_mmap.h 2001/11/16 04:22:14
> @@ -111,6 +111,8 @@
> void *mm;
> /** The amount of data in the mmap */
> apr_size_t size;
> + /** Whether this object is reponsible for doing the munmap */
> + int is_owner;
> };
>
> #if APR_HAS_MMAP || defined(DOXYGEN)
> @@ -134,6 +136,19 @@
> apr_file_t *file, apr_off_t offset,
> apr_size_t size, apr_int32_t flag,
> apr_pool_t *cntxt);
> +
> +/**
> + * Duplicate the specified MMAP.
> + * @param new_mmap The structure to duplicate into.
> + * @param old_mmap The file to duplicate.
> + * @param p The pool to use for the new file.
> + * @param transfer_ownership Whether responsibility for destroying
> + * the memory-mapped segment is transferred from old_mmap to new_mmap
> + */
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership);
>
> /**
> * Remove a mmap'ed.
> Index: srclib/apr/mmap/unix/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/unix/mmap.c,v
> retrieving revision 1.36
> diff -u -r1.36 mmap.c
> --- srclib/apr/mmap/unix/mmap.c 2001/08/02 04:26:53 1.36
> +++ srclib/apr/mmap/unix/mmap.c 2001/11/16 04:22:14
> @@ -83,6 +83,11 @@
> {
> apr_mmap_t *mm = themmap;
> int rv;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> #ifdef BEOS
> rv = delete_area(mm->area);
>
> @@ -159,10 +164,37 @@
> (*new)->mm = mm;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr/mmap/win32/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/win32/mmap.c,v
> retrieving revision 1.6
> diff -u -r1.6 mmap.c
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/16 04:22:14
> @@ -66,6 +66,11 @@
> {
> apr_mmap_t *mm = themmap;
> apr_status_t rv = 0;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> if (mm->mv) {
> if (!UnmapViewOfFile(mm->mv))
> {
> @@ -155,10 +160,37 @@
> (*new)->mm = (char*)((*new)->mv) + offset;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr-util/buckets/apr_buckets_mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr-util/buckets/apr_buckets_mmap.c,v
> retrieving revision 1.43
> diff -u -r1.43 apr_buckets_mmap.c
> --- srclib/apr-util/buckets/apr_buckets_mmap.c 2001/09/22 22:36:07 1.43
> +++ srclib/apr-util/buckets/apr_buckets_mmap.c 2001/11/16 04:22:14
> @@ -121,24 +121,18 @@
> {
> apr_bucket_mmap *m = data->data;
> apr_mmap_t *mm = m->mmap;
> - char *base;
> - void *addr;
> + apr_mmap_t *new_mm;
> apr_status_t ok;
>
> if (apr_pool_is_ancestor(mm->cntxt, p)) {
> return APR_SUCCESS;
> }
>
> - ok = apr_mmap_offset(&addr, m->mmap, data->start);
> + ok = apr_mmap_dup(&new_mm, mm, p, 1);
> if (ok != APR_SUCCESS) {
> return ok;
> }
> -
> - base = apr_palloc(p, data->length);
> - memcpy(base, addr, data->length);
> - data = apr_bucket_pool_make(data, base, data->length, p);
> - mmap_destroy(m);
> -
> + m->mmap = new_mm;
> return APR_SUCCESS;
> }
>
> Index: modules/cache/mod_file_cache.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/cache/mod_file_cache.c,v
> retrieving revision 1.62
> diff -u -r1.62 mod_file_cache.c
> --- modules/cache/mod_file_cache.c 2001/08/11 04:04:12 1.62
> +++ modules/cache/mod_file_cache.c 2001/11/16 04:22:15
> @@ -333,8 +333,20 @@
> #if APR_HAS_MMAP
> apr_bucket *b;
> apr_bucket_brigade *bb = apr_brigade_create(r->pool);
> + apr_mmap_t *mm;
>
> - b = apr_bucket_mmap_create(file->mm, 0, (apr_size_t)file->finfo.size);
> + /* Create a copy of the apr_mmap_t that's marked as
> + * a "non-owner." The reason for this is that the
> + * mmap bucket setaside function might later try to
> + * transfer ownership of the mmap from a request
> + * pool to a parent pool. For this particular mmap,
> + * though, we want the cache to retain ownership so
> + * that it never gets munmapped. Thus we give the
> + * bucket code a non-owner copy to work with.
> + */
> + apr_mmap_dup(&mm, file->mm, r->pool, 0);
> +
> + b = apr_bucket_mmap_create(mm, 0, (apr_size_t)file->finfo.size);
> APR_BRIGADE_INSERT_TAIL(bb, b);
> b = apr_bucket_eos_create();
> APR_BRIGADE_INSERT_TAIL(bb, b);
--
Ian Holsman IanH@cnet.com
Performance Measurement & Analysis
CNET Networks - (415) 344-2608
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Ian Holsman <ia...@cnet.com>.
Thanks Brian.
It's commited.
On Thu, 2001-11-15 at 20:35, Brian Pane wrote:
> Greg Stein wrote:
> [...]
>
> >>I gave up trying to do full reference counting semantics for
> >>duplicates of apr_mmap_t, because I couldn't find a suitable
> >>pool to own the locks that would be required in a threaded
> >>implementation.
> >>
> >
> >Not sure what you mean here. I don't quite understand how threading comes
> >into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
> >AFAIK. And I don't think they should either...
> >
>
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
>
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
>
> >>...
> >>--- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> >>+++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> >>@@ -62,10 +62,15 @@
> >>
> >> #if APR_HAS_MMAP
> >>
> >>-static apr_status_t mmap_cleanup(void *themmap)
> >>+APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> >> {
> >>
> >
> >Why is this APR_DECLARE'd now? And why the function rename?
> >
> This is an error left over from an earlier design approach that I tried
> and abandoned. Thanks for catching that. I've attached a new version
> of the patch that fixes it.
>
> --Brian
>
>
>
> ----
>
> Index: srclib/apr/include/apr_mmap.h
> ===================================================================
> RCS file: /home/cvspublic/apr/include/apr_mmap.h,v
> retrieving revision 1.25
> diff -u -r1.25 apr_mmap.h
> --- srclib/apr/include/apr_mmap.h 2001/08/18 15:15:46 1.25
> +++ srclib/apr/include/apr_mmap.h 2001/11/16 04:22:14
> @@ -111,6 +111,8 @@
> void *mm;
> /** The amount of data in the mmap */
> apr_size_t size;
> + /** Whether this object is reponsible for doing the munmap */
> + int is_owner;
> };
>
> #if APR_HAS_MMAP || defined(DOXYGEN)
> @@ -134,6 +136,19 @@
> apr_file_t *file, apr_off_t offset,
> apr_size_t size, apr_int32_t flag,
> apr_pool_t *cntxt);
> +
> +/**
> + * Duplicate the specified MMAP.
> + * @param new_mmap The structure to duplicate into.
> + * @param old_mmap The file to duplicate.
> + * @param p The pool to use for the new file.
> + * @param transfer_ownership Whether responsibility for destroying
> + * the memory-mapped segment is transferred from old_mmap to new_mmap
> + */
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership);
>
> /**
> * Remove a mmap'ed.
> Index: srclib/apr/mmap/unix/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/unix/mmap.c,v
> retrieving revision 1.36
> diff -u -r1.36 mmap.c
> --- srclib/apr/mmap/unix/mmap.c 2001/08/02 04:26:53 1.36
> +++ srclib/apr/mmap/unix/mmap.c 2001/11/16 04:22:14
> @@ -83,6 +83,11 @@
> {
> apr_mmap_t *mm = themmap;
> int rv;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> #ifdef BEOS
> rv = delete_area(mm->area);
>
> @@ -159,10 +164,37 @@
> (*new)->mm = mm;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr/mmap/win32/mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr/mmap/win32/mmap.c,v
> retrieving revision 1.6
> diff -u -r1.6 mmap.c
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/16 04:22:14
> @@ -66,6 +66,11 @@
> {
> apr_mmap_t *mm = themmap;
> apr_status_t rv = 0;
> +
> + if (!mm->is_owner) {
> + return APR_SUCCESS;
> + }
> +
> if (mm->mv) {
> if (!UnmapViewOfFile(mm->mv))
> {
> @@ -155,10 +160,37 @@
> (*new)->mm = (char*)((*new)->mv) + offset;
> (*new)->size = size;
> (*new)->cntxt = cont;
> + (*new)->is_owner = 1;
>
> /* register the cleanup... */
> apr_pool_cleanup_register((*new)->cntxt, (void*)(*new), mmap_cleanup,
> apr_pool_cleanup_null);
> + return APR_SUCCESS;
> +}
> +
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> + *new_mmap = (apr_mmap_t *)apr_pmemdup(p, old_mmap, sizeof(apr_mmap_t));
> + (*new_mmap)->cntxt = p;
> +
> + /* The old_mmap can transfer ownership only if the old_mmap itself
> + * is an owner of the mmap'ed segment.
> + */
> + if (old_mmap->is_owner) {
> + if (transfer_ownership) {
> + (*new_mmap)->is_owner = 1;
> + old_mmap->is_owner = 0;
> + apr_pool_cleanup_kill(old_mmap->cntxt, old_mmap, mmap_cleanup);
> + }
> + else {
> + (*new_mmap)->is_owner = 0;
> + }
> + apr_pool_cleanup_register(p, *new_mmap, mmap_cleanup,
> + apr_pool_cleanup_null);
> + }
> return APR_SUCCESS;
> }
>
> Index: srclib/apr-util/buckets/apr_buckets_mmap.c
> ===================================================================
> RCS file: /home/cvspublic/apr-util/buckets/apr_buckets_mmap.c,v
> retrieving revision 1.43
> diff -u -r1.43 apr_buckets_mmap.c
> --- srclib/apr-util/buckets/apr_buckets_mmap.c 2001/09/22 22:36:07 1.43
> +++ srclib/apr-util/buckets/apr_buckets_mmap.c 2001/11/16 04:22:14
> @@ -121,24 +121,18 @@
> {
> apr_bucket_mmap *m = data->data;
> apr_mmap_t *mm = m->mmap;
> - char *base;
> - void *addr;
> + apr_mmap_t *new_mm;
> apr_status_t ok;
>
> if (apr_pool_is_ancestor(mm->cntxt, p)) {
> return APR_SUCCESS;
> }
>
> - ok = apr_mmap_offset(&addr, m->mmap, data->start);
> + ok = apr_mmap_dup(&new_mm, mm, p, 1);
> if (ok != APR_SUCCESS) {
> return ok;
> }
> -
> - base = apr_palloc(p, data->length);
> - memcpy(base, addr, data->length);
> - data = apr_bucket_pool_make(data, base, data->length, p);
> - mmap_destroy(m);
> -
> + m->mmap = new_mm;
> return APR_SUCCESS;
> }
>
> Index: modules/cache/mod_file_cache.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/cache/mod_file_cache.c,v
> retrieving revision 1.62
> diff -u -r1.62 mod_file_cache.c
> --- modules/cache/mod_file_cache.c 2001/08/11 04:04:12 1.62
> +++ modules/cache/mod_file_cache.c 2001/11/16 04:22:15
> @@ -333,8 +333,20 @@
> #if APR_HAS_MMAP
> apr_bucket *b;
> apr_bucket_brigade *bb = apr_brigade_create(r->pool);
> + apr_mmap_t *mm;
>
> - b = apr_bucket_mmap_create(file->mm, 0, (apr_size_t)file->finfo.size);
> + /* Create a copy of the apr_mmap_t that's marked as
> + * a "non-owner." The reason for this is that the
> + * mmap bucket setaside function might later try to
> + * transfer ownership of the mmap from a request
> + * pool to a parent pool. For this particular mmap,
> + * though, we want the cache to retain ownership so
> + * that it never gets munmapped. Thus we give the
> + * bucket code a non-owner copy to work with.
> + */
> + apr_mmap_dup(&mm, file->mm, r->pool, 0);
> +
> + b = apr_bucket_mmap_create(mm, 0, (apr_size_t)file->finfo.size);
> APR_BRIGADE_INSERT_TAIL(bb, b);
> b = apr_bucket_eos_create();
> APR_BRIGADE_INSERT_TAIL(bb, b);
--
Ian Holsman IanH@cnet.com
Performance Measurement & Analysis
CNET Networks - (415) 344-2608
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 16 Nov 2001, Greg Stein wrote:
> > around the reference count. The lock API requires that we create the
> > lock from a pool, but there isn't a really suitable pool from which
> > to allocate the lock, given that its lifetime isn't necessarily tied
> > to that of a request or connection.
>
> The "suitable pool" is that of the apr_mmap_t itself. Note that the file
> cache would be putting that lock into its own long-lived pool.
>
> Not so hard, or I'm missing something :-)
You're missing something. :-)
Let's say you have a pool hierarchy like this:
a
/ \
b c
/
d
You have an apr_mmap_t allocated out of c. You request to dup the mmap
into b. Will b live longer than c? No way to know. Will c live longer
than b? No way to know. So if you've allocated your refcount or the lock
on that refcount out of c, and c goes away before b does, then b has a
reference to no-longer-existing memory.
This is not an odd situation: it's almost exactly what's happening in
mod_file_cache right now. c is the cmd->pool. b is the c->pool. d is
the r->pool. (There might be some pools between a and b for the diagram
to be exactly right, but the hierarchy is essentially correct I believe.)
Granted, in that particular example, c lives much, much longer than b, but
in general that's not at all guaranteed to be the case.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 16 Nov 2001, Greg Stein wrote:
> > around the reference count. The lock API requires that we create the
> > lock from a pool, but there isn't a really suitable pool from which
> > to allocate the lock, given that its lifetime isn't necessarily tied
> > to that of a request or connection.
>
> The "suitable pool" is that of the apr_mmap_t itself. Note that the file
> cache would be putting that lock into its own long-lived pool.
>
> Not so hard, or I'm missing something :-)
You're missing something. :-)
Let's say you have a pool hierarchy like this:
a
/ \
b c
/
d
You have an apr_mmap_t allocated out of c. You request to dup the mmap
into b. Will b live longer than c? No way to know. Will c live longer
than b? No way to know. So if you've allocated your refcount or the lock
on that refcount out of c, and c goes away before b does, then b has a
reference to no-longer-existing memory.
This is not an odd situation: it's almost exactly what's happening in
mod_file_cache right now. c is the cmd->pool. b is the c->pool. d is
the r->pool. (There might be some pools between a and b for the diagram
to be exactly right, but the hierarchy is essentially correct I believe.)
Granted, in that particular example, c lives much, much longer than b, but
in general that's not at all guaranteed to be the case.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Nov 15, 2001 at 08:35:47PM -0800, Brian Pane wrote:
>...
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
The "suitable pool" is that of the apr_mmap_t itself. Note that the file
cache would be putting that lock into its own long-lived pool.
Not so hard, or I'm missing something :-)
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
Yah.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Nov 15, 2001 at 08:35:47PM -0800, Brian Pane wrote:
>...
> When I tried to implement a traditional reference-counting solution
> (all apr_mmap_t's duplicated from the same original share a reference
> count, and the mmap'ed segment is destroyed only when the refcount
> reaches zero), I ran into threading problems with mod_file_cache.
> It's possible for two threads to be doing a dup of the same
> cached apr_mmap_t at the same time--if they're both delivering the
> same statically cached file, and they both end up having to call
> mmap_setaside. To avoid the race condition here, we'd need a lock
> around the reference count. The lock API requires that we create the
> lock from a pool, but there isn't a really suitable pool from which
> to allocate the lock, given that its lifetime isn't necessarily tied
> to that of a request or connection.
The "suitable pool" is that of the apr_mmap_t itself. Note that the file
cache would be putting that lock into its own long-lived pool.
Not so hard, or I'm missing something :-)
> Fortunately, this whole problem went away when I switched from the
> reference-counted model to the transfer-of-ownership model. The latter
> doesn't require any locking.
Yah.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/